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

  This is where they belong, and this makes it cleaner to apply a
follow-up fix to the uffd builds.

Signed-off-by: John Hubbard <jhubbard@nvidia.com>
---
 tools/testing/selftests/mm/Makefile           |   7 +-
 tools/testing/selftests/mm/hugepage-mremap.c  |   2 +-
 .../selftests/mm/ksm_functional_tests.c       |   2 +-
 tools/testing/selftests/mm/uffd-common.c      | 105 ++++++++++++++++++
 tools/testing/selftests/mm/uffd-common.h      |  12 +-
 tools/testing/selftests/mm/vm_util.c          | 104 -----------------
 tools/testing/selftests/mm/vm_util.h          |  10 --
 7 files changed, 122 insertions(+), 120 deletions(-)
  

On Thu, Jun 01, 2023 at 06:33:56PM -0700, John Hubbard wrote:
> This is where they belong, and this makes it cleaner to apply a
> follow-up fix to the uffd builds.
> 
> Signed-off-by: John Hubbard <jhubbard@nvidia.com>

Thanks for further looking into this.

I'm fine to move it over if you think proper, but just to mention I had
those in vm_utils.h just because I left all uffd specific tests shared code
in uffd-common.h, so my plan was uffd-common.h shouldn't be included in
most test cases except uffd tests.

I'm not sure whether we can just make your next patch of "ifndef.." into
vm_utils.h to avoid the movement, or is it a must?

Thanks,
  

On 6/2/23 08:59, Peter Xu wrote:
> On Thu, Jun 01, 2023 at 06:33:56PM -0700, John Hubbard wrote:
>> This is where they belong, and this makes it cleaner to apply a
>> follow-up fix to the uffd builds.
>>
>> Signed-off-by: John Hubbard <jhubbard@nvidia.com>
> 
> Thanks for further looking into this.
> 
> I'm fine to move it over if you think proper, but just to mention I had
> those in vm_utils.h just because I left all uffd specific tests shared code
> in uffd-common.h, so my plan was uffd-common.h shouldn't be included in
> most test cases except uffd tests.

I think we're in agreement that we want to only include uffd-common.h
where it's actually required. Likewise with the uffd*() routines. So I
would like to still move this over, yes, just to have things in their
best-named location.

> 
> I'm not sure whether we can just make your next patch of "ifndef.." into
> vm_utils.h to avoid the movement, or is it a must?
> 

Actually, I think I can drop the next patch entirely, based on
Muhammad's observation that we should be doing a "make headers"
to pull in those items. I'll have more to say over on that thread.


thanks,
  

On Fri, Jun 02, 2023 at 03:11:52PM -0700, John Hubbard wrote:
> On 6/2/23 08:59, Peter Xu wrote:
> > On Thu, Jun 01, 2023 at 06:33:56PM -0700, John Hubbard wrote:
> > > This is where they belong, and this makes it cleaner to apply a
> > > follow-up fix to the uffd builds.
> > > 
> > > Signed-off-by: John Hubbard <jhubbard@nvidia.com>
> > 
> > Thanks for further looking into this.
> > 
> > I'm fine to move it over if you think proper, but just to mention I had
> > those in vm_utils.h just because I left all uffd specific tests shared code
> > in uffd-common.h, so my plan was uffd-common.h shouldn't be included in
> > most test cases except uffd tests.
> 
> I think we're in agreement that we want to only include uffd-common.h
> where it's actually required. Likewise with the uffd*() routines. So I
> would like to still move this over, yes, just to have things in their
> best-named location.

Sorry I didn't get it - e.g. I'm confused why we need to export
uffd_test_ops into ksm unit test, it doesn't make much sense to me..

If you think vm_util.h is a name too common to contain uffd helpers, shall
we create another vm_util_uffd.h just to put the uffd helpers?

Just see what's there in uffd-common.h, which is still ugly (I could look
into it some other day):

extern unsigned long nr_cpus, nr_pages, nr_pages_per_cpu, page_size;
extern char *area_src, *area_src_alias, *area_dst, *area_dst_alias, *area_remap;
extern int uffd, uffd_flags, finished, *pipefd, test_type;
extern bool map_shared;
extern bool test_uffdio_wp;
extern unsigned long long *count_verify;
extern volatile bool test_uffdio_copy_eexist;

