Message-ID: <CAJHvVcj9FpJgC5EsB1_6yfgDA8p5-=Oakccg+TUrGUWZB5nHzA@mail.gmail.com>
Date:   Wed, 20 Oct 2021 11:28:49 -0700
From:   Axel Rasmussen <axelrasmussen@...gle.com>
To:     Andrew Morton <akpm@...ux-foundation.org>,
        Peter Xu <peterx@...hat.com>, Shuah Khan <shuah@...nel.org>
Cc:     Linux MM <linux-mm@...ck.org>,
        Linuxkselftest <linux-kselftest@...r.kernel.org>,
        LKML <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v2 2/3] userfaultfd/selftests: fix feature support detection

Just a friendly bump for review. :) Peter, any objections to this
version? I think it fairly closely matches your suggestions from v1.

On Thu, Sep 30, 2021 at 2:23 PM Axel Rasmussen <axelrasmussen@...gle.com> wrote:
>
> Before any tests are run, in set_test_type, we decide what feature(s) we
> are going to be testing, based upon our command line arguments. However,
> the supported features are not just a function of the memory type being
> used, so this is broken.
>
> For instance, consider writeprotect support. It is "normally" supported
> for anonymous memory, but furthermore it requires that the kernel has
> CONFIG_HAVE_ARCH_USERFAULTFD_WP. So, it is *not* supported at all on
> aarch64, for example.
>
> So, this commit fixes this by querying the kernel for the set of
> features it supports in set_test_type, by opening a userfaultfd and
> issuing a UFFDIO_API ioctl. Based upon the reported features, we toggle
> what tests are enabled.
>
> Signed-off-by: Axel Rasmussen <axelrasmussen@...gle.com>
> ---
>  tools/testing/selftests/vm/userfaultfd.c | 54 ++++++++++++++----------
>  1 file changed, 31 insertions(+), 23 deletions(-)
>
> diff --git a/tools/testing/selftests/vm/userfaultfd.c b/tools/testing/selftests/vm/userfaultfd.c
> index 2a71a91559a7..00d1b7555865 100644
> --- a/tools/testing/selftests/vm/userfaultfd.c
> +++ b/tools/testing/selftests/vm/userfaultfd.c
> @@ -346,6 +346,16 @@ static struct uffd_test_ops hugetlb_uffd_test_ops = {
>
>  static struct uffd_test_ops *uffd_test_ops;
>
> +static inline uint64_t uffd_minor_feature(void)
> +{
> +       if (test_type == TEST_HUGETLB && map_shared)
> +               return UFFD_FEATURE_MINOR_HUGETLBFS;
> +       else if (test_type == TEST_SHMEM)
> +               return UFFD_FEATURE_MINOR_SHMEM;
> +       else
> +               return 0;
> +}
> +
>  static void userfaultfd_open(uint64_t *features)
>  {
>         struct uffdio_api uffdio_api;
> @@ -406,7 +416,7 @@ static void uffd_test_ctx_clear(void)
>         munmap_area((void **)&area_dst_alias);
>  }
>
> -static void uffd_test_ctx_init_ext(uint64_t *features)
> +static void uffd_test_ctx_init(uint64_t features)
>  {
>         unsigned long nr, cpu;
>
> @@ -418,7 +428,7 @@ static void uffd_test_ctx_init_ext(uint64_t *features)
>         uffd_test_ops->release_pages(area_src);
>         uffd_test_ops->release_pages(area_dst);
>
> -       userfaultfd_open(features);
> +       userfaultfd_open(&features);
>
>         count_verify = malloc(nr_pages * sizeof(unsigned long long));
>         if (!count_verify)
> @@ -446,11 +456,6 @@ static void uffd_test_ctx_init_ext(uint64_t *features)
>                         err("pipe");
>  }
>
> -static inline void uffd_test_ctx_init(uint64_t features)
> -{
> -       uffd_test_ctx_init_ext(&features);
> -}
> -
>  static int my_bcmp(char *str1, char *str2, size_t n)
>  {
>         unsigned long i;
> @@ -1191,7 +1196,6 @@ static int userfaultfd_minor_test(void)
>         void *expected_page;
>         char c;
>         struct uffd_stats stats = { 0 };
> -       uint64_t req_features, features_out;
>
>         if (!test_uffdio_minor)
>                 return 0;
> @@ -1199,21 +1203,7 @@ static int userfaultfd_minor_test(void)
>         printf("testing minor faults: ");
>         fflush(stdout);
>
> -       if (test_type == TEST_HUGETLB)
> -               req_features = UFFD_FEATURE_MINOR_HUGETLBFS;
> -       else if (test_type == TEST_SHMEM)
> -               req_features = UFFD_FEATURE_MINOR_SHMEM;
> -       else
> -               return 1;
> -
> -       features_out = req_features;
> -       uffd_test_ctx_init_ext(&features_out);
> -       /* If kernel reports required features aren't supported, skip test. */
> -       if ((features_out & req_features) != req_features) {
> -               printf("skipping test due to lack of feature support\n");
> -               fflush(stdout);
> -               return 0;
> -       }
> +       uffd_test_ctx_init(uffd_minor_feature());
>
>         uffdio_register.range.start = (unsigned long)area_dst_alias;
>         uffdio_register.range.len = nr_pages * page_size;
> @@ -1574,6 +1564,8 @@ unsigned long default_huge_page_size(void)
>
>  static void set_test_type(const char *type)
>  {
> +       uint64_t features = UFFD_API_FEATURES;
> +
>         if (!strcmp(type, "anon")) {
>                 test_type = TEST_ANON;
>                 uffd_test_ops = &anon_uffd_test_ops;
> @@ -1607,6 +1599,22 @@ static void set_test_type(const char *type)
>         if ((unsigned long) area_count(NULL, 0) + sizeof(unsigned long long) * 2
>             > page_size)
>                 err("Impossible to run this test");
> +
> +       /*
> +        * Whether we can test certain features depends not just on test type,
> +        * but also on whether or not this particular kernel supports the
> +        * feature.
> +        */
> +
> +       userfaultfd_open(&features);
> +
> +       test_uffdio_wp = test_uffdio_wp &&
> +               (features & UFFD_FEATURE_PAGEFAULT_FLAG_WP);
> +       test_uffdio_minor = test_uffdio_minor &&
> +               (features & uffd_minor_feature());
> +
> +       close(uffd);
> +       uffd = -1;
>  }
>
>  static void sigalrm(int sig)
> --
> 2.33.0.800.g4c38ced690-goog
>

Powered by blists - more mailing lists