diff mbox series

[v7,14/14] selftests/vm: test faulting in kernel, and verify pinnable pages

Message ID 20210122033748.924330-15-pasha.tatashin@soleen.com (mailing list archive)
State New
Headers show
Series prohibit pinning pages in ZONE_MOVABLE | expand

Commit Message

Pavel Tatashin Jan. 22, 2021, 3:37 a.m. UTC
When pages are pinned they can be faulted in userland and migrated, and
they can be faulted right in kernel without migration.

In either case, the pinned pages must end-up being pinnable (not movable).

Add a new test to gup_test, to help verify that the gup/pup
(get_user_pages() / pin_user_pages()) behavior with respect to pinnable
and movable pages is reasonable and correct. Specifically, provide a
way to:

1) Verify that only "pinnable" pages are pinned. This is checked
automatically for you.

2) Verify that gup/pup performance is reasonable. This requires
comparing benchmarks between doing gup/pup on pages that have been
pre-faulted in from user space, vs. doing gup/pup on pages that are not
faulted in until gup/pup time (via FOLL_TOUCH). This decision is
controlled with the new -z command line option.

Signed-off-by: Pavel Tatashin <pasha.tatashin@soleen.com>
---
 mm/gup_test.c                         |  6 ++++++
 tools/testing/selftests/vm/gup_test.c | 23 +++++++++++++++++++----
 2 files changed, 25 insertions(+), 4 deletions(-)

Comments

John Hubbard Jan. 24, 2021, 11:18 p.m. UTC | #1
On 1/21/21 7:37 PM, Pavel Tatashin wrote:
> When pages are pinned they can be faulted in userland and migrated, and
> they can be faulted right in kernel without migration.
> 
> In either case, the pinned pages must end-up being pinnable (not movable).
> 
> Add a new test to gup_test, to help verify that the gup/pup
> (get_user_pages() / pin_user_pages()) behavior with respect to pinnable
> and movable pages is reasonable and correct. Specifically, provide a
> way to:
> 
> 1) Verify that only "pinnable" pages are pinned. This is checked
> automatically for you.
> 
> 2) Verify that gup/pup performance is reasonable. This requires
> comparing benchmarks between doing gup/pup on pages that have been
> pre-faulted in from user space, vs. doing gup/pup on pages that are not
> faulted in until gup/pup time (via FOLL_TOUCH). This decision is
> controlled with the new -z command line option.
> 
> Signed-off-by: Pavel Tatashin <pasha.tatashin@soleen.com>
> ---
>   mm/gup_test.c                         |  6 ++++++
>   tools/testing/selftests/vm/gup_test.c | 23 +++++++++++++++++++----
>   2 files changed, 25 insertions(+), 4 deletions(-)
> 

This also looks good. I do see the WARN_ON_ONCE firing in
internal_get_user_pages_fast(), when running with *only* the new -z
option.

I'll poke around the rest of the patchset and see if that is expected
and normal, but either way the test code itself looks correct and seems
to be passing my set of "run a bunch of different gup_test options" here,
so feel free to add:

Reviewed-by: John Hubbard <jhubbard@nvidia.com>

thanks,
John Hubbard Jan. 24, 2021, 11:40 p.m. UTC | #2
On 1/24/21 3:18 PM, John Hubbard wrote:
> On 1/21/21 7:37 PM, Pavel Tatashin wrote:
>> When pages are pinned they can be faulted in userland and migrated, and
>> they can be faulted right in kernel without migration.
>>
>> In either case, the pinned pages must end-up being pinnable (not movable).
>>
>> Add a new test to gup_test, to help verify that the gup/pup
>> (get_user_pages() / pin_user_pages()) behavior with respect to pinnable
>> and movable pages is reasonable and correct. Specifically, provide a
>> way to:
>>
>> 1) Verify that only "pinnable" pages are pinned. This is checked
>> automatically for you.
>>
>> 2) Verify that gup/pup performance is reasonable. This requires
>> comparing benchmarks between doing gup/pup on pages that have been
>> pre-faulted in from user space, vs. doing gup/pup on pages that are not
>> faulted in until gup/pup time (via FOLL_TOUCH). This decision is
>> controlled with the new -z command line option.
>>
>> Signed-off-by: Pavel Tatashin <pasha.tatashin@soleen.com>
>> ---
>>   mm/gup_test.c                         |  6 ++++++
>>   tools/testing/selftests/vm/gup_test.c | 23 +++++++++++++++++++----
>>   2 files changed, 25 insertions(+), 4 deletions(-)
>>
> 
> This also looks good. I do see the WARN_ON_ONCE firing in
> internal_get_user_pages_fast(), when running with *only* the new -z
> option.
> 
> I'll poke around the rest of the patchset and see if that is expected
> and normal, but either way the test code itself looks correct and seems

