Re: [PATCH v7 13/14] selftests/vm: test flag is broken
On Sun, Jan 24, 2021 at 6:03 PM John Hubbard wrote:
>
> On 1/21/21 7:37 PM, Pavel Tatashin wrote:
> > In gup_test both gup_flags and test_flags use the same flags field.
> > This is broken.
> >
> > Farther, in the actual gup_test.c all the passed gup_flags are erased and
> > unconditionally replaced with FOLL_WRITE.
> >
> > Which means that test_flags are ignored, and code like this always
> > performs pin dump test:
> >
> > 155 if (gup->flags & GUP_TEST_FLAG_DUMP_PAGES_USE_PIN)
> > 156 nr = pin_user_pages(addr, nr, gup->flags,
> > 157 pages + i, NULL);
> > 158 else
> > 159 nr = get_user_pages(addr, nr, gup->flags,
> > 160 pages + i, NULL);
> > 161 break;
> >
> > Add a new test_flags field, to allow raw gup_flags to work.
> > Add a new subcommand for DUMP_USER_PAGES_TEST to specify that pin test
> > should be performed.
> > Remove unconditional overwriting of gup_flags via FOLL_WRITE. But,
> > preserve the previous behaviour where FOLL_WRITE was the default flag,
> > and add a new option "-W" to unset FOLL_WRITE.
> >
> > Rename flags with gup_flags.
>
> Hi Pavel,
>
> Thanks again for fixing this up! Looks good, with a tiny point about the
> subject line:
>
> 1) To follow convention, the subject line should say what you're doing,
> not what the previous condition was. Also, there are several tests in that
> directory, so we should say which one. So more like this:
>
> "selftests/vm: gup_test: fix test flag"
>
> That is just a minor documentation point, so either way, feel free to add:
>
>
> Reviewed-by: John Hubbard
Thank you for your review , I will change the subject line in the next update.
Pasha
>
>
> thanks,
> --
> John Hubbard
> NVIDIA
>
> >
> > With the fix, dump works like this:
> >
> > root@virtme:/# gup_test -c
> > page #0, starting from user virt addr: 0x7f8acb9e4000
> > page:d3d2ee27 refcount:2 mapcount:1 mapping:
> > index:0x0 pfn:0x100bcf
> > anon flags: 0x3080016(referenced|uptodate|lru|swapbacked)
> > raw: 03080016 d0e204021608 d0e208df2e88 8ea04243ec61
> > raw: 0002
> > page dumped because: gup_test: dump_pages() test
> > DUMP_USER_PAGES_TEST: done
> >
> > root@virtme:/# gup_test -c -p
> > page #0, starting from user virt addr: 0x7fd19701b000
> > page:baed3c7d refcount:1025 mapcount:1 mapping:
> > index:0x0 pfn:0x108008
> > anon flags: 0x3080014(uptodate|lru|swapbacked)
> > raw: 03080014 d0e204200188 d0e205e09088 8ea04243ee71
> > raw: 0401
> > page dumped because: gup_test: dump_pages() test
> > DUMP_USER_PAGES_TEST: done
> >
> > Refcount shows the difference between pin vs no-pin case.
> > Also change type of nr from int to long, as it counts number of pages.
> >
> > Signed-off-by: Pavel Tatashin
> > ---
> > mm/gup_test.c | 23 ++-
> > mm/gup_test.h | 3 ++-
> > tools/testing/selftests/vm/gup_test.c | 15 +++
> > 3 files changed, 23 insertions(+), 18 deletions(-)
> >
> > diff --git a/mm/gup_test.c b/mm/gup_test.c
> > index e3cf78e5873e..a6ed1c877679 100644
> > --- a/mm/gup_test.c
> > +++ b/mm/gup_test.c
> > @@ -94,7 +94,7 @@ static int __gup_test_ioctl(unsigned int cmd,
> > {
> > ktime_t start_time, end_time;
> > unsigned long i, nr_pages, addr, next;
> > - int nr;
> > + long nr;
> > struct page **pages;
> > int ret = 0;
> > bool needs_mmap_lock =
> > @@ -126,37 +126,34 @@ static int __gup_test_ioctl(unsigned int cmd,
> > nr = (next - addr) / PAGE_SIZE;
> > }
> >
> > - /* Filter out most gup flags: only allow a tiny subset here:
> > */
> > - gup->flags &= FOLL_WRITE;
> > -
> > switch (cmd) {
> > case GUP_FAST_BENCHMARK:
> > - nr = get_user_pages_fast(addr, nr, gup->flags,
> > + nr = get_user_pages_fast(addr, nr, gup->gup_flags,
> >pages + i);
> > break;
> > case GUP_BASIC_TEST:
> > - nr = get_user_pages(addr, nr, gup->flags, pages + i,
> > + nr = get_user_pages(addr, nr, gup->gup_flags, pages +
> > i,
> > NULL);
> > break;
> > case PIN_FAST_BENCHMARK:
> > - nr = pin_user_pages_fast(addr, nr, gup->flags,
> > + nr = pin_user_pages_fast(addr, nr, gup->gup_flags,
> >pages + i);
> > break;
> >
Re: [PATCH v7 13/14] selftests/vm: test flag is broken
On 1/21/21 7:37 PM, Pavel Tatashin wrote:
In gup_test both gup_flags and test_flags use the same flags field.
This is broken.
Farther, in the actual gup_test.c all the passed gup_flags are erased and
unconditionally replaced with FOLL_WRITE.
Which means that test_flags are ignored, and code like this always
performs pin dump test:
155 if (gup->flags & GUP_TEST_FLAG_DUMP_PAGES_USE_PIN)
156 nr = pin_user_pages(addr, nr, gup->flags,
157 pages + i, NULL);
158 else
159 nr = get_user_pages(addr, nr, gup->flags,
160 pages + i, NULL);
161 break;
Add a new test_flags field, to allow raw gup_flags to work.
Add a new subcommand for DUMP_USER_PAGES_TEST to specify that pin test
should be performed.
Remove unconditional overwriting of gup_flags via FOLL_WRITE. But,
preserve the previous behaviour where FOLL_WRITE was the default flag,
and add a new option "-W" to unset FOLL_WRITE.
Rename flags with gup_flags.
Hi Pavel,
Thanks again for fixing this up! Looks good, with a tiny point about the
subject line:
1) To follow convention, the subject line should say what you're doing,
not what the previous condition was. Also, there are several tests in that
directory, so we should say which one. So more like this:
"selftests/vm: gup_test: fix test flag"
That is just a minor documentation point, so either way, feel free to add:
Reviewed-by: John Hubbard
thanks,
--
John Hubbard
NVIDIA
With the fix, dump works like this:
root@virtme:/# gup_test -c
page #0, starting from user virt addr: 0x7f8acb9e4000
page:d3d2ee27 refcount:2 mapcount:1 mapping:
index:0x0 pfn:0x100bcf
anon flags: 0x3080016(referenced|uptodate|lru|swapbacked)
raw: 03080016 d0e204021608 d0e208df2e88 8ea04243ec61
raw: 0002
page dumped because: gup_test: dump_pages() test
DUMP_USER_PAGES_TEST: done
root@virtme:/# gup_test -c -p
page #0, starting from user virt addr: 0x7fd19701b000
page:baed3c7d refcount:1025 mapcount:1 mapping:
index:0x0 pfn:0x108008
anon flags: 0x3080014(uptodate|lru|swapbacked)
raw: 03080014 d0e204200188 d0e205e09088 8ea04243ee71
raw: 0401
page dumped because: gup_test: dump_pages() test
DUMP_USER_PAGES_TEST: done
Refcount shows the difference between pin vs no-pin case.
Also change type of nr from int to long, as it counts number of pages.
Signed-off-by: Pavel Tatashin
---
mm/gup_test.c | 23 ++-
mm/gup_test.h | 3 ++-
tools/testing/selftests/vm/gup_test.c | 15 +++
3 files changed, 23 insertions(+), 18 deletions(-)
diff --git a/mm/gup_test.c b/mm/gup_test.c
index e3cf78e5873e..a6ed1c877679 100644
--- a/mm/gup_test.c
+++ b/mm/gup_test.c
@@ -94,7 +94,7 @@ static int __gup_test_ioctl(unsigned int cmd,
{
ktime_t start_time, end_time;
unsigned long i, nr_pages, addr, next;
- int nr;
+ long nr;
struct page **pages;
int ret = 0;
bool needs_mmap_lock =
@@ -126,37 +126,34 @@ static int __gup_test_ioctl(unsigned int cmd,
nr = (next - addr) / PAGE_SIZE;
}
- /* Filter out most gup flags: only allow a tiny subset here: */
- gup->flags &= FOLL_WRITE;
-
switch (cmd) {
case GUP_FAST_BENCHMARK:
- nr = get_user_pages_fast(addr, nr, gup->flags,
+ nr = get_user_pages_fast(addr, nr, gup->gup_flags,
pages + i);
break;
case GUP_BASIC_TEST:
- nr = get_user_pages(addr, nr, gup->flags, pages + i,
+ nr = get_user_pages(addr, nr, gup->gup_flags, pages + i,
NULL);
break;
case PIN_FAST_BENCHMARK:
- nr = pin_user_pages_fast(addr, nr, gup->flags,
+ nr = pin_user_pages_fast(addr, nr, gup->gup_flags,
pages + i);
break;
case PIN_BASIC_TEST:
- nr = pin_user_pages(addr, nr, gup->flags, pages + i,
+ nr = pin_user_pages(addr, nr, gup->gup_flags, pages + i,
NULL);
break;
case PIN_LONGTERM_BENCHMARK:
nr = pin_user_pages(addr, nr,
- gup->flags | FOLL_LONGTERM,
+
[PATCH v7 13/14] selftests/vm: test flag is broken
In gup_test both gup_flags and test_flags use the same flags field.
This is broken.
Farther, in the actual gup_test.c all the passed gup_flags are erased and
unconditionally replaced with FOLL_WRITE.
Which means that test_flags are ignored, and code like this always
performs pin dump test:
155 if (gup->flags & GUP_TEST_FLAG_DUMP_PAGES_USE_PIN)
156 nr = pin_user_pages(addr, nr, gup->flags,
157 pages + i, NULL);
158 else
159 nr = get_user_pages(addr, nr, gup->flags,
160 pages + i, NULL);
161 break;
Add a new test_flags field, to allow raw gup_flags to work.
Add a new subcommand for DUMP_USER_PAGES_TEST to specify that pin test
should be performed.
Remove unconditional overwriting of gup_flags via FOLL_WRITE. But,
preserve the previous behaviour where FOLL_WRITE was the default flag,
and add a new option "-W" to unset FOLL_WRITE.
Rename flags with gup_flags.
With the fix, dump works like this:
root@virtme:/# gup_test -c
page #0, starting from user virt addr: 0x7f8acb9e4000
page:d3d2ee27 refcount:2 mapcount:1 mapping:
index:0x0 pfn:0x100bcf
anon flags: 0x3080016(referenced|uptodate|lru|swapbacked)
raw: 03080016 d0e204021608 d0e208df2e88 8ea04243ec61
raw: 0002
page dumped because: gup_test: dump_pages() test
DUMP_USER_PAGES_TEST: done
root@virtme:/# gup_test -c -p
page #0, starting from user virt addr: 0x7fd19701b000
page:baed3c7d refcount:1025 mapcount:1 mapping:
index:0x0 pfn:0x108008
anon flags: 0x3080014(uptodate|lru|swapbacked)
raw: 03080014 d0e204200188 d0e205e09088 8ea04243ee71
raw: 0401
page dumped because: gup_test: dump_pages() test
DUMP_USER_PAGES_TEST: done
Refcount shows the difference between pin vs no-pin case.
Also change type of nr from int to long, as it counts number of pages.
Signed-off-by: Pavel Tatashin
---
mm/gup_test.c | 23 ++-
mm/gup_test.h | 3 ++-
tools/testing/selftests/vm/gup_test.c | 15 +++
3 files changed, 23 insertions(+), 18 deletions(-)
diff --git a/mm/gup_test.c b/mm/gup_test.c
index e3cf78e5873e..a6ed1c877679 100644
--- a/mm/gup_test.c
+++ b/mm/gup_test.c
@@ -94,7 +94,7 @@ static int __gup_test_ioctl(unsigned int cmd,
{
ktime_t start_time, end_time;
unsigned long i, nr_pages, addr, next;
- int nr;
+ long nr;
struct page **pages;
int ret = 0;
bool needs_mmap_lock =
@@ -126,37 +126,34 @@ static int __gup_test_ioctl(unsigned int cmd,
nr = (next - addr) / PAGE_SIZE;
}
- /* Filter out most gup flags: only allow a tiny subset here: */
- gup->flags &= FOLL_WRITE;
-
switch (cmd) {
case GUP_FAST_BENCHMARK:
- nr = get_user_pages_fast(addr, nr, gup->flags,
+ nr = get_user_pages_fast(addr, nr, gup->gup_flags,
pages + i);
break;
case GUP_BASIC_TEST:
- nr = get_user_pages(addr, nr, gup->flags, pages + i,
+ nr = get_user_pages(addr, nr, gup->gup_flags, pages + i,
NULL);
break;
case PIN_FAST_BENCHMARK:
- nr = pin_user_pages_fast(addr, nr, gup->flags,
+ nr = pin_user_pages_fast(addr, nr, gup->gup_flags,
pages + i);
break;
case PIN_BASIC_TEST:
- nr = pin_user_pages(addr, nr, gup->flags, pages + i,
+ nr = pin_user_pages(addr, nr, gup->gup_flags, pages + i,
NULL);
break;
case PIN_LONGTERM_BENCHMARK:
nr = pin_user_pages(addr, nr,
- gup->flags | FOLL_LONGTERM,
+ gup->gup_flags | FOLL_LONGTERM,
pages + i, NULL);
break;
case DUMP_USER_PAGES_TEST:
- if (gup->flags & GUP_TEST_FLAG_DUMP_PAGES_USE_PIN)
- nr = pin_user_pages(addr, nr, gup->flags,
+ if (gup->test_flags & GUP_TEST_FLAG_DUMP_PAGES_USE_PIN)
+ nr = pin_user_pages(addr, nr, gup->gup_flags,
pages +
