Re: [PATCH kvmtool v1 06/17] Use memfd for hugetlbfs when allocating guest ram

[Alexandru Elisei <alexandru.elisei@arm.com>]

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@google.com>
> ---
>  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
>