| Message ID | 20230330160717.3107010-1-peterx@redhat.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | selftests/mm: Split / Refactor userfault test | expand |
On 03/30/23 12:07, Peter Xu wrote: > The idea was trying to flip this var in the alarm handler from time to time > to test -EEXIST of UFFDIO_ZEROPAGE, but firstly it's only used in the > zeropage test so probably only used once, meanwhile we passed > "retry==false" so it'll never got tested anyway. > > Drop both sides so we always test UFFDIO_ZEROPAGE retries if has_zeropage > is set (!hugetlb). > > One more thing to do is doing UFFDIO_REGISTER for the alias buffer too, > because otherwise the test won't even pass! We were just lucky that this > test never really got ran at all. > > Signed-off-by: Peter Xu <peterx@redhat.com> > --- > tools/testing/selftests/mm/userfaultfd.c | 20 +++++++++++--------- > 1 file changed, 11 insertions(+), 9 deletions(-) Thanks! With only passing "retry==false", that code is indeed dead. I had to read the code again to understand area_dst_alias. Thanks for taking this on as the code is difficult to understand. Reviewed-by: Mike Kravetz <mike.kravetz@oracle.com>
On 30.03.23 18:07, Peter Xu wrote: > The idea was trying to flip this var in the alarm handler from time to time > to test -EEXIST of UFFDIO_ZEROPAGE, but firstly it's only used in the > zeropage test so probably only used once, meanwhile we passed > "retry==false" so it'll never got tested anyway. > > Drop both sides so we always test UFFDIO_ZEROPAGE retries if has_zeropage > is set (!hugetlb). > > One more thing to do is doing UFFDIO_REGISTER for the alias buffer too, > because otherwise the test won't even pass! We were just lucky that this > test never really got ran at all. > > Signed-off-by: Peter Xu <peterx@redhat.com> > --- Reviewed-by: David Hildenbrand <david@redhat.com>
On Fri, Mar 31, 2023 at 05:03:17PM -0700, Mike Kravetz wrote: > On 03/30/23 12:07, Peter Xu wrote: > > The idea was trying to flip this var in the alarm handler from time to time > > to test -EEXIST of UFFDIO_ZEROPAGE, but firstly it's only used in the > > zeropage test so probably only used once, meanwhile we passed > > "retry==false" so it'll never got tested anyway. > > > > Drop both sides so we always test UFFDIO_ZEROPAGE retries if has_zeropage > > is set (!hugetlb). > > > > One more thing to do is doing UFFDIO_REGISTER for the alias buffer too, > > because otherwise the test won't even pass! We were just lucky that this > > test never really got ran at all. > > > > Signed-off-by: Peter Xu <peterx@redhat.com> > > --- > > tools/testing/selftests/mm/userfaultfd.c | 20 +++++++++++--------- > > 1 file changed, 11 insertions(+), 9 deletions(-) > > Thanks! With only passing "retry==false", that code is indeed dead. I had > to read the code again to understand area_dst_alias. Thanks for taking this > on as the code is difficult to understand. Yes it's very confusing. I plan to move on the cleanup to remove all these global variables at least in the unit tests, but I'll first see how this one goes. > > Reviewed-by: Mike Kravetz <mike.kravetz@oracle.com> Thanks,
On Thu, Mar 30, 2023 at 12:07:17PM -0400, Peter Xu wrote: > The idea was trying to flip this var in the alarm handler from time to time > to test -EEXIST of UFFDIO_ZEROPAGE, but firstly it's only used in the > zeropage test so probably only used once, meanwhile we passed > "retry==false" so it'll never got tested anyway. > > Drop both sides so we always test UFFDIO_ZEROPAGE retries if has_zeropage > is set (!hugetlb). > > One more thing to do is doing UFFDIO_REGISTER for the alias buffer too, > because otherwise the test won't even pass! We were just lucky that this > test never really got ran at all. > > Signed-off-by: Peter Xu <peterx@redhat.com> Reviewed-by: Mike Rapoport (IBM) <rppt@kernel.org> > --- > tools/testing/selftests/mm/userfaultfd.c | 20 +++++++++++--------- > 1 file changed, 11 insertions(+), 9 deletions(-) > > diff --git a/tools/testing/selftests/mm/userfaultfd.c b/tools/testing/selftests/mm/userfaultfd.c > index d724f1c78847..3487ec0bfcc8 100644 > --- a/tools/testing/selftests/mm/userfaultfd.c > +++ b/tools/testing/selftests/mm/userfaultfd.c > @@ -88,7 +88,6 @@ static bool test_dev_userfaultfd; > /* exercise the test_uffdio_*_eexist every ALARM_INTERVAL_SECS */ > #define ALARM_INTERVAL_SECS 10 > static volatile bool test_uffdio_copy_eexist = true; > -static volatile bool test_uffdio_zeropage_eexist = true; > /* Whether to test uffd write-protection */ > static bool test_uffdio_wp = true; > /* Whether to test uffd minor faults */ > @@ -1114,7 +1113,7 @@ static void retry_uffdio_zeropage(int ufd, > } > } > > -static int __uffdio_zeropage(int ufd, unsigned long offset, bool retry) > +static int __uffdio_zeropage(int ufd, unsigned long offset) > { > struct uffdio_zeropage uffdio_zeropage; > int ret; > @@ -1138,11 +1137,8 @@ static int __uffdio_zeropage(int ufd, unsigned long offset, bool retry) > if (res != page_size) { > err("UFFDIO_ZEROPAGE unexpected size"); > } else { > - if (test_uffdio_zeropage_eexist && retry) { > - test_uffdio_zeropage_eexist = false; > - retry_uffdio_zeropage(ufd, &uffdio_zeropage, > - offset); > - } > + retry_uffdio_zeropage(ufd, &uffdio_zeropage, > + offset); > return 1; > } > } else > @@ -1153,7 +1149,7 @@ static int __uffdio_zeropage(int ufd, unsigned long offset, bool retry) > > static int uffdio_zeropage(int ufd, unsigned long offset) > { > - return __uffdio_zeropage(ufd, offset, false); > + return __uffdio_zeropage(ufd, offset); > } > > /* exercise UFFDIO_ZEROPAGE */ > @@ -1177,6 +1173,13 @@ static int userfaultfd_zeropage_test(void) > assert_expected_ioctls_present( > uffdio_register.mode, uffdio_register.ioctls); > > + if (area_dst_alias) { > + /* Needed this to test zeropage-retry on shared memory */ > + uffdio_register.range.start = (unsigned long) area_dst_alias; > + if (ioctl(uffd, UFFDIO_REGISTER, &uffdio_register)) > + err("register failure"); > + } > + > if (uffdio_zeropage(uffd, 0)) > if (my_bcmp(area_dst, zeropage, page_size)) > err("zeropage is not zero"); > @@ -1763,7 +1766,6 @@ static void sigalrm(int sig) > if (sig != SIGALRM) > abort(); > test_uffdio_copy_eexist = true; > - test_uffdio_zeropage_eexist = true; > alarm(ALARM_INTERVAL_SECS); > } > > -- > 2.39.1 >
diff --git a/tools/testing/selftests/mm/userfaultfd.c b/tools/testing/selftests/mm/userfaultfd.c index d724f1c78847..3487ec0bfcc8 100644 --- a/tools/testing/selftests/mm/userfaultfd.c +++ b/tools/testing/selftests/mm/userfaultfd.c @@ -88,7 +88,6 @@ static bool test_dev_userfaultfd; /* exercise the test_uffdio_*_eexist every ALARM_INTERVAL_SECS */ #define ALARM_INTERVAL_SECS 10 static volatile bool test_uffdio_copy_eexist = true; -static volatile bool test_uffdio_zeropage_eexist = true; /* Whether to test uffd write-protection */ static bool test_uffdio_wp = true; /* Whether to test uffd minor faults */ @@ -1114,7 +1113,7 @@ static void retry_uffdio_zeropage(int ufd, } } -static int __uffdio_zeropage(int ufd, unsigned long offset, bool retry) +static int __uffdio_zeropage(int ufd, unsigned long offset) { struct uffdio_zeropage uffdio_zeropage; int ret; @@ -1138,11 +1137,8 @@ static int __uffdio_zeropage(int ufd, unsigned long offset, bool retry) if (res != page_size) { err("UFFDIO_ZEROPAGE unexpected size"); } else { - if (test_uffdio_zeropage_eexist && retry) { - test_uffdio_zeropage_eexist = false; - retry_uffdio_zeropage(ufd, &uffdio_zeropage, - offset); - } + retry_uffdio_zeropage(ufd, &uffdio_zeropage, + offset); return 1; } } else @@ -1153,7 +1149,7 @@ static int __uffdio_zeropage(int ufd, unsigned long offset, bool retry) static int uffdio_zeropage(int ufd, unsigned long offset) { - return __uffdio_zeropage(ufd, offset, false); + return __uffdio_zeropage(ufd, offset); } /* exercise UFFDIO_ZEROPAGE */ @@ -1177,6 +1173,13 @@ static int userfaultfd_zeropage_test(void) assert_expected_ioctls_present( uffdio_register.mode, uffdio_register.ioctls); + if (area_dst_alias) { + /* Needed this to test zeropage-retry on shared memory */ + uffdio_register.range.start = (unsigned long) area_dst_alias; + if (ioctl(uffd, UFFDIO_REGISTER, &uffdio_register)) + err("register failure"); + } + if (uffdio_zeropage(uffd, 0)) if (my_bcmp(area_dst, zeropage, page_size)) err("zeropage is not zero"); @@ -1763,7 +1766,6 @@ static void sigalrm(int sig) if (sig != SIGALRM) abort(); test_uffdio_copy_eexist = true; - test_uffdio_zeropage_eexist = true; alarm(ALARM_INTERVAL_SECS); }
The idea was trying to flip this var in the alarm handler from time to time to test -EEXIST of UFFDIO_ZEROPAGE, but firstly it's only used in the zeropage test so probably only used once, meanwhile we passed "retry==false" so it'll never got tested anyway. Drop both sides so we always test UFFDIO_ZEROPAGE retries if has_zeropage is set (!hugetlb). One more thing to do is doing UFFDIO_REGISTER for the alias buffer too, because otherwise the test won't even pass! We were just lucky that this test never really got ran at all. Signed-off-by: Peter Xu <peterx@redhat.com> --- tools/testing/selftests/mm/userfaultfd.c | 20 +++++++++++--------- 1 file changed, 11 insertions(+), 9 deletions(-)