extern uffd_test_ops_t anon_uffd_test_ops;
extern uffd_test_ops_t shmem_uffd_test_ops;
extern uffd_test_ops_t hugetlb_uffd_test_ops;
extern uffd_test_ops_t *uffd_test_ops;

and more.

That's why I think this header should not better be included by anyone else
besides uffd-stress.c and uffd-unit-tests.c for now.

> 
> > 
> > I'm not sure whether we can just make your next patch of "ifndef.." into
> > vm_utils.h to avoid the movement, or is it a must?
> > 
> 
> Actually, I think I can drop the next patch entirely, based on
> Muhammad's observation that we should be doing a "make headers"
> to pull in those items. I'll have more to say over on that thread.

Sure, great if the local headers will work.  Thanks.
  

On 6/2/23 15:38, Peter Xu wrote:
...
>>> I'm fine to move it over if you think proper, but just to mention I had
>>> those in vm_utils.h just because I left all uffd specific tests shared code
>>> in uffd-common.h, so my plan was uffd-common.h shouldn't be included in
>>> most test cases except uffd tests.
>>
>> I think we're in agreement that we want to only include uffd-common.h
>> where it's actually required. Likewise with the uffd*() routines. So I
>> would like to still move this over, yes, just to have things in their
>> best-named location.
> 
> Sorry I didn't get it - e.g. I'm confused why we need to export
> uffd_test_ops into ksm unit test, it doesn't make much sense to me..

Oh, I see what you mean, finally. Yes. ksm should not need that.

> 
> If you think vm_util.h is a name too common to contain uffd helpers, shall

Right, given the presence of uffd-common.[chg], I really want to avoid putting
the uffd helpers somewhere else...

> we create another vm_util_uffd.h just to put the uffd helpers?
> 
> Just see what's there in uffd-common.h, which is still ugly (I could look
> into it some other day):

Good point.

> 
> extern unsigned long nr_cpus, nr_pages, nr_pages_per_cpu, page_size;
> extern char *area_src, *area_src_alias, *area_dst, *area_dst_alias, *area_remap;
> extern int uffd, uffd_flags, finished, *pipefd, test_type;
> extern bool map_shared;
> extern bool test_uffdio_wp;
> extern unsigned long long *count_verify;
> extern volatile bool test_uffdio_copy_eexist;
> 
> extern uffd_test_ops_t anon_uffd_test_ops;
> extern uffd_test_ops_t shmem_uffd_test_ops;
> extern uffd_test_ops_t hugetlb_uffd_test_ops;
> extern uffd_test_ops_t *uffd_test_ops;
> 
> and more.
> 
> That's why I think this header should not better be included by anyone else
> besides uffd-stress.c and uffd-unit-tests.c for now.
> 

OK, I think I can arrange things to meet that requirement. Let me
take another shot at it.


thanks,
  

On 6/2/23 15:52, John Hubbard wrote:
> On 6/2/23 15:38, Peter Xu wrote:
> ...
>>> I think we're in agreement that we want to only include uffd-common.h
>>> where it's actually required. Likewise with the uffd*() routines. So I
>>> would like to still move this over, yes, just to have things in their
>>> best-named location.
>>
>> Sorry I didn't get it - e.g. I'm confused why we need to export
>> uffd_test_ops into ksm unit test, it doesn't make much sense to me..
> 
> Oh, I see what you mean, finally. Yes. ksm should not need that.
> 

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


thanks,
  

On Fri, Jun 02, 2023 at 05:43:19PM -0700, John Hubbard wrote:
> On 6/2/23 15:52, John Hubbard wrote:
> > On 6/2/23 15:38, Peter Xu wrote:
> > ...
> > > > I think we're in agreement that we want to only include uffd-common.h
> > > > where it's actually required. Likewise with the uffd*() routines. So I
> > > > would like to still move this over, yes, just to have things in their
> > > > best-named location.
> > > 
> > > Sorry I didn't get it - e.g. I'm confused why we need to export
> > > uffd_test_ops into ksm unit test, it doesn't make much sense to me..
> > 
> > Oh, I see what you mean, finally. Yes. ksm should not need that.
> > 
> 
> ...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).

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.

Thanks,
  

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,