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

Message ID 20230602013358.900637-11-jhubbard@nvidia.com
State New
Headers
Series A minor flurry of selftest/mm fixes |

Commit Message

John Hubbard June 2, 2023, 1:33 a.m. UTC
  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(-)
  

Comments

Peter Xu June 2, 2023, 3:59 p.m. UTC | #1
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,
  
John Hubbard June 2, 2023, 10:11 p.m. UTC | #2
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,
  
Peter Xu June 2, 2023, 10:38 p.m. UTC | #3
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.
  
John Hubbard June 2, 2023, 10:52 p.m. UTC | #4
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,
  
John Hubbard June 3, 2023, 12:43 a.m. UTC | #5
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,
  
Peter Xu June 3, 2023, 1:18 a.m. UTC | #6
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,
  
John Hubbard June 3, 2023, 1:39 a.m. UTC | #7
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,
  

Patch

diff --git a/tools/testing/selftests/mm/Makefile b/tools/testing/selftests/mm/Makefile
index 473bf1811552..9bf3305b7dea 100644
--- a/tools/testing/selftests/mm/Makefile
+++ b/tools/testing/selftests/mm/Makefile
@@ -109,8 +109,11 @@  include ../lib.mk
 
 $(TEST_GEN_PROGS): vm_util.c
 
-$(OUTPUT)/uffd-stress: uffd-common.c
-$(OUTPUT)/uffd-unit-tests: uffd-common.c
+$(OUTPUT)/uffd-stress:          uffd-common.c
+$(OUTPUT)/uffd-unit-tests:      uffd-common.c
+$(OUTPUT)/hugepage-mremap:      uffd-common.c
+$(OUTPUT)/write_to_hugetlbfs:   uffd-common.c
+$(OUTPUT)/ksm_functional_tests: uffd-common.c
 
 ifeq ($(MACHINE),x86_64)
 BINARIES_32 := $(patsubst %,$(OUTPUT)/%,$(BINARIES_32))
diff --git a/tools/testing/selftests/mm/hugepage-mremap.c b/tools/testing/selftests/mm/hugepage-mremap.c
index cabd0084f57b..8158fe909f5e 100644
--- a/tools/testing/selftests/mm/hugepage-mremap.c
+++ b/tools/testing/selftests/mm/hugepage-mremap.c
@@ -24,7 +24,7 @@ 
 #include <sys/ioctl.h>
 #include <string.h>
 #include <stdbool.h>
-#include "vm_util.h"
+#include "uffd-common.h"
 
 #define DEFAULT_LENGTH_MB 10UL
 #define MB_TO_BYTES(x) (x * 1024 * 1024)
diff --git a/tools/testing/selftests/mm/ksm_functional_tests.c b/tools/testing/selftests/mm/ksm_functional_tests.c
index 26853badae70..648188ad73fa 100644
--- a/tools/testing/selftests/mm/ksm_functional_tests.c
+++ b/tools/testing/selftests/mm/ksm_functional_tests.c
@@ -22,7 +22,7 @@ 
 #include <linux/userfaultfd.h>
 
 #include "../kselftest.h"
-#include "vm_util.h"
+#include "uffd-common.h"
 
 #define KiB 1024u
 #define MiB (1024 * KiB)
diff --git a/tools/testing/selftests/mm/uffd-common.c b/tools/testing/selftests/mm/uffd-common.c
index 61c6250adf93..e1ad63668a05 100644
--- a/tools/testing/selftests/mm/uffd-common.c
+++ b/tools/testing/selftests/mm/uffd-common.c
@@ -6,6 +6,7 @@ 
  */
 
 #include "uffd-common.h"
+#include "vm_util.h"
 
 #define BASE_PMD_ADDR ((void *)(1UL << 30))
 
@@ -616,3 +617,107 @@  int copy_page(int ufd, unsigned long offset, bool wp)
 {
 	return __copy_page(ufd, offset, false, wp);
 }
