Message-ID: <CAJHvVchQUz9VnpSsh_3jJ7o6qKMF0-NWhw42E7tO8m6+R9Uf8g@mail.gmail.com>
Date:   Thu, 13 May 2021 15:02:28 -0700
From:   Axel Rasmussen <axelrasmussen@...gle.com>
To:     Ben Gardon <bgardon@...gle.com>
Cc:     Aaron Lewis <aaronlewis@...gle.com>,
        Alexander Graf <graf@...zon.com>,
        Andrew Jones <drjones@...hat.com>,
        Andrew Morton <akpm@...ux-foundation.org>,
        Emanuele Giuseppe Esposito <eesposit@...hat.com>,
        Eric Auger <eric.auger@...hat.com>,
        Jacob Xu <jacobhxu@...gle.com>,
        Makarand Sonare <makarandsonare@...gle.com>,
        Oliver Upton <oupton@...gle.com>,
        Paolo Bonzini <pbonzini@...hat.com>,
        Peter Xu <peterx@...hat.com>, Shuah Khan <shuah@...nel.org>,
        Yanan Wang <wangyanan55@...wei.com>, kvm <kvm@...r.kernel.org>,
        LKML <linux-kernel@...r.kernel.org>,
        linux-kselftest@...r.kernel.org
Subject: Re: [PATCH 1/5] KVM: selftests: allow different backing memory types
 for demand paging

On Thu, May 13, 2021 at 2:32 PM Ben Gardon <bgardon@...gle.com> wrote:
>
> On Wed, May 12, 2021 at 2:45 PM Axel Rasmussen <axelrasmussen@...gle.com> wrote:
> >
> > Add an argument which lets us specify a different backing memory type
> > for the test. The default is just to use anonymous, matching existing
> > behavior (if the argument is omitted).
> >
> > This is in preparation for testing UFFD minor faults. For that, we need
> > to use a new backing memory type which is setup with MAP_SHARED.
> >
> > This notably requires one other change. Perhaps counter-intuitively,
> > perf_test_args.host_page_size is the host's *native* page size, not the
> > size of the pages the host is using to back the guest. This means, if we
> > try to run the test with e.g. VM_MEM_SRC_ANONYMOUS_HUGETLB, we'll try to
> > do demand paging with 4k pages instead of 2M hugepages.
>
> Would it make sense to factor this change out into another commit
> preceding this one? Perhaps only worth it if you send a v2.
>
> When you say "we'll try to do demand paging with 4k pages instead of
> 2M hugepages," what would that mean? Would we only copy 4k worth of
> the contents of the 2M page in, leading to the guest seeing bad
> memory? Do we have the capability to do demand paging at a smaller
> granularity than the backing page size with UFFD?

Basically I think the existing behavior is to always use 4k - so we
UFFDIO_COPY that size at a time.

This is totally fine for anonymous, or even for THPs. But, once we're
using hugetlbfs, the UFFDIO_COPY ioctl will just fail unless we do 2M
(or some multiple thereof) at a time. If memory serves it returns
-EINVAL in this case.

It wouldn't be so much trouble to factor it out, so just let me know
what is preferred. For now, I'll do it if a v2 is needed for other
reasons.