The warning that is firing in internal_get_user_pages_fast() is:

	if (WARN_ON_ONCE(gup_flags & ~(FOLL_WRITE | FOLL_LONGTERM |
				       FOLL_FORCE | FOLL_PIN | FOLL_GET |
				       FOLL_FAST_ONLY)))
		return -EINVAL;

...OK, so this is because "./gup_test -z" invokes get_user_pages_fast(),
which so far does not allow passing in FOLL_TOUCH. Probably because there
is nothing "fast" about touching and possibly faulting in pages. :)

So, again, the test code still looks correct, even though it's possible
to pass in options that run into things that are rejected by gup.c



thanks,
Pavel Tatashin Jan. 25, 2021, 2:17 p.m. UTC | #3
On Sun, Jan 24, 2021 at 6:18 PM John Hubbard <jhubbard@nvidia.com> wrote:
>
> On 1/21/21 7:37 PM, Pavel Tatashin wrote:
> > When pages are pinned they can be faulted in userland and migrated, and
> > they can be faulted right in kernel without migration.
> >
> > In either case, the pinned pages must end-up being pinnable (not movable).
> >
> > Add a new test to gup_test, to help verify that the gup/pup
> > (get_user_pages() / pin_user_pages()) behavior with respect to pinnable
> > and movable pages is reasonable and correct. Specifically, provide a
> > way to:
> >
> > 1) Verify that only "pinnable" pages are pinned. This is checked
> > automatically for you.
> >
> > 2) Verify that gup/pup performance is reasonable. This requires
> > comparing benchmarks between doing gup/pup on pages that have been
> > pre-faulted in from user space, vs. doing gup/pup on pages that are not
> > faulted in until gup/pup time (via FOLL_TOUCH). This decision is
> > controlled with the new -z command line option.
> >
> > Signed-off-by: Pavel Tatashin <pasha.tatashin@soleen.com>
> > ---
> >   mm/gup_test.c                         |  6 ++++++
> >   tools/testing/selftests/vm/gup_test.c | 23 +++++++++++++++++++----
> >   2 files changed, 25 insertions(+), 4 deletions(-)
> >
>
> This also looks good. I do see the WARN_ON_ONCE firing in
> internal_get_user_pages_fast(), when running with *only* the new -z
> option.
>
> I'll poke around the rest of the patchset and see if that is expected
> and normal, but either way the test code itself looks correct and seems
> to be passing my set of "run a bunch of different gup_test options" here,
> so feel free to add:
>
> Reviewed-by: John Hubbard <jhubbard@nvidia.com>

Thank you!

Pasha

