Re: [PATCH 1/3] userfaultfd/selftests: fix feature support detection

[Axel Rasmussen]

On Tue, Sep 21, 2021 at 12:21 PM Peter Xu <peterx@xxxxxxxxxx> wrote:
>
> On Tue, Sep 21, 2021 at 11:26:14AM -0700, Axel Rasmussen wrote:
> > On Tue, Sep 21, 2021 at 10:44 AM Peter Xu <peterx@xxxxxxxxxx> wrote:
> > >
> > > Hi, Axel,
> > >
> > > On Tue, Sep 21, 2021 at 09:33:21AM -0700, Axel Rasmussen wrote:
> > > > diff --git a/tools/testing/selftests/vm/userfaultfd.c b/tools/testing/selftests/vm/userfaultfd.c
> > > > index 10ab56c2484a..2366caf90435 100644
> > > > --- a/tools/testing/selftests/vm/userfaultfd.c
> > > > +++ b/tools/testing/selftests/vm/userfaultfd.c
> > > > @@ -79,10 +79,6 @@ static int test_type;
> > > >  #define ALARM_INTERVAL_SECS 10
> > > >  static volatile bool test_uffdio_copy_eexist = true;
> > > >  static volatile bool test_uffdio_zeropage_eexist = true;
> > > > -/* Whether to test uffd write-protection */
> > > > -static bool test_uffdio_wp = false;
> > > > -/* Whether to test uffd minor faults */
> > > > -static bool test_uffdio_minor = false;
> > >
> > > IMHO it's not a fault to have these variables; they're still the fastest way to
> > > do branching.  It's just that in some cases we should set them to "false"
> > > rather than "true", am I right?
> > >
> > > How about we just set them properly in set_test_type?  Say, we can fetch the
> > > feature bits in set_test_type rather than assuming it's only related to the
> > > type of memory.
> >
> > We could do that, but it would require opening a userfaultfd, issuing
> > a UFFDIO_API ioctl, and getting the feature bits in set_test_type. And
> > then I guess just closing the UFFD again, as we aren't yet setting up
> > for any particular test. To me, it seemed "messier" than this
> > approach.
> >
> > Another thing to consider is, for the next patch we don't just want to
> > know "does this kernel support $FEATURE in general?" but also "is
> > $FEATURE supported for this particular memory region I've
> > registered?", and we can't have a single global answer to that.
>
> Could I ask why?  For each run, the memory type doesn't change, isn't it?  Then
> I think the capability it should support is a constant?

Ah, it has to do with us asserting the list of expected ioctls. The
kernel changes the list of ioctls it reports in response to a
UFFDIO_REGISTER, depending on the particular kind of vma being
registered, **as well as what mode(s) it is being registered with**.

So for example, consider the hugetlb_shared test. When registering,
the kernel might set the UFFDIO_CONTINUE bit or not, depending on
whether we registered with the MINOR mode bit set in particular. So it
will be present in one test case, but not in another, and so the set
of expected ioctls has to be computed at test time, rather than in
set_test_type.

>
> Btw, note that "open an uffd, detect features, close uffd quickly" during setup
> phase is totally fine to me just for probing the capabilities, and instead of
> thinking it being messy I see it a very clean approach..
>
> > It seemed a bit cleaner to me to write the code as if I was dealing with that
> > case, and then re-use the infrastructure I'd built for patch 2/3.
>
> I didn't comment on patch 2, but I had the same confusion - aren't all these
> information constant after we settle the hardware, the kernel and the memory
> type to test?
>
> >
> > Basically, I didn't initially have a goal of getting rid of these
> > variables, but it ended up being the cleanest way (IMHO).
> >
> > Just trying to explain the thinking. :) In the end, I think it's a
> > stylistic choice and don't feel super strongly about it, either way
> > could work. So, I can change it if you or others do feel strongly.
>
> I have no strong opinion as long as the code works (which I trust you on :).
> We can keep it in Andrew's queue unless you do feel the other way is better.
>
> Thanks,
>
> --
> Peter Xu
>