>
> Otherwise this patch looks reasonable to me. I'll try to review the
> rest of your patches today / Monday.
>
> >
> > So, convert everything to use a new demand_paging_size, computed based
> > on the backing memory type.
> >
> > Signed-off-by: Axel Rasmussen <axelrasmussen@...gle.com>
> > ---
> >  .../selftests/kvm/demand_paging_test.c        | 24 +++++++++++++------
> >  1 file changed, 17 insertions(+), 7 deletions(-)
> >
> > diff --git a/tools/testing/selftests/kvm/demand_paging_test.c b/tools/testing/selftests/kvm/demand_paging_test.c
> > index 5f7a229c3af1..10c7ba76a9c6 100644
> > --- a/tools/testing/selftests/kvm/demand_paging_test.c
> > +++ b/tools/testing/selftests/kvm/demand_paging_test.c
> > @@ -38,6 +38,7 @@
> >
> >  static int nr_vcpus = 1;
> >  static uint64_t guest_percpu_mem_size = DEFAULT_PER_VCPU_MEM_SIZE;
> > +static size_t demand_paging_size;
> >  static char *guest_data_prototype;
> >
> >  static void *vcpu_worker(void *data)
> > @@ -83,7 +84,7 @@ static int handle_uffd_page_request(int uffd, uint64_t addr)
> >
> >         copy.src = (uint64_t)guest_data_prototype;
> >         copy.dst = addr;
> > -       copy.len = perf_test_args.host_page_size;
> > +       copy.len = demand_paging_size;
> >         copy.mode = 0;
> >
> >         clock_gettime(CLOCK_MONOTONIC, &start);
> > @@ -100,7 +101,7 @@ static int handle_uffd_page_request(int uffd, uint64_t addr)
> >         PER_PAGE_DEBUG("UFFDIO_COPY %d \t%ld ns\n", tid,
> >                        timespec_to_ns(ts_diff));
> >         PER_PAGE_DEBUG("Paged in %ld bytes at 0x%lx from thread %d\n",
> > -                      perf_test_args.host_page_size, addr, tid);
> > +                      demand_paging_size, addr, tid);
> >
> >         return 0;
> >  }
> > @@ -250,6 +251,7 @@ static int setup_demand_paging(struct kvm_vm *vm,
> >  struct test_params {
> >         bool use_uffd;
> >         useconds_t uffd_delay;
> > +       enum vm_mem_backing_src_type src_type;
> >         bool partition_vcpu_memory_access;
> >  };
> >
> > @@ -267,14 +269,16 @@ static void run_test(enum vm_guest_mode mode, void *arg)
> >         int r;
> >
> >         vm = perf_test_create_vm(mode, nr_vcpus, guest_percpu_mem_size,
> > -                                VM_MEM_SRC_ANONYMOUS);
> > +                                p->src_type);
> >
> >         perf_test_args.wr_fract = 1;
> >
> > -       guest_data_prototype = malloc(perf_test_args.host_page_size);
> > +       demand_paging_size = get_backing_src_pagesz(p->src_type);
> > +
> > +       guest_data_prototype = malloc(demand_paging_size);
> >         TEST_ASSERT(guest_data_prototype,
> >                     "Failed to allocate buffer for guest data pattern");
> > -       memset(guest_data_prototype, 0xAB, perf_test_args.host_page_size);
> > +       memset(guest_data_prototype, 0xAB, demand_paging_size);
> >
> >         vcpu_threads = malloc(nr_vcpus * sizeof(*vcpu_threads));
> >         TEST_ASSERT(vcpu_threads, "Memory allocation failed");
> > @@ -388,7 +392,7 @@ static void help(char *name)
> >  {
> >         puts("");
> >         printf("usage: %s [-h] [-m mode] [-u] [-d uffd_delay_usec]\n"
> > -              "          [-b memory] [-v vcpus] [-o]\n", name);
> > +              "          [-b memory] [-t type] [-v vcpus] [-o]\n", name);
> >         guest_modes_help();
> >         printf(" -u: use User Fault FD to handle vCPU page\n"
> >                "     faults.\n");
> > @@ -398,6 +402,8 @@ static void help(char *name)
> >         printf(" -b: specify the size of the memory region which should be\n"
> >                "     demand paged by each vCPU. e.g. 10M or 3G.\n"
> >                "     Default: 1G\n");
> > +       printf(" -t: The type of backing memory to use. Default: anonymous\n");
> > +       backing_src_help();
> >         printf(" -v: specify the number of vCPUs to run.\n");
> >         printf(" -o: Overlap guest memory accesses instead of partitioning\n"
> >                "     them into a separate region of memory for each vCPU.\n");
> > @@ -409,13 +415,14 @@ int main(int argc, char *argv[])
> >  {
> >         int max_vcpus = kvm_check_cap(KVM_CAP_MAX_VCPUS);
> >         struct test_params p = {
> > +               .src_type = VM_MEM_SRC_ANONYMOUS,
> >                 .partition_vcpu_memory_access = true,
> >         };
> >         int opt;
> >
> >         guest_modes_append_default();
> >
> > -       while ((opt = getopt(argc, argv, "hm:ud:b:v:o")) != -1) {
> > +       while ((opt = getopt(argc, argv, "hm:ud:b:t:v:o")) != -1) {
> >                 switch (opt) {
> >                 case 'm':
> >                         guest_modes_cmdline(optarg);
> > @@ -430,6 +437,9 @@ int main(int argc, char *argv[])
> >                 case 'b':
> >                         guest_percpu_mem_size = parse_size(optarg);
> >                         break;
> > +               case 't':
> > +                       p.src_type = parse_backing_src_type(optarg);
> > +                       break;
> >                 case 'v':
> >                         nr_vcpus = atoi(optarg);
> >                         TEST_ASSERT(nr_vcpus > 0 && nr_vcpus <= max_vcpus,
> > --
> > 2.31.1.607.g51e8a6a459-goog
> >

Powered by blists - more mailing lists