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

[Peter Xu]

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?

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