Hi,
On Thu, Nov 24, 2022 at 10:19 AM Alexandru Elisei
<alexandru.elisei@xxxxxxx> wrote:
>
> 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.
I'm referring for the need to create a temporary file in the hugetlbfs path.
>
> >
> > 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?
Ack. I'll replace them with 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");
This is simpler, yes.
>
> 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.
Ack.
>
> > +
> > 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.
Will do.
Cheers,
/fuad
>
> 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
> >
