Hi,
On Tue, Nov 15, 2022 at 11:15:38AM +0000, Fuad Tabba wrote:
> This removes the need of using a temporary file for the fd.
I'm confused by this. The man page for memfd_create says that it creates an
anonymous file that lives in RAM.
>
> Signed-off-by: Fuad Tabba <tabba@xxxxxxxxxx>
> ---
> util/util.c | 25 ++++++++++++++++++++-----
> 1 file changed, 20 insertions(+), 5 deletions(-)
>
> diff --git a/util/util.c b/util/util.c
> index e6c0951..d6ceb5d 100644
> --- a/util/util.c
> +++ b/util/util.c
> @@ -10,6 +10,14 @@
> #include <sys/stat.h>
> #include <sys/statfs.h>
>
> +#ifndef MFD_HUGETLB
> +#define MFD_HUGETLB 0x0004U
> +#endif
> +
> +#ifndef MFD_HUGE_SHIFT
> +#define MFD_HUGE_SHIFT 26
> +#endif
Hm... on my machine these are defined in linux/memfd.h, maybe you are
missing the include?
> +
> static void report(const char *prefix, const char *err, va_list params)
> {
> char msg[1024];
> @@ -96,10 +104,12 @@ static u64 get_hugepage_blk_size(const char *htlbfs_path)
>
> static void *mmap_hugetlbfs(struct kvm *kvm, const char *htlbfs_path, u64 size)
> {
> - char mpath[PATH_MAX];
> + const char *name = "kvmtool";
> + unsigned int flags = 0;
> int fd;
> void *addr;
> u64 blk_size;
> + int htsize;
>
> blk_size = get_hugepage_blk_size(htlbfs_path);
> if (blk_size == 0 || blk_size > size) {
> @@ -107,13 +117,18 @@ static void *mmap_hugetlbfs(struct kvm *kvm, const char *htlbfs_path, u64 size)
> (unsigned long long)blk_size, (unsigned long long)size);
> }
>
> + htsize = __builtin_ctzl(blk_size);
> + if ((1ULL << htsize) != blk_size)
> + die("Hugepage size must be a power of 2.\n");
> +
> + flags |= MFD_HUGETLB;
> + flags |= htsize << MFD_HUGE_SHIFT;
If I understand the intention correctly, this entire sequence can be
rewritten using is_power_of_two() from util.h:
if (!is_power_of_two(blk_size))
die("Hugepage size must be a power of 2");
flags |= MFD_HUGETLB;
flags |= blk_size << MFD_HUGE_SHIFT;
Also, die() automatically adds the newline at the end of the string.
That's unless you specifically wanted two newline characters at the end of
the message.
> +
> kvm->ram_pagesize = blk_size;
>
> - snprintf(mpath, PATH_MAX, "%s/kvmtoolXXXXXX", htlbfs_path);
> - fd = mkstemp(mpath);
> + fd = memfd_create(name, flags);
> if (fd < 0)
> - die("Can't open %s for hugetlbfs map\n", mpath);
> - unlink(mpath);
> + die("Can't memfd_create for hugetlbfs map\n");
die_perror("memfd_create")? That way you also print the error number and
the message associated with it. Same thing with the other die statements
here, replacing them with die_perror() looks like it would be helpful.
Thanks,
Alex
> if (ftruncate(fd, size) < 0)
> die("Can't ftruncate for mem mapping size %lld\n",
> (unsigned long long)size);
> --
> 2.38.1.431.g37b22c650d-goog
>
