Re: [PATCH 10/12] selftests/mm: move uffd* routines from vm_util.c to uffd-common.c

[John Hubbard]

On 6/2/23 18:18, Peter Xu wrote:
...
> > ...whoops, correction, our very own David Hildenbrand recently made
> > changes that contradict the claim that "ksm and uffd selftests are
> > independent". In fact, ksm now *intentionally* depends upon uffd, as of
> > commit 93fb70aa5904c ("selftests/vm: add KSM unmerge tests"), aha!
> >
> > That added commit added a call to test_unmerge_uffd_wp(), to
> > ksm_functional_tests.c .
> >
> > So this needs to stay approximately as-is, it seems.
>
> So I think it depends on what is "as-is" to me in the above sentence. :)
>
> test_unmerge_uffd_wp() impled its own uffd ioctls, and it still doesn't use
> any of uffd-common.h of now (e.g. uffd_test_ops).
>
> IMHO if we want we can let test_unmerge_uffd_wp() reuse either
> uffd_get_features(), uffd_open(), uffd_register() etc., but still all of
> them are provided by vm_util.h not uffd-common.h for now, and that's
> intended (vm_util.h can contain uffd helpers, or whatever helpers as long
> as generic mm/ unit tests need).

ksm_functional_tests.c calls uffd_register(). That's about as clear
as it gets: this file distinctly depends upon uffd test functionality.

The goal here is to put uffd*() routines into uffd-common.[ch], and
everything else into vm_utils.[ch]. Because that's what you do, when you
have such named files.

Putting uffd*() routines somewhere other than uffd-common.* requires
some...reason. And all I've heard so far is, "it was already
scrambled--as intended, don't mess with it!" :)

> We can even move wp_range() from uffd-common.[ch] into vm_utils.[ch], then
> it can also share that (need to replace err(), that's uffd-common
> specific).  Not necessary anything must be done in this series, though.

But wp_range(), despite its generic-sounding name, is another example of
something that remains tightly coupled to the uffd code: it uses
UFFDIO_WRITEPROTECT to get its work done.

So I'd recommend leaving this one in uffd-common.c.


thanks,
--
John Hubbard
NVIDIA