+
+/* If `ioctls' non-NULL, the allowed ioctls will be returned into the var */
+int uffd_register_with_ioctls(int uffd, void *addr, uint64_t len,
+			      bool miss, bool wp, bool minor, uint64_t *ioctls)
+{
+	struct uffdio_register uffdio_register = { 0 };
+	uint64_t mode = 0;
+	int ret = 0;
+
+	if (miss)
+		mode |= UFFDIO_REGISTER_MODE_MISSING;
+	if (wp)
+		mode |= UFFDIO_REGISTER_MODE_WP;
+	if (minor)
+		mode |= UFFDIO_REGISTER_MODE_MINOR;
+
+	uffdio_register.range.start = (unsigned long)addr;
+	uffdio_register.range.len = len;
+	uffdio_register.mode = mode;
+
+	if (ioctl(uffd, UFFDIO_REGISTER, &uffdio_register) == -1)
+		ret = -errno;
+	else if (ioctls)
+		*ioctls = uffdio_register.ioctls;
+
+	return ret;
+}
+
+int uffd_register(int uffd, void *addr, uint64_t len,
+		  bool miss, bool wp, bool minor)
+{
+	return uffd_register_with_ioctls(uffd, addr, len,
+					 miss, wp, minor, NULL);
+}
+
+int uffd_unregister(int uffd, void *addr, uint64_t len)
+{
+	struct uffdio_range range = { .start = (uintptr_t)addr, .len = len };
+	int ret = 0;
+
+	if (ioctl(uffd, UFFDIO_UNREGISTER, &range) == -1)
+		ret = -errno;
+
+	return ret;
+}
+
+int uffd_open_dev(unsigned int flags)
+{
+	int fd, uffd;
+
+	fd = open("/dev/userfaultfd", O_RDWR | O_CLOEXEC);
+	if (fd < 0)
+		return fd;
+	uffd = ioctl(fd, USERFAULTFD_IOC_NEW, flags);
+	close(fd);
+
+	return uffd;
+}
+
+int uffd_open_sys(unsigned int flags)
+{
+#ifdef __NR_userfaultfd
+	return syscall(__NR_userfaultfd, flags);
+#else
+	return -1;
+#endif
+}
+
+int uffd_open(unsigned int flags)
+{
+	int uffd = uffd_open_sys(flags);
+
+	if (uffd < 0)
+		uffd = uffd_open_dev(flags);
+
+	return uffd;
+}
+
+int uffd_get_features(uint64_t *features)
+{
+	struct uffdio_api uffdio_api = { .api = UFFD_API, .features = 0 };
+	/*
+	 * This should by default work in most kernels; the feature list
+	 * will be the same no matter what we pass in here.
+	 */
+	int fd = uffd_open(UFFD_USER_MODE_ONLY);
+
+	if (fd < 0)
+		/* Maybe the kernel is older than user-only mode? */
+		fd = uffd_open(0);
+
+	if (fd < 0)
+		return fd;
+
+	if (ioctl(fd, UFFDIO_API, &uffdio_api)) {
+		close(fd);
+		return -errno;
+	}
+
+	*features = uffdio_api.features;
+	close(fd);
+
+	return 0;
+}
diff --git a/tools/testing/selftests/mm/uffd-common.h b/tools/testing/selftests/mm/uffd-common.h
index 6068f2346b86..a1cdb78c0762 100644
--- a/tools/testing/selftests/mm/uffd-common.h
+++ b/tools/testing/selftests/mm/uffd-common.h
@@ -19,8 +19,6 @@ 
 #include <signal.h>
 #include <poll.h>
 #include <string.h>
-#include <linux/mman.h>
-#include <sys/mman.h>
 #include <sys/syscall.h>
 #include <sys/ioctl.h>
 #include <sys/wait.h>
@@ -110,6 +108,16 @@  int __copy_page(int ufd, unsigned long offset, bool retry, bool wp);
 int copy_page(int ufd, unsigned long offset, bool wp);
 void *uffd_poll_thread(void *arg);
 
+int uffd_register(int uffd, void *addr, uint64_t len,
+		  bool miss, bool wp, bool minor);
+int uffd_unregister(int uffd, void *addr, uint64_t len);
+int uffd_open_dev(unsigned int flags);
+int uffd_open_sys(unsigned int flags);
+int uffd_open(unsigned int flags);
+int uffd_get_features(uint64_t *features);
+int uffd_register_with_ioctls(int uffd, void *addr, uint64_t len,
+			      bool miss, bool wp, bool minor, uint64_t *ioctls);
+
 #define TEST_ANON	1
 #define TEST_HUGETLB	2
 #define TEST_SHMEM	3
diff --git a/tools/testing/selftests/mm/vm_util.c b/tools/testing/selftests/mm/vm_util.c
index 01296c17df02..c64a0134f83c 100644
--- a/tools/testing/selftests/mm/vm_util.c
+++ b/tools/testing/selftests/mm/vm_util.c
@@ -198,110 +198,6 @@  unsigned long default_huge_page_size(void)
 	return hps;
 }
 
-/* If `ioctls' non-NULL, the allowed ioctls will be returned into the var */
-int uffd_register_with_ioctls(int uffd, void *addr, uint64_t len,
-			      bool miss, bool wp, bool minor, uint64_t *ioctls)
-{
-	struct uffdio_register uffdio_register = { 0 };
-	uint64_t mode = 0;
-	int ret = 0;
-
-	if (miss)
-		mode |= UFFDIO_REGISTER_MODE_MISSING;
-	if (wp)
-		mode |= UFFDIO_REGISTER_MODE_WP;
-	if (minor)
-		mode |= UFFDIO_REGISTER_MODE_MINOR;
-
-	uffdio_register.range.start = (unsigned long)addr;
-	uffdio_register.range.len = len;
-	uffdio_register.mode = mode;
-
-	if (ioctl(uffd, UFFDIO_REGISTER, &uffdio_register) == -1)
-		ret = -errno;
-	else if (ioctls)
-		*ioctls = uffdio_register.ioctls;
-
-	return ret;
-}
-
-int uffd_register(int uffd, void *addr, uint64_t len,
-		  bool miss, bool wp, bool minor)
-{
-	return uffd_register_with_ioctls(uffd, addr, len,
-					 miss, wp, minor, NULL);
-}
-
-int uffd_unregister(int uffd, void *addr, uint64_t len)
-{
-	struct uffdio_range range = { .start = (uintptr_t)addr, .len = len };
-	int ret = 0;
-
-	if (ioctl(uffd, UFFDIO_UNREGISTER, &range) == -1)
-		ret = -errno;
-
-	return ret;
-}
-
-int uffd_open_dev(unsigned int flags)
-{
-	int fd, uffd;
-
-	fd = open("/dev/userfaultfd", O_RDWR | O_CLOEXEC);
-	if (fd < 0)
-		return fd;
-	uffd = ioctl(fd, USERFAULTFD_IOC_NEW, flags);
-	close(fd);
-
-	return uffd;
-}
-
-int uffd_open_sys(unsigned int flags)
-{
-#ifdef __NR_userfaultfd
-	return syscall(__NR_userfaultfd, flags);
-#else
-	return -1;
-#endif
-}
-
-int uffd_open(unsigned int flags)
-{
-	int uffd = uffd_open_sys(flags);
-
-	if (uffd < 0)
-		uffd = uffd_open_dev(flags);
-
-	return uffd;
-}
-
-int uffd_get_features(uint64_t *features)
-{
-	struct uffdio_api uffdio_api = { .api = UFFD_API, .features = 0 };
-	/*
-	 * This should by default work in most kernels; the feature list
-	 * will be the same no matter what we pass in here.
-	 */
-	int fd = uffd_open(UFFD_USER_MODE_ONLY);
-
-	if (fd < 0)
-		/* Maybe the kernel is older than user-only mode? */
-		fd = uffd_open(0);
-
-	if (fd < 0)
-		return fd;
-
-	if (ioctl(fd, UFFDIO_API, &uffdio_api)) {
-		close(fd);
-		return -errno;
-	}
-
-	*features = uffdio_api.features;
-	close(fd);
-
-	return 0;
-}
-
 unsigned int psize(void)
 {
 	if (!__page_size)
diff --git a/tools/testing/selftests/mm/vm_util.h b/tools/testing/selftests/mm/vm_util.h
index 232ffeb5805c..7f5aac0ac680 100644
--- a/tools/testing/selftests/mm/vm_util.h
+++ b/tools/testing/selftests/mm/vm_util.h
@@ -33,16 +33,6 @@  bool check_huge_shmem(void *addr, int nr_hpages, uint64_t hpage_size);
 int64_t allocate_transhuge(void *ptr, int pagemap_fd);
 unsigned long default_huge_page_size(void);
 
-int uffd_register(int uffd, void *addr, uint64_t len,
-		  bool miss, bool wp, bool minor);
-int uffd_unregister(int uffd, void *addr, uint64_t len);
-int uffd_open_dev(unsigned int flags);
-int uffd_open_sys(unsigned int flags);
-int uffd_open(unsigned int flags);
-int uffd_get_features(uint64_t *features);
-int uffd_register_with_ioctls(int uffd, void *addr, uint64_t len,
-			      bool miss, bool wp, bool minor, uint64_t *ioctls);
-
 /*
  * On ppc64 this will only work with radix 2M hugepage size
  */