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
