On Mon, Apr 03, 2023 at 12:10:43PM -0400, Peter Xu wrote:
> On Mon, Apr 03, 2023 at 09:55:41AM +0200, David Hildenbrand wrote:
> > On 01.04.23 03:57, Axel Rasmussen wrote:
> > > On Fri, Mar 31, 2023 at 11:37 AM Mike Kravetz <mike.kravetz@xxxxxxxxxx> wrote:
> > > >
> > > > On 03/30/23 12:07, Peter Xu wrote:
> > > > > Make the check as simple as "test_type == TEST_HUGETLB" because that's the
> > > > > only mem that doesn't support ZEROPAGE.
> > > > >
> > > > > Signed-off-by: Peter Xu <peterx@xxxxxxxxxx>
> > > > > ---
> > > > > tools/testing/selftests/mm/userfaultfd.c | 2 +-
> > > > > 1 file changed, 1 insertion(+), 1 deletion(-)
> > > > >
> > > > > diff --git a/tools/testing/selftests/mm/userfaultfd.c b/tools/testing/selftests/mm/userfaultfd.c
> > > > > index 795fbc4d84f8..d724f1c78847 100644
> > > > > --- a/tools/testing/selftests/mm/userfaultfd.c
> > > > > +++ b/tools/testing/selftests/mm/userfaultfd.c
> > > > > @@ -1118,7 +1118,7 @@ static int __uffdio_zeropage(int ufd, unsigned long offset, bool retry)
> > > > > {
> > > > > struct uffdio_zeropage uffdio_zeropage;
> > > > > int ret;
> > > > > - bool has_zeropage = get_expected_ioctls(0) & (1 << _UFFDIO_ZEROPAGE);
> > > > > + bool has_zeropage = !(test_type == TEST_HUGETLB);
> > > >
> > > > It is true that hugetlb is the only mem type that does not support zeropage.
> > > > So, the change is correct.
> > > >
> > > > However, I actually prefer the explicit check that is there today. It seems
> > > > more like a test of the API. And, is more future proof is code changes.
> > > >
> > > > Just my opinion/thoughts, not a strong objection.
> > >
> > > I agree. The existing code is more robust to future changes where we
> > > might support or stop supporting this ioctl in some cases. It also
> > > proves that the ioctl works, any time the API reports that it is
> > > supported / ought to work, independent of when the *test* thinks it
> > > should be supported.
> > >
> > > Then again, I think this is unlikely to change in the future, so I
> > > also agree with Mike that it's not the biggest deal.
> >
> > As there were already discussions on eventually supporting UFFDIO_ZEROPAGE
> > that doesn't place the shared zeropage but ... a fresh zeropage, it might
> > make sense to keep it as is.
>
> Thanks everyone.
>
> So here the major goal is to drop get_expected_ioctls(), and I think it's
> really unwanted here. Besides it's a blocker for split the test in a clean
> way, a major reason is get_expected_ioctls() fetches "wheter we support
> zeropage for this mem" from UFFD_API_RANGE_IOCTLS, rather than from the
> UFFDIO_REGISTER anyway:
>
> uint64_t get_expected_ioctls(uint64_t mode)
> {
> uint64_t ioctls = UFFD_API_RANGE_IOCTLS;
>
> if (test_type == TEST_HUGETLB)
> ioctls &= ~(1 << _UFFDIO_ZEROPAGE);
>
> if (!((mode & UFFDIO_REGISTER_MODE_WP) && test_uffdio_wp))
> ioctls &= ~(1 << _UFFDIO_WRITEPROTECT);
>
> if (!((mode & UFFDIO_REGISTER_MODE_MINOR) && test_uffdio_minor))
> ioctls &= ~(1 << _UFFDIO_CONTINUE);
>
> return ioctls;
> }
>
> It means it'll succeed or fail depending on what kernel we run this test
> on, and also on what headers we compile the test against.
>
> I actually mentioned some of the reasoning in a follow up patch (sorry
> maybe the split here caused some confusion):
>
> selftests/mm: uffd_[un]register()
>
> Add two helpers to register/unregister to an uffd. Use them to drop
> duplicate codes.
>
> This patch also drops assert_expected_ioctls_present() and
> get_expected_ioctls(). Reasons:
>
> - It'll need a lot of effort to pass test_type==HUGETLB into it from the
> upper, so it's the simplest way to get rid of another global var
>
> - The ioctls returned in UFFDIO_REGISTER is hardly useful at all, because
> any app can already detect kernel support on any ioctl via its
> corresponding UFFD_FEATURE_*. The check here is for sanity mostly but
> it's probably destined no user app will even use it.
>
> - It's not friendly to one future goal of uffd to run on old kernels, the
> problem is get_expected_ioctls() compiles against UFFD_API_RANGE_IOCTLS,
> which is a value that can change depending on where the test is compiled,
> rather than reflecting what the kernel underneath has. It means it'll
> report false negatives on old kernels so it's against our will.
>
> So let's make our live easier.
>
> But I do agree that it's helpful to keep a test against
> uffdio_register.ioctls in this case against _UFFDIO_ZEROPAGE, so it can be
> detected dynamically. IOW, even if we would like to avoid "test !=
> HUGETLB" here, at least we should like to fix that with the UFFDIO_REGISTER
> results.
>
> Here's my offer below. :)
>
> Could I keep this patch as-is (as part of getting rid of
> get_expected_ioctls() effort; I can squash this one into "selftests/mm:
> uffd_[un]register()" if any of you think proper), meanwhile I'll squash a
> fixup to the "move zeropage test into uffd-unit-tests" explicitly check
> uffdio_register.ioctls in the same patchset? IOW, we'll have a few test
> commits missing this specific ioctl test, but then we'll have a better one
> dynamically detected from the kernel.
>
> The fixup patch attached. I think it'll automatically work when someone
> would like to introduce UFFDIO_ZEROPAGE to hugetlb too, another side
> benefit is I merged the zeropage test into one, which does look better too.
I agree that it makes sense. A nit below :)
> Thanks,
>
> --
> Peter Xu
> From 5b06f921cf8420600c697a3072a1459a5cb4956b Mon Sep 17 00:00:00 2001
> From: Peter Xu <peterx@xxxxxxxxxx>
> Date: Mon, 3 Apr 2023 11:57:07 -0400
> Subject: [PATCH] fixup! selftests/mm: Move zeropage test into uffd unit tests
>
> Signed-off-by: Peter Xu <peterx@xxxxxxxxxx>
> ---
> tools/testing/selftests/mm/uffd-unit-tests.c | 62 +++++++++++---------
> 1 file changed, 33 insertions(+), 29 deletions(-)
>
> diff --git a/tools/testing/selftests/mm/uffd-unit-tests.c b/tools/testing/selftests/mm/uffd-unit-tests.c
> index 793931da5056..247700bb4dd0 100644
> --- a/tools/testing/selftests/mm/uffd-unit-tests.c
> +++ b/tools/testing/selftests/mm/uffd-unit-tests.c
> @@ -711,54 +711,58 @@ static bool do_uffdio_zeropage(int ufd, bool has_zeropage)
> return false;
> }
>
> +/*
> + * Registers a range with MISSING mode only for zeropage test. Return true
> + * if UFFDIO_ZEROPAGE supported, false otherwise. Can't use uffd_register()
> + * because we want to detect .ioctls along the way.
> + */
> +static bool
> +uffd_register_detect_zp(int uffd, void *addr, uint64_t len)
Let's spell out 'zp' as zeropage, what do you say?
> +{
> + struct uffdio_register uffdio_register = { 0 };
> + uint64_t mode = UFFDIO_REGISTER_MODE_MISSING;
> +
> + uffdio_register.range.start = (unsigned long)addr;
> + uffdio_register.range.len = len;
> + uffdio_register.mode = mode;
> +
> + if (ioctl(uffd, UFFDIO_REGISTER, &uffdio_register) == -1)
> + err("zeropage test register fail");
> +
> + return uffdio_register.ioctls & (1 << _UFFDIO_ZEROPAGE);
> +}
> +
> +
> /* exercise UFFDIO_ZEROPAGE */
> -static void uffd_zeropage_test_common(bool has_zeropage)
> +static void uffd_zeropage_test(void)
> {
> - if (uffd_register(uffd, area_dst, page_size,
> - true, false, false))
> - err("register");
> + bool has_zeropage;
> + int i;
>
> + has_zeropage = uffd_register_detect_zp(uffd, area_dst, page_size);
> if (area_dst_alias)
> - if (uffd_register(uffd, area_dst_alias, page_size,
> - true, false, false))
> - err("register");
> -
> - if (do_uffdio_zeropage(uffd, has_zeropage)) {
> - int i;
> + /* Ignore the retval; we already have it */
> + uffd_register_detect_zp(uffd, area_dst_alias, page_size);
>
> + if (do_uffdio_zeropage(uffd, has_zeropage))
> for (i = 0; i < page_size; i++)
> if (area_dst[i] != 0)
> err("data non-zero at offset %d\n", i);
> - }
>
> + if (uffd_unregister(uffd, area_dst, page_size))
> + err("unregister");
>
> - if (uffd_unregister(uffd, area_dst, page_size * nr_pages))
> + if (area_dst_alias && uffd_unregister(uffd, area_dst_alias, page_size))
> err("unregister");
>
> uffd_test_pass();
> }
>
> -static void uffd_zeropage_test(void)
> -{
> - uffd_zeropage_test_common(true);
> -}
> -
> -static void uffd_zeropage_hugetlb_test(void)
> -{
> - uffd_zeropage_test_common(false);
> -}
> -
> uffd_test_case_t uffd_tests[] = {
> {
> .name = "zeropage",
> .uffd_fn = uffd_zeropage_test,
> - .mem_targets = MEM_ANON | MEM_SHMEM | MEM_SHMEM_PRIVATE,
> - .uffd_feature_required = 0,
> - },
> - {
> - .name = "zeropage-hugetlb",
> - .uffd_fn = uffd_zeropage_hugetlb_test,
> - .mem_targets = MEM_HUGETLB | MEM_HUGETLB_PRIVATE,
> + .mem_targets = MEM_ALL,
> .uffd_feature_required = 0,
> },
> {
> --
> 2.39.1
>
--
Sincerely yours,
Mike.