>
> thanks,
> --
> John Hubbard
> NVIDIA
>
> > diff --git a/mm/gup_test.c b/mm/gup_test.c
> > index a6ed1c877679..d974dec19e1c 100644
> > --- a/mm/gup_test.c
> > +++ b/mm/gup_test.c
> > @@ -52,6 +52,12 @@ static void verify_dma_pinned(unsigned int cmd, struct page **pages,
> >
> >                               dump_page(page, "gup_test failure");
> >                               break;
> > +                     } else if (cmd == PIN_LONGTERM_BENCHMARK &&
> > +                             WARN(!is_pinnable_page(page),
> > +                                  "pages[%lu] is NOT pinnable but pinned\n",
> > +                                  i)) {
> > +                             dump_page(page, "gup_test failure");
> > +                             break;
> >                       }
> >               }
> >               break;
> > diff --git a/tools/testing/selftests/vm/gup_test.c b/tools/testing/selftests/vm/gup_test.c
> > index 943cc2608dc2..1e662d59c502 100644
> > --- a/tools/testing/selftests/vm/gup_test.c
> > +++ b/tools/testing/selftests/vm/gup_test.c
> > @@ -13,6 +13,7 @@
> >
> >   /* Just the flags we need, copied from mm.h: */
> >   #define FOLL_WRITE  0x01    /* check pte is writable */
> > +#define FOLL_TOUCH   0x02    /* mark page accessed */
> >
> >   static char *cmd_to_str(unsigned long cmd)
> >   {
> > @@ -39,11 +40,11 @@ int main(int argc, char **argv)
> >       unsigned long size = 128 * MB;
> >       int i, fd, filed, opt, nr_pages = 1, thp = -1, repeats = 1, write = 1;
> >       unsigned long cmd = GUP_FAST_BENCHMARK;
> > -     int flags = MAP_PRIVATE;
> > +     int flags = MAP_PRIVATE, touch = 0;
> >       char *file = "/dev/zero";
> >       char *p;
> >
> > -     while ((opt = getopt(argc, argv, "m:r:n:F:f:abctTLUuwWSHp")) != -1) {
> > +     while ((opt = getopt(argc, argv, "m:r:n:F:f:abctTLUuwWSHpz")) != -1) {
> >               switch (opt) {
> >               case 'a':
> >                       cmd = PIN_FAST_BENCHMARK;
> > @@ -110,6 +111,10 @@ int main(int argc, char **argv)
> >               case 'H':
> >                       flags |= (MAP_HUGETLB | MAP_ANONYMOUS);
> >                       break;
> > +             case 'z':
> > +                     /* fault pages in gup, do not fault in userland */
> > +                     touch = 1;
> > +                     break;
> >               default:
> >                       return -1;
> >               }
> > @@ -167,8 +172,18 @@ int main(int argc, char **argv)
> >       else if (thp == 0)
> >               madvise(p, size, MADV_NOHUGEPAGE);
> >
> > -     for (; (unsigned long)p < gup.addr + size; p += PAGE_SIZE)
> > -             p[0] = 0;
> > +     /*
> > +      * FOLL_TOUCH, in gup_test, is used as an either/or case: either
> > +      * fault pages in from the kernel via FOLL_TOUCH, or fault them
> > +      * in here, from user space. This allows comparison of performance
> > +      * between those two cases.
> > +      */
> > +     if (touch) {
> > +             gup.gup_flags |= FOLL_TOUCH;
> > +     } else {
> > +             for (; (unsigned long)p < gup.addr + size; p += PAGE_SIZE)
> > +                     p[0] = 0;
> > +     }
> >
> >       /* Only report timing information on the *_BENCHMARK commands: */
> >       if ((cmd == PIN_FAST_BENCHMARK) || (cmd == GUP_FAST_BENCHMARK) ||
> >
>
diff mbox series

Patch

diff --git a/mm/gup_test.c b/mm/gup_test.c
index a6ed1c877679..d974dec19e1c 100644
--- a/mm/gup_test.c
+++ b/mm/gup_test.c
@@ -52,6 +52,12 @@  static void verify_dma_pinned(unsigned int cmd, struct page **pages,
 
 				dump_page(page, "gup_test failure");
 				break;
+			} else if (cmd == PIN_LONGTERM_BENCHMARK &&
+				WARN(!is_pinnable_page(page),
+				     "pages[%lu] is NOT pinnable but pinned\n",
+				     i)) {
+				dump_page(page, "gup_test failure");
+				break;
 			}
 		}
 		break;
diff --git a/tools/testing/selftests/vm/gup_test.c b/tools/testing/selftests/vm/gup_test.c
index 943cc2608dc2..1e662d59c502 100644
--- a/tools/testing/selftests/vm/gup_test.c
+++ b/tools/testing/selftests/vm/gup_test.c
@@ -13,6 +13,7 @@ 
 
 /* Just the flags we need, copied from mm.h: */
 #define FOLL_WRITE	0x01	/* check pte is writable */
+#define FOLL_TOUCH	0x02	/* mark page accessed */
 
 static char *cmd_to_str(unsigned long cmd)
 {
@@ -39,11 +40,11 @@  int main(int argc, char **argv)
 	unsigned long size = 128 * MB;
 	int i, fd, filed, opt, nr_pages = 1, thp = -1, repeats = 1, write = 1;
 	unsigned long cmd = GUP_FAST_BENCHMARK;
-	int flags = MAP_PRIVATE;
+	int flags = MAP_PRIVATE, touch = 0;
 	char *file = "/dev/zero";
 	char *p;
 
-	while ((opt = getopt(argc, argv, "m:r:n:F:f:abctTLUuwWSHp")) != -1) {
+	while ((opt = getopt(argc, argv, "m:r:n:F:f:abctTLUuwWSHpz")) != -1) {
 		switch (opt) {
 		case 'a':
 			cmd = PIN_FAST_BENCHMARK;
@@ -110,6 +111,10 @@  int main(int argc, char **argv)
 		case 'H':
 			flags |= (MAP_HUGETLB | MAP_ANONYMOUS);
 			break;
+		case 'z':
+			/* fault pages in gup, do not fault in userland */
+			touch = 1;
+			break;
 		default:
 			return -1;
 		}
@@ -167,8 +172,18 @@  int main(int argc, char **argv)
 	else if (thp == 0)
 		madvise(p, size, MADV_NOHUGEPAGE);
 
-	for (; (unsigned long)p < gup.addr + size; p += PAGE_SIZE)
-		p[0] = 0;
+	/*
+	 * FOLL_TOUCH, in gup_test, is used as an either/or case: either
+	 * fault pages in from the kernel via FOLL_TOUCH, or fault them
+	 * in here, from user space. This allows comparison of performance
+	 * between those two cases.
+	 */
+	if (touch) {
+		gup.gup_flags |= FOLL_TOUCH;
+	} else {
+		for (; (unsigned long)p < gup.addr + size; p += PAGE_SIZE)
+			p[0] = 0;
+	}
 
 	/* Only report timing information on the *_BENCHMARK commands: */
 	if ((cmd == PIN_FAST_BENCHMARK) || (cmd == GUP_FAST_BENCHMARK) ||