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

[Axel Rasmussen]

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. 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.

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.

>
> Thanks,
>
> --
> Peter Xu
>