Re: [RFC PATCH v2 6/7] KVM: selftests: Adapt vm_userspace_mem_region_add to new helpers

[wangyanan (Y)]

On 2021/2/26 7:44, Ben Gardon wrote:
> On Wed, Feb 24, 2021 at 10:03 PM Yanan Wang <wangyanan55@xxxxxxxxxx> wrote:
> > With VM_MEM_SRC_ANONYMOUS_THP specified in vm_userspace_mem_region_add(),
> > we have to get the transparent hugepage size for HVA alignment. With the
> > new helpers, we can use get_backing_src_pagesz() to check whether THP is
> > configured and then get the exact configured hugepage size.
> >
> > As different architectures may have different THP page sizes configured,
> > this can get the accurate THP page sizes on any platform.
> >
> > Signed-off-by: Yanan Wang <wangyanan55@xxxxxxxxxx>
> > ---
> >   tools/testing/selftests/kvm/lib/kvm_util.c | 27 +++++++---------------
> >   1 file changed, 8 insertions(+), 19 deletions(-)
> >
> > diff --git a/tools/testing/selftests/kvm/lib/kvm_util.c b/tools/testing/selftests/kvm/lib/kvm_util.c
> > index b91c8e3a7ee1..0105fbfed036 100644
> > --- a/tools/testing/selftests/kvm/lib/kvm_util.c
> > +++ b/tools/testing/selftests/kvm/lib/kvm_util.c
> > @@ -18,7 +18,6 @@
> >   #include <unistd.h>
> >   #include <linux/kernel.h>
> >
> > -#define KVM_UTIL_PGS_PER_HUGEPG 512
> >   #define KVM_UTIL_MIN_PFN       2
> >
> >   /* Aligns x up to the next multiple of size. Size must be a power of 2. */
> > @@ -686,7 +685,7 @@ void vm_userspace_mem_region_add(struct kvm_vm *vm,
> >   {
> >          int ret;
> >          struct userspace_mem_region *region;
> > -       size_t huge_page_size = KVM_UTIL_PGS_PER_HUGEPG * vm->page_size;
> > +       size_t backing_src_pagesz = get_backing_src_pagesz(src_type);
> >          size_t alignment;
> >
> >          TEST_ASSERT(vm_adjust_num_guest_pages(vm->mode, npages) == npages,
> > @@ -748,7 +747,7 @@ void vm_userspace_mem_region_add(struct kvm_vm *vm,
> >   #endif
> >
> >          if (src_type == VM_MEM_SRC_ANONYMOUS_THP)
> > -               alignment = max(huge_page_size, alignment);
> > +               alignment = max(backing_src_pagesz, alignment);
> >
> >          /* Add enough memory to align up if necessary */
> >          if (alignment > 1)
> > @@ -767,22 +766,12 @@ void vm_userspace_mem_region_add(struct kvm_vm *vm,
> >          region->host_mem = align(region->mmap_start, alignment);
> >
> >          /* As needed perform madvise */
> > -       if (src_type == VM_MEM_SRC_ANONYMOUS || src_type == VM_MEM_SRC_ANONYMOUS_THP) {
> > -               struct stat statbuf;
> > -
> > -               ret = stat("/sys/kernel/mm/transparent_hugepage", &statbuf);
> > -               TEST_ASSERT(ret == 0 || (ret == -1 && errno == ENOENT),
> > -                           "stat /sys/kernel/mm/transparent_hugepage");
> > -
> > -               TEST_ASSERT(ret == 0 || src_type != VM_MEM_SRC_ANONYMOUS_THP,
> > -                           "VM_MEM_SRC_ANONYMOUS_THP requires THP to be configured in the host kernel");
> > -
> > -               if (ret == 0) {
> > -                       ret = madvise(region->host_mem, npages * vm->page_size,
> > -                                     src_type == VM_MEM_SRC_ANONYMOUS ? MADV_NOHUGEPAGE : MADV_HUGEPAGE);
> > -                       TEST_ASSERT(ret == 0, "madvise failed, addr: %p length: 0x%lx src_type: %x",
> > -                                   region->host_mem, npages * vm->page_size, src_type);
> > -               }
> > +       if (src_type <= VM_MEM_SRC_ANONYMOUS_THP && thp_configured()) {
> This check relies on an unstated property of the backing src type
> enums where VM_MEM_SRC_ANONYMOUS and VM_MEM_SRC_ANONYMOUS_THP are
> declared first.
> It would probably be more readable for folks if the check was explicit:
> if ((src_type == VM_MEM_SRC_ANONYMOUS || src_type ==
> VM_MEM_SRC_ANONYMOUS_THP) && thp_configured()) {
Yes, this makes sense, I will fix it.

Thanks,

Yanan

> > +               ret = madvise(region->host_mem, npages * vm->page_size,
> > +                             src_type == VM_MEM_SRC_ANONYMOUS ? MADV_NOHUGEPAGE : MADV_HUGEPAGE);
> > +               TEST_ASSERT(ret == 0, "madvise failed, addr: %p length: 0x%lx src_type: %s",
> > +                           region->host_mem, npages * vm->page_size,
> > +                           vm_mem_backing_src_alias(src_type)->name);
> >          }
> >
> >          region->unused_phy_pages = sparsebit_alloc();
> > --
> > 2.19.1
> .