| Message ID | 20240428190151.201002-3-peterx@redhat.com (mailing list archive) |
|---|---|
| State | Handled Elsewhere |
| Headers | show |
| Series | mm/gup: Fix hugepd for longterm R/O pin on Power | expand |
On 28.04.24 21:01, Peter Xu wrote: > Prefault, especially with RW, makes the GUP test too easy, and may not yet > reach the core of the test. > > For example, R/O longterm pins will just hit, pte_write()==true for > whatever cases, the unsharing logic won't be ever tested. > > This patch remove the prefault. This tortures more code paths at least to > cover the unshare care for R/O longterm pins, in which case the first R/O > GUP attempt will fault in the page R/O first, then the 2nd will go through > the unshare path, checking whether an unshare is needed. > > Cc: David Hildenbrand <david@redhat.com> > Signed-off-by: Peter Xu <peterx@redhat.com> > --- > tools/testing/selftests/mm/gup_longterm.c | 12 +++++++++--- > 1 file changed, 9 insertions(+), 3 deletions(-) > > diff --git a/tools/testing/selftests/mm/gup_longterm.c b/tools/testing/selftests/mm/gup_longterm.c > index ad168d35b23b..488e32186246 100644 > --- a/tools/testing/selftests/mm/gup_longterm.c > +++ b/tools/testing/selftests/mm/gup_longterm.c > @@ -119,10 +119,16 @@ static void do_test(int fd, size_t size, enum test_type type, bool shared) > } > > /* > - * Fault in the page writable such that GUP-fast can eventually pin > - * it immediately. > + * Explicitly avoid pre-faulting in the page, this can help testing > + * more code paths. > + * > + * Take example of an upcoming R/O pin test, if we RW prefault the > + * page, such pin will directly skip R/O unsharing and the longterm > + * pin will success mostly always. When not prefaulted, R/O > + * longterm pin will first fault in a RO page, then the 2nd round > + * it'll go via the unshare check. Otherwise those paths aren't > + * covered. > */ This will mean that GUP-fast never succeeds, which removes quite some testing coverage for most other tests here. Note that the main motivation of this test was to test gup_fast_folio_allowed(), where we had issues with GUP-fast during development. Would the following also get the job done? diff --git a/tools/testing/selftests/mm/gup_longterm.c b/tools/testing/selftests/mm/gup_longterm.c index ad168d35b23b7..e917a7c58d571 100644 --- a/tools/testing/selftests/mm/gup_longterm.c +++ b/tools/testing/selftests/mm/gup_longterm.c @@ -92,7 +92,7 @@ static void do_test(int fd, size_t size, enum test_type type, bool shared) { __fsword_t fs_type = get_fs_type(fd); bool should_work; - char *mem; + char tmp, *mem; int ret; if (ftruncate(fd, size)) { @@ -119,10 +119,19 @@ static void do_test(int fd, size_t size, enum test_type type, bool shared) } /* - * Fault in the page writable such that GUP-fast can eventually pin - * it immediately. + * Fault in the page such that GUP-fast might be able to pin it + * immediately. To cover more cases, don't fault in pages writable when + * R/O pinning. */ - memset(mem, 0, size); + switch (type) { + case TEST_TYPE_RO: + case TEST_TYPE_RO_FAST: + tmp = *mem; + asm volatile("" : "+r" (tmp)); + break; + default: + memset(mem, 0, size); + }; switch (type) { case TEST_TYPE_RO:
On Mon, Apr 29, 2024 at 09:28:15AM +0200, David Hildenbrand wrote: > On 28.04.24 21:01, Peter Xu wrote: > > Prefault, especially with RW, makes the GUP test too easy, and may not yet > > reach the core of the test. > > > > For example, R/O longterm pins will just hit, pte_write()==true for > > whatever cases, the unsharing logic won't be ever tested. > > > > This patch remove the prefault. This tortures more code paths at least to > > cover the unshare care for R/O longterm pins, in which case the first R/O > > GUP attempt will fault in the page R/O first, then the 2nd will go through > > the unshare path, checking whether an unshare is needed. > > > > Cc: David Hildenbrand <david@redhat.com> > > Signed-off-by: Peter Xu <peterx@redhat.com> > > --- > > tools/testing/selftests/mm/gup_longterm.c | 12 +++++++++--- > > 1 file changed, 9 insertions(+), 3 deletions(-) > > > > diff --git a/tools/testing/selftests/mm/gup_longterm.c b/tools/testing/selftests/mm/gup_longterm.c > > index ad168d35b23b..488e32186246 100644 > > --- a/tools/testing/selftests/mm/gup_longterm.c > > +++ b/tools/testing/selftests/mm/gup_longterm.c > > @@ -119,10 +119,16 @@ static void do_test(int fd, size_t size, enum test_type type, bool shared) > > } > > /* > > - * Fault in the page writable such that GUP-fast can eventually pin > > - * it immediately. > > + * Explicitly avoid pre-faulting in the page, this can help testing > > + * more code paths. > > + * > > + * Take example of an upcoming R/O pin test, if we RW prefault the > > + * page, such pin will directly skip R/O unsharing and the longterm > > + * pin will success mostly always. When not prefaulted, R/O > > + * longterm pin will first fault in a RO page, then the 2nd round > > + * it'll go via the unshare check. Otherwise those paths aren't > > + * covered. > > */ > This will mean that GUP-fast never succeeds, which removes quite some testing > coverage for most other tests here. > > Note that the main motivation of this test was to test gup_fast_folio_allowed(), > where we had issues with GUP-fast during development. Ah I didn't notice that, as I thought that whitelists memfd ones. > > Would the following also get the job done? > > diff --git a/tools/testing/selftests/mm/gup_longterm.c b/tools/testing/selftests/mm/gup_longterm.c > index ad168d35b23b7..e917a7c58d571 100644 > --- a/tools/testing/selftests/mm/gup_longterm.c > +++ b/tools/testing/selftests/mm/gup_longterm.c > @@ -92,7 +92,7 @@ static void do_test(int fd, size_t size, enum test_type type, bool shared) > { > __fsword_t fs_type = get_fs_type(fd); > bool should_work; > - char *mem; > + char tmp, *mem; > int ret; > if (ftruncate(fd, size)) { > @@ -119,10 +119,19 @@ static void do_test(int fd, size_t size, enum test_type type, bool shared) > } > /* > - * Fault in the page writable such that GUP-fast can eventually pin > - * it immediately. > + * Fault in the page such that GUP-fast might be able to pin it > + * immediately. To cover more cases, don't fault in pages writable when > + * R/O pinning. > */ > - memset(mem, 0, size); > + switch (type) { > + case TEST_TYPE_RO: > + case TEST_TYPE_RO_FAST: > + tmp = *mem; > + asm volatile("" : "+r" (tmp)); > + break; > + default: > + memset(mem, 0, size); > + }; > switch (type) { > case TEST_TYPE_RO: Yes this could work too. The test patch here doesn't need to rush. David, how about you prepare a better and verified patch and post it separately, making sure to cover all the things we used to cover plus the unshare? IIUC it used to be not touched because of pte_write() always returns true with a write prefault. Then we let patch 1 go through first, and drop this one? Thanks,
On 29.04.24 15:10, Peter Xu wrote: > On Mon, Apr 29, 2024 at 09:28:15AM +0200, David Hildenbrand wrote: >> On 28.04.24 21:01, Peter Xu wrote: >>> Prefault, especially with RW, makes the GUP test too easy, and may not yet >>> reach the core of the test. >>> >>> For example, R/O longterm pins will just hit, pte_write()==true for >>> whatever cases, the unsharing logic won't be ever tested. >>> >>> This patch remove the prefault. This tortures more code paths at least to >>> cover the unshare care for R/O longterm pins, in which case the first R/O >>> GUP attempt will fault in the page R/O first, then the 2nd will go through >>> the unshare path, checking whether an unshare is needed. >>> >>> Cc: David Hildenbrand <david@redhat.com> >>> Signed-off-by: Peter Xu <peterx@redhat.com> >>> --- >>> tools/testing/selftests/mm/gup_longterm.c | 12 +++++++++--- >>> 1 file changed, 9 insertions(+), 3 deletions(-) >>> >>> diff --git a/tools/testing/selftests/mm/gup_longterm.c b/tools/testing/selftests/mm/gup_longterm.c >>> index ad168d35b23b..488e32186246 100644 >>> --- a/tools/testing/selftests/mm/gup_longterm.c >>> +++ b/tools/testing/selftests/mm/gup_longterm.c >>> @@ -119,10 +119,16 @@ static void do_test(int fd, size_t size, enum test_type type, bool shared) >>> } >>> /* >>> - * Fault in the page writable such that GUP-fast can eventually pin >>> - * it immediately. >>> + * Explicitly avoid pre-faulting in the page, this can help testing >>> + * more code paths. >>> + * >>> + * Take example of an upcoming R/O pin test, if we RW prefault the >>> + * page, such pin will directly skip R/O unsharing and the longterm >>> + * pin will success mostly always. When not prefaulted, R/O >>> + * longterm pin will first fault in a RO page, then the 2nd round >>> + * it'll go via the unshare check. Otherwise those paths aren't >>> + * covered. >>> */ >> This will mean that GUP-fast never succeeds, which removes quite some testing >> coverage for most other tests here. >> >> Note that the main motivation of this test was to test gup_fast_folio_allowed(), >> where we had issues with GUP-fast during development. > > Ah I didn't notice that, as I thought that whitelists memfd ones. > >> >> Would the following also get the job done? >> >> diff --git a/tools/testing/selftests/mm/gup_longterm.c b/tools/testing/selftests/mm/gup_longterm.c >> index ad168d35b23b7..e917a7c58d571 100644 >> --- a/tools/testing/selftests/mm/gup_longterm.c >> +++ b/tools/testing/selftests/mm/gup_longterm.c >> @@ -92,7 +92,7 @@ static void do_test(int fd, size_t size, enum test_type type, bool shared) >> { >> __fsword_t fs_type = get_fs_type(fd); >> bool should_work; >> - char *mem; >> + char tmp, *mem; >> int ret; >> if (ftruncate(fd, size)) { >> @@ -119,10 +119,19 @@ static void do_test(int fd, size_t size, enum test_type type, bool shared) >> } >> /* >> - * Fault in the page writable such that GUP-fast can eventually pin >> - * it immediately. >> + * Fault in the page such that GUP-fast might be able to pin it >> + * immediately. To cover more cases, don't fault in pages writable when >> + * R/O pinning. >> */ >> - memset(mem, 0, size); >> + switch (type) { >> + case TEST_TYPE_RO: >> + case TEST_TYPE_RO_FAST: >> + tmp = *mem; >> + asm volatile("" : "+r" (tmp)); >> + break; >> + default: >> + memset(mem, 0, size); >> + }; >> switch (type) { >> case TEST_TYPE_RO: > > Yes this could work too. > > The test patch here doesn't need to rush. David, how about you prepare a > better and verified patch and post it separately, making sure to cover all > the things we used to cover plus the unshare? IIUC it used to be not > touched because of pte_write() always returns true with a write prefault. > > Then we let patch 1 go through first, and drop this one? Whatever you prefer!
On Mon, Apr 29, 2024 at 03:26:22PM +0200, David Hildenbrand wrote: > > The test patch here doesn't need to rush. David, how about you prepare a > > better and verified patch and post it separately, making sure to cover all > > the things we used to cover plus the unshare? IIUC it used to be not > > touched because of pte_write() always returns true with a write prefault. > > > > Then we let patch 1 go through first, and drop this one? > > Whatever you prefer! Thanks! Andrew, would you consider taking patch 1 but ignore this patch 2? Or do you prefer me to resend?
diff --git a/tools/testing/selftests/mm/gup_longterm.c b/tools/testing/selftests/mm/gup_longterm.c index ad168d35b23b..488e32186246 100644 --- a/tools/testing/selftests/mm/gup_longterm.c +++ b/tools/testing/selftests/mm/gup_longterm.c @@ -119,10 +119,16 @@ static void do_test(int fd, size_t size, enum test_type type, bool shared) } /* - * Fault in the page writable such that GUP-fast can eventually pin - * it immediately. + * Explicitly avoid pre-faulting in the page, this can help testing + * more code paths. + * + * Take example of an upcoming R/O pin test, if we RW prefault the + * page, such pin will directly skip R/O unsharing and the longterm + * pin will success mostly always. When not prefaulted, R/O + * longterm pin will first fault in a RO page, then the 2nd round + * it'll go via the unshare check. Otherwise those paths aren't + * covered. */ - memset(mem, 0, size); switch (type) { case TEST_TYPE_RO:
Prefault, especially with RW, makes the GUP test too easy, and may not yet reach the core of the test. For example, R/O longterm pins will just hit, pte_write()==true for whatever cases, the unsharing logic won't be ever tested. This patch remove the prefault. This tortures more code paths at least to cover the unshare care for R/O longterm pins, in which case the first R/O GUP attempt will fault in the page R/O first, then the 2nd will go through the unshare path, checking whether an unshare is needed. Cc: David Hildenbrand <david@redhat.com> Signed-off-by: Peter Xu <peterx@redhat.com> --- tools/testing/selftests/mm/gup_longterm.c | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-)