| Message ID | 20230713135440.3651409-4-ryan.roberts@arm.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | selftests/mm fixes for arm64 | expand |
On 13.07.23 15:54, Ryan Roberts wrote: > arm64 does not support the soft-dirty PTE bit. However there are tests > in `madv_populate` and `soft-dirty` which assume it is supported and > cause spurious failures to be reported when preferred behaviour would be > to mark the tests as skipped. > > Unfortunately, the only way to determine if the soft-dirty dirty bit is > supported is to write to a page, then see if the bit is set in > /proc/self/pagemap. But the tests that we want to conditionally execute > are testing precicesly this. So if we introduced this feature check, we > could accedentally turn a real failure (on a system that claims to > support soft-dirty) into a skip. > > So instead, do the check based on architecture; for arm64, we report > that soft-dirty is not supported. This is wrapped up into a utility > function `system_has_softdirty()`, which is used to skip the whole > `soft-dirty` suite, and mark the soft-dirty tests in the `madv_populate` > suite as skipped. > > Signed-off-by: Ryan Roberts <ryan.roberts@arm.com> > --- > tools/testing/selftests/mm/madv_populate.c | 18 +++++++++++++----- > tools/testing/selftests/mm/soft-dirty.c | 3 +++ > tools/testing/selftests/mm/vm_util.c | 17 +++++++++++++++++ > tools/testing/selftests/mm/vm_util.h | 1 + > 4 files changed, 34 insertions(+), 5 deletions(-) > > diff --git a/tools/testing/selftests/mm/madv_populate.c b/tools/testing/selftests/mm/madv_populate.c > index 60547245e479..5a8c176d7fec 100644 > --- a/tools/testing/selftests/mm/madv_populate.c > +++ b/tools/testing/selftests/mm/madv_populate.c > @@ -232,6 +232,14 @@ static bool range_is_not_softdirty(char *start, ssize_t size) > return ret; > } > > +#define ksft_test_result_if_softdirty(cond, ...) \ > +do { \ > + if (system_has_softdirty()) \ > + ksft_test_result(cond, __VA_ARGS__); \ > + else \ > + ksft_test_result_skip(__VA_ARGS__); \ > +} while (0) > + > static void test_softdirty(void) > { > char *addr; > @@ -246,19 +254,19 @@ static void test_softdirty(void) > > /* Clear any softdirty bits. */ > clear_softdirty(); > - ksft_test_result(range_is_not_softdirty(addr, SIZE), > + ksft_test_result_if_softdirty(range_is_not_softdirty(addr, SIZE), > "range is not softdirty\n"); > > /* Populating READ should set softdirty. */ > ret = madvise(addr, SIZE, MADV_POPULATE_READ); > - ksft_test_result(!ret, "MADV_POPULATE_READ\n"); > - ksft_test_result(range_is_not_softdirty(addr, SIZE), > + ksft_test_result_if_softdirty(!ret, "MADV_POPULATE_READ\n"); > + ksft_test_result_if_softdirty(range_is_not_softdirty(addr, SIZE), > "range is not softdirty\n"); > > /* Populating WRITE should set softdirty. */ > ret = madvise(addr, SIZE, MADV_POPULATE_WRITE); > - ksft_test_result(!ret, "MADV_POPULATE_WRITE\n"); > - ksft_test_result(range_is_softdirty(addr, SIZE), > + ksft_test_result_if_softdirty(!ret, "MADV_POPULATE_WRITE\n"); > + ksft_test_result_if_softdirty(range_is_softdirty(addr, SIZE), > "range is softdirty\n"); We probably want to skip the whole test_*softdirty* test instead of adding this (IMHO suboptimal) ksft_test_result_if_softdirty.
On 13/07/2023 14:56, David Hildenbrand wrote: > On 13.07.23 15:54, Ryan Roberts wrote: >> arm64 does not support the soft-dirty PTE bit. However there are tests >> in `madv_populate` and `soft-dirty` which assume it is supported and >> cause spurious failures to be reported when preferred behaviour would be >> to mark the tests as skipped. >> >> Unfortunately, the only way to determine if the soft-dirty dirty bit is >> supported is to write to a page, then see if the bit is set in >> /proc/self/pagemap. But the tests that we want to conditionally execute >> are testing precicesly this. So if we introduced this feature check, we >> could accedentally turn a real failure (on a system that claims to >> support soft-dirty) into a skip. >> >> So instead, do the check based on architecture; for arm64, we report >> that soft-dirty is not supported. This is wrapped up into a utility >> function `system_has_softdirty()`, which is used to skip the whole >> `soft-dirty` suite, and mark the soft-dirty tests in the `madv_populate` >> suite as skipped. >> >> Signed-off-by: Ryan Roberts <ryan.roberts@arm.com> >> --- >> tools/testing/selftests/mm/madv_populate.c | 18 +++++++++++++----- >> tools/testing/selftests/mm/soft-dirty.c | 3 +++ >> tools/testing/selftests/mm/vm_util.c | 17 +++++++++++++++++ >> tools/testing/selftests/mm/vm_util.h | 1 + >> 4 files changed, 34 insertions(+), 5 deletions(-) >> >> diff --git a/tools/testing/selftests/mm/madv_populate.c >> b/tools/testing/selftests/mm/madv_populate.c >> index 60547245e479..5a8c176d7fec 100644 >> --- a/tools/testing/selftests/mm/madv_populate.c >> +++ b/tools/testing/selftests/mm/madv_populate.c >> @@ -232,6 +232,14 @@ static bool range_is_not_softdirty(char *start, ssize_t >> size) >> return ret; >> } >> >> +#define ksft_test_result_if_softdirty(cond, ...) \ >> +do { \ >> + if (system_has_softdirty()) \ >> + ksft_test_result(cond, __VA_ARGS__); \ >> + else \ >> + ksft_test_result_skip(__VA_ARGS__); \ >> +} while (0) >> + >> static void test_softdirty(void) >> { >> char *addr; >> @@ -246,19 +254,19 @@ static void test_softdirty(void) >> >> /* Clear any softdirty bits. */ >> clear_softdirty(); >> - ksft_test_result(range_is_not_softdirty(addr, SIZE), >> + ksft_test_result_if_softdirty(range_is_not_softdirty(addr, SIZE), >> "range is not softdirty\n"); >> >> /* Populating READ should set softdirty. */ >> ret = madvise(addr, SIZE, MADV_POPULATE_READ); >> - ksft_test_result(!ret, "MADV_POPULATE_READ\n"); >> - ksft_test_result(range_is_not_softdirty(addr, SIZE), >> + ksft_test_result_if_softdirty(!ret, "MADV_POPULATE_READ\n"); >> + ksft_test_result_if_softdirty(range_is_not_softdirty(addr, SIZE), >> "range is not softdirty\n"); >> >> /* Populating WRITE should set softdirty. */ >> ret = madvise(addr, SIZE, MADV_POPULATE_WRITE); >> - ksft_test_result(!ret, "MADV_POPULATE_WRITE\n"); >> - ksft_test_result(range_is_softdirty(addr, SIZE), >> + ksft_test_result_if_softdirty(!ret, "MADV_POPULATE_WRITE\n"); >> + ksft_test_result_if_softdirty(range_is_softdirty(addr, SIZE), >> "range is softdirty\n"); > > We probably want to skip the whole test_*softdirty* test instead of adding this > (IMHO suboptimal) ksft_test_result_if_softdirty. Yeah I thought about doing it that way, but then the output just looks like there were fewer tests and they all passed. But thinking about it now, I guess the TAP header outputs the number of planned tests and the number of tests executed are fewer, so a machine parser would still notice. I just don't like that it outputs skipped:0. But it a lightly held view. Happy to just do: if (system_has_softdirty()) test_softdirty() If you insist. ;-) >
On 13.07.23 16:03, Ryan Roberts wrote: > On 13/07/2023 14:56, David Hildenbrand wrote: >> On 13.07.23 15:54, Ryan Roberts wrote: >>> arm64 does not support the soft-dirty PTE bit. However there are tests >>> in `madv_populate` and `soft-dirty` which assume it is supported and >>> cause spurious failures to be reported when preferred behaviour would be >>> to mark the tests as skipped. >>> >>> Unfortunately, the only way to determine if the soft-dirty dirty bit is >>> supported is to write to a page, then see if the bit is set in >>> /proc/self/pagemap. But the tests that we want to conditionally execute >>> are testing precicesly this. So if we introduced this feature check, we >>> could accedentally turn a real failure (on a system that claims to >>> support soft-dirty) into a skip. >>> >>> So instead, do the check based on architecture; for arm64, we report >>> that soft-dirty is not supported. This is wrapped up into a utility >>> function `system_has_softdirty()`, which is used to skip the whole >>> `soft-dirty` suite, and mark the soft-dirty tests in the `madv_populate` >>> suite as skipped. >>> >>> Signed-off-by: Ryan Roberts <ryan.roberts@arm.com> >>> --- >>> tools/testing/selftests/mm/madv_populate.c | 18 +++++++++++++----- >>> tools/testing/selftests/mm/soft-dirty.c | 3 +++ >>> tools/testing/selftests/mm/vm_util.c | 17 +++++++++++++++++ >>> tools/testing/selftests/mm/vm_util.h | 1 + >>> 4 files changed, 34 insertions(+), 5 deletions(-) >>> >>> diff --git a/tools/testing/selftests/mm/madv_populate.c >>> b/tools/testing/selftests/mm/madv_populate.c >>> index 60547245e479..5a8c176d7fec 100644 >>> --- a/tools/testing/selftests/mm/madv_populate.c >>> +++ b/tools/testing/selftests/mm/madv_populate.c >>> @@ -232,6 +232,14 @@ static bool range_is_not_softdirty(char *start, ssize_t >>> size) >>> return ret; >>> } >>> >>> +#define ksft_test_result_if_softdirty(cond, ...) \ >>> +do { \ >>> + if (system_has_softdirty()) \ >>> + ksft_test_result(cond, __VA_ARGS__); \ >>> + else \ >>> + ksft_test_result_skip(__VA_ARGS__); \ >>> +} while (0) >>> + >>> static void test_softdirty(void) >>> { >>> char *addr; >>> @@ -246,19 +254,19 @@ static void test_softdirty(void) >>> >>> /* Clear any softdirty bits. */ >>> clear_softdirty(); >>> - ksft_test_result(range_is_not_softdirty(addr, SIZE), >>> + ksft_test_result_if_softdirty(range_is_not_softdirty(addr, SIZE), >>> "range is not softdirty\n"); >>> >>> /* Populating READ should set softdirty. */ >>> ret = madvise(addr, SIZE, MADV_POPULATE_READ); >>> - ksft_test_result(!ret, "MADV_POPULATE_READ\n"); >>> - ksft_test_result(range_is_not_softdirty(addr, SIZE), >>> + ksft_test_result_if_softdirty(!ret, "MADV_POPULATE_READ\n"); >>> + ksft_test_result_if_softdirty(range_is_not_softdirty(addr, SIZE), >>> "range is not softdirty\n"); >>> >>> /* Populating WRITE should set softdirty. */ >>> ret = madvise(addr, SIZE, MADV_POPULATE_WRITE); >>> - ksft_test_result(!ret, "MADV_POPULATE_WRITE\n"); >>> - ksft_test_result(range_is_softdirty(addr, SIZE), >>> + ksft_test_result_if_softdirty(!ret, "MADV_POPULATE_WRITE\n"); >>> + ksft_test_result_if_softdirty(range_is_softdirty(addr, SIZE), >>> "range is softdirty\n"); >> >> We probably want to skip the whole test_*softdirty* test instead of adding this >> (IMHO suboptimal) ksft_test_result_if_softdirty. > > Yeah I thought about doing it that way, but then the output just looks like > there were fewer tests and they all passed. But thinking about it now, I guess > the TAP header outputs the number of planned tests and the number of tests > executed are fewer, so a machine parser would still notice. I just don't like > that it outputs skipped:0. > > But it a lightly held view. Happy to just do: > > if (system_has_softdirty()) > test_softdirty() > > If you insist. ;-) diff --git a/tools/testing/selftests/mm/madv_populate.c b/tools/testing/selftests/mm/madv_populate.c index 60547245e479..33fda0337b32 100644 --- a/tools/testing/selftests/mm/madv_populate.c +++ b/tools/testing/selftests/mm/madv_populate.c @@ -266,12 +266,16 @@ static void test_softdirty(void) int main(int argc, char **argv) { + int nr_tests = 16; int err; pagesize = getpagesize(); + if (system_has_softdirty()) + nr_tests += 5; + ksft_print_header(); - ksft_set_plan(21); + ksft_set_plan(nr_tests); sense_support(); test_prot_read(); @@ -279,7 +283,8 @@ int main(int argc, char **argv) test_holes(); test_populate_read(); test_populate_write(); - test_softdirty(); + if (system_has_softdirty()) + test_softdirty(); err = ksft_get_fail_cnt(); if (err)
On 13.07.23 16:09, David Hildenbrand wrote: > On 13.07.23 16:03, Ryan Roberts wrote: >> On 13/07/2023 14:56, David Hildenbrand wrote: >>> On 13.07.23 15:54, Ryan Roberts wrote: >>>> arm64 does not support the soft-dirty PTE bit. However there are tests >>>> in `madv_populate` and `soft-dirty` which assume it is supported and >>>> cause spurious failures to be reported when preferred behaviour would be >>>> to mark the tests as skipped. >>>> >>>> Unfortunately, the only way to determine if the soft-dirty dirty bit is >>>> supported is to write to a page, then see if the bit is set in >>>> /proc/self/pagemap. But the tests that we want to conditionally execute >>>> are testing precicesly this. So if we introduced this feature check, we >>>> could accedentally turn a real failure (on a system that claims to >>>> support soft-dirty) into a skip. >>>> >>>> So instead, do the check based on architecture; for arm64, we report >>>> that soft-dirty is not supported. This is wrapped up into a utility >>>> function `system_has_softdirty()`, which is used to skip the whole >>>> `soft-dirty` suite, and mark the soft-dirty tests in the `madv_populate` >>>> suite as skipped. >>>> >>>> Signed-off-by: Ryan Roberts <ryan.roberts@arm.com> >>>> --- >>>> tools/testing/selftests/mm/madv_populate.c | 18 +++++++++++++----- >>>> tools/testing/selftests/mm/soft-dirty.c | 3 +++ >>>> tools/testing/selftests/mm/vm_util.c | 17 +++++++++++++++++ >>>> tools/testing/selftests/mm/vm_util.h | 1 + >>>> 4 files changed, 34 insertions(+), 5 deletions(-) >>>> >>>> diff --git a/tools/testing/selftests/mm/madv_populate.c >>>> b/tools/testing/selftests/mm/madv_populate.c >>>> index 60547245e479..5a8c176d7fec 100644 >>>> --- a/tools/testing/selftests/mm/madv_populate.c >>>> +++ b/tools/testing/selftests/mm/madv_populate.c >>>> @@ -232,6 +232,14 @@ static bool range_is_not_softdirty(char *start, ssize_t >>>> size) >>>> return ret; >>>> } >>>> >>>> +#define ksft_test_result_if_softdirty(cond, ...) \ >>>> +do { \ >>>> + if (system_has_softdirty()) \ >>>> + ksft_test_result(cond, __VA_ARGS__); \ >>>> + else \ >>>> + ksft_test_result_skip(__VA_ARGS__); \ >>>> +} while (0) >>>> + >>>> static void test_softdirty(void) >>>> { >>>> char *addr; >>>> @@ -246,19 +254,19 @@ static void test_softdirty(void) >>>> >>>> /* Clear any softdirty bits. */ >>>> clear_softdirty(); >>>> - ksft_test_result(range_is_not_softdirty(addr, SIZE), >>>> + ksft_test_result_if_softdirty(range_is_not_softdirty(addr, SIZE), >>>> "range is not softdirty\n"); >>>> >>>> /* Populating READ should set softdirty. */ >>>> ret = madvise(addr, SIZE, MADV_POPULATE_READ); >>>> - ksft_test_result(!ret, "MADV_POPULATE_READ\n"); >>>> - ksft_test_result(range_is_not_softdirty(addr, SIZE), >>>> + ksft_test_result_if_softdirty(!ret, "MADV_POPULATE_READ\n"); >>>> + ksft_test_result_if_softdirty(range_is_not_softdirty(addr, SIZE), >>>> "range is not softdirty\n"); >>>> >>>> /* Populating WRITE should set softdirty. */ >>>> ret = madvise(addr, SIZE, MADV_POPULATE_WRITE); >>>> - ksft_test_result(!ret, "MADV_POPULATE_WRITE\n"); >>>> - ksft_test_result(range_is_softdirty(addr, SIZE), >>>> + ksft_test_result_if_softdirty(!ret, "MADV_POPULATE_WRITE\n"); >>>> + ksft_test_result_if_softdirty(range_is_softdirty(addr, SIZE), >>>> "range is softdirty\n"); >>> >>> We probably want to skip the whole test_*softdirty* test instead of adding this >>> (IMHO suboptimal) ksft_test_result_if_softdirty. >> >> Yeah I thought about doing it that way, but then the output just looks like >> there were fewer tests and they all passed. But thinking about it now, I guess >> the TAP header outputs the number of planned tests and the number of tests >> executed are fewer, so a machine parser would still notice. I just don't like >> that it outputs skipped:0. >> >> But it a lightly held view. Happy to just do: >> >> if (system_has_softdirty()) >> test_softdirty() >> >> If you insist. ;-) > > diff --git a/tools/testing/selftests/mm/madv_populate.c b/tools/testing/selftests/mm/madv_populate.c > index 60547245e479..33fda0337b32 100644 > --- a/tools/testing/selftests/mm/madv_populate.c > +++ b/tools/testing/selftests/mm/madv_populate.c > @@ -266,12 +266,16 @@ static void test_softdirty(void) > > int main(int argc, char **argv) > { > + int nr_tests = 16; > int err; > > pagesize = getpagesize(); > > + if (system_has_softdirty()) > + nr_tests += 5; > + > ksft_print_header(); > - ksft_set_plan(21); > + ksft_set_plan(nr_tests); > > sense_support(); > test_prot_read(); > @@ -279,7 +283,8 @@ int main(int argc, char **argv) > test_holes(); > test_populate_read(); > test_populate_write(); > - test_softdirty(); > + if (system_has_softdirty()) > + test_softdirty(); > > err = ksft_get_fail_cnt(); > if (err) > > Oh, and if you want to have the skip, then you can think about converting test_softdirty() to only perform a single ksft_test_result(), and have a single skip on top. All cleaner IMHO than ksft_test_result_if_softdirty ;)
On 13/07/2023 15:09, David Hildenbrand wrote: > On 13.07.23 16:03, Ryan Roberts wrote: >> On 13/07/2023 14:56, David Hildenbrand wrote: >>> On 13.07.23 15:54, Ryan Roberts wrote: >>>> arm64 does not support the soft-dirty PTE bit. However there are tests >>>> in `madv_populate` and `soft-dirty` which assume it is supported and >>>> cause spurious failures to be reported when preferred behaviour would be >>>> to mark the tests as skipped. >>>> >>>> Unfortunately, the only way to determine if the soft-dirty dirty bit is >>>> supported is to write to a page, then see if the bit is set in >>>> /proc/self/pagemap. But the tests that we want to conditionally execute >>>> are testing precicesly this. So if we introduced this feature check, we >>>> could accedentally turn a real failure (on a system that claims to >>>> support soft-dirty) into a skip. >>>> >>>> So instead, do the check based on architecture; for arm64, we report >>>> that soft-dirty is not supported. This is wrapped up into a utility >>>> function `system_has_softdirty()`, which is used to skip the whole >>>> `soft-dirty` suite, and mark the soft-dirty tests in the `madv_populate` >>>> suite as skipped. >>>> >>>> Signed-off-by: Ryan Roberts <ryan.roberts@arm.com> >>>> --- >>>> tools/testing/selftests/mm/madv_populate.c | 18 +++++++++++++----- >>>> tools/testing/selftests/mm/soft-dirty.c | 3 +++ >>>> tools/testing/selftests/mm/vm_util.c | 17 +++++++++++++++++ >>>> tools/testing/selftests/mm/vm_util.h | 1 + >>>> 4 files changed, 34 insertions(+), 5 deletions(-) >>>> >>>> diff --git a/tools/testing/selftests/mm/madv_populate.c >>>> b/tools/testing/selftests/mm/madv_populate.c >>>> index 60547245e479..5a8c176d7fec 100644 >>>> --- a/tools/testing/selftests/mm/madv_populate.c >>>> +++ b/tools/testing/selftests/mm/madv_populate.c >>>> @@ -232,6 +232,14 @@ static bool range_is_not_softdirty(char *start, ssize_t >>>> size) >>>> return ret; >>>> } >>>> >>>> +#define ksft_test_result_if_softdirty(cond, ...) \ >>>> +do { \ >>>> + if (system_has_softdirty()) \ >>>> + ksft_test_result(cond, __VA_ARGS__); \ >>>> + else \ >>>> + ksft_test_result_skip(__VA_ARGS__); \ >>>> +} while (0) >>>> + >>>> static void test_softdirty(void) >>>> { >>>> char *addr; >>>> @@ -246,19 +254,19 @@ static void test_softdirty(void) >>>> >>>> /* Clear any softdirty bits. */ >>>> clear_softdirty(); >>>> - ksft_test_result(range_is_not_softdirty(addr, SIZE), >>>> + ksft_test_result_if_softdirty(range_is_not_softdirty(addr, SIZE), >>>> "range is not softdirty\n"); >>>> >>>> /* Populating READ should set softdirty. */ >>>> ret = madvise(addr, SIZE, MADV_POPULATE_READ); >>>> - ksft_test_result(!ret, "MADV_POPULATE_READ\n"); >>>> - ksft_test_result(range_is_not_softdirty(addr, SIZE), >>>> + ksft_test_result_if_softdirty(!ret, "MADV_POPULATE_READ\n"); >>>> + ksft_test_result_if_softdirty(range_is_not_softdirty(addr, SIZE), >>>> "range is not softdirty\n"); >>>> >>>> /* Populating WRITE should set softdirty. */ >>>> ret = madvise(addr, SIZE, MADV_POPULATE_WRITE); >>>> - ksft_test_result(!ret, "MADV_POPULATE_WRITE\n"); >>>> - ksft_test_result(range_is_softdirty(addr, SIZE), >>>> + ksft_test_result_if_softdirty(!ret, "MADV_POPULATE_WRITE\n"); >>>> + ksft_test_result_if_softdirty(range_is_softdirty(addr, SIZE), >>>> "range is softdirty\n"); >>> >>> We probably want to skip the whole test_*softdirty* test instead of adding this >>> (IMHO suboptimal) ksft_test_result_if_softdirty. >> >> Yeah I thought about doing it that way, but then the output just looks like >> there were fewer tests and they all passed. But thinking about it now, I guess >> the TAP header outputs the number of planned tests and the number of tests >> executed are fewer, so a machine parser would still notice. I just don't like >> that it outputs skipped:0. >> >> But it a lightly held view. Happy to just do: >> >> if (system_has_softdirty()) >> test_softdirty() >> >> If you insist. ;-) > > diff --git a/tools/testing/selftests/mm/madv_populate.c > b/tools/testing/selftests/mm/madv_populate.c > index 60547245e479..33fda0337b32 100644 > --- a/tools/testing/selftests/mm/madv_populate.c > +++ b/tools/testing/selftests/mm/madv_populate.c > @@ -266,12 +266,16 @@ static void test_softdirty(void) > > int main(int argc, char **argv) > { > + int nr_tests = 16; > int err; > > pagesize = getpagesize(); > > + if (system_has_softdirty()) > + nr_tests += 5; This is the opposite of the point I was trying to make; If there are 21 tests in a suite, I'd like to know that there are 21 tests, 16 of which passed and 5 of which were skipped. This will hide the 5 from the test report. > + > ksft_print_header(); > - ksft_set_plan(21); > + ksft_set_plan(nr_tests); > > sense_support(); > test_prot_read(); > @@ -279,7 +283,8 @@ int main(int argc, char **argv) > test_holes(); > test_populate_read(); > test_populate_write(); > - test_softdirty(); > + if (system_has_softdirty()) > + test_softdirty(); > > err = ksft_get_fail_cnt(); > if (err) > >
On 13/07/2023 15:12, David Hildenbrand wrote: > On 13.07.23 16:09, David Hildenbrand wrote: >> On 13.07.23 16:03, Ryan Roberts wrote: >>> On 13/07/2023 14:56, David Hildenbrand wrote: >>>> On 13.07.23 15:54, Ryan Roberts wrote: >>>>> arm64 does not support the soft-dirty PTE bit. However there are tests >>>>> in `madv_populate` and `soft-dirty` which assume it is supported and >>>>> cause spurious failures to be reported when preferred behaviour would be >>>>> to mark the tests as skipped. >>>>> >>>>> Unfortunately, the only way to determine if the soft-dirty dirty bit is >>>>> supported is to write to a page, then see if the bit is set in >>>>> /proc/self/pagemap. But the tests that we want to conditionally execute >>>>> are testing precicesly this. So if we introduced this feature check, we >>>>> could accedentally turn a real failure (on a system that claims to >>>>> support soft-dirty) into a skip. >>>>> >>>>> So instead, do the check based on architecture; for arm64, we report >>>>> that soft-dirty is not supported. This is wrapped up into a utility >>>>> function `system_has_softdirty()`, which is used to skip the whole >>>>> `soft-dirty` suite, and mark the soft-dirty tests in the `madv_populate` >>>>> suite as skipped. >>>>> >>>>> Signed-off-by: Ryan Roberts <ryan.roberts@arm.com> >>>>> --- >>>>> tools/testing/selftests/mm/madv_populate.c | 18 +++++++++++++----- >>>>> tools/testing/selftests/mm/soft-dirty.c | 3 +++ >>>>> tools/testing/selftests/mm/vm_util.c | 17 +++++++++++++++++ >>>>> tools/testing/selftests/mm/vm_util.h | 1 + >>>>> 4 files changed, 34 insertions(+), 5 deletions(-) >>>>> >>>>> diff --git a/tools/testing/selftests/mm/madv_populate.c >>>>> b/tools/testing/selftests/mm/madv_populate.c >>>>> index 60547245e479..5a8c176d7fec 100644 >>>>> --- a/tools/testing/selftests/mm/madv_populate.c >>>>> +++ b/tools/testing/selftests/mm/madv_populate.c >>>>> @@ -232,6 +232,14 @@ static bool range_is_not_softdirty(char *start, ssize_t >>>>> size) >>>>> return ret; >>>>> } >>>>> >>>>> +#define ksft_test_result_if_softdirty(cond, ...) \ >>>>> +do { \ >>>>> + if (system_has_softdirty()) \ >>>>> + ksft_test_result(cond, __VA_ARGS__); \ >>>>> + else \ >>>>> + ksft_test_result_skip(__VA_ARGS__); \ >>>>> +} while (0) >>>>> + >>>>> static void test_softdirty(void) >>>>> { >>>>> char *addr; >>>>> @@ -246,19 +254,19 @@ static void test_softdirty(void) >>>>> >>>>> /* Clear any softdirty bits. */ >>>>> clear_softdirty(); >>>>> - ksft_test_result(range_is_not_softdirty(addr, SIZE), >>>>> + ksft_test_result_if_softdirty(range_is_not_softdirty(addr, SIZE), >>>>> "range is not softdirty\n"); >>>>> >>>>> /* Populating READ should set softdirty. */ >>>>> ret = madvise(addr, SIZE, MADV_POPULATE_READ); >>>>> - ksft_test_result(!ret, "MADV_POPULATE_READ\n"); >>>>> - ksft_test_result(range_is_not_softdirty(addr, SIZE), >>>>> + ksft_test_result_if_softdirty(!ret, "MADV_POPULATE_READ\n"); >>>>> + ksft_test_result_if_softdirty(range_is_not_softdirty(addr, SIZE), >>>>> "range is not softdirty\n"); >>>>> >>>>> /* Populating WRITE should set softdirty. */ >>>>> ret = madvise(addr, SIZE, MADV_POPULATE_WRITE); >>>>> - ksft_test_result(!ret, "MADV_POPULATE_WRITE\n"); >>>>> - ksft_test_result(range_is_softdirty(addr, SIZE), >>>>> + ksft_test_result_if_softdirty(!ret, "MADV_POPULATE_WRITE\n"); >>>>> + ksft_test_result_if_softdirty(range_is_softdirty(addr, SIZE), >>>>> "range is softdirty\n"); >>>> >>>> We probably want to skip the whole test_*softdirty* test instead of adding this >>>> (IMHO suboptimal) ksft_test_result_if_softdirty. >>> >>> Yeah I thought about doing it that way, but then the output just looks like >>> there were fewer tests and they all passed. But thinking about it now, I guess >>> the TAP header outputs the number of planned tests and the number of tests >>> executed are fewer, so a machine parser would still notice. I just don't like >>> that it outputs skipped:0. >>> >>> But it a lightly held view. Happy to just do: >>> >>> if (system_has_softdirty()) >>> test_softdirty() >>> >>> If you insist. ;-) >> >> diff --git a/tools/testing/selftests/mm/madv_populate.c >> b/tools/testing/selftests/mm/madv_populate.c >> index 60547245e479..33fda0337b32 100644 >> --- a/tools/testing/selftests/mm/madv_populate.c >> +++ b/tools/testing/selftests/mm/madv_populate.c >> @@ -266,12 +266,16 @@ static void test_softdirty(void) >> int main(int argc, char **argv) >> { >> + int nr_tests = 16; >> int err; >> pagesize = getpagesize(); >> + if (system_has_softdirty()) >> + nr_tests += 5; >> + >> ksft_print_header(); >> - ksft_set_plan(21); >> + ksft_set_plan(nr_tests); >> sense_support(); >> test_prot_read(); >> @@ -279,7 +283,8 @@ int main(int argc, char **argv) >> test_holes(); >> test_populate_read(); >> test_populate_write(); >> - test_softdirty(); >> + if (system_has_softdirty()) >> + test_softdirty(); >> err = ksft_get_fail_cnt(); >> if (err) >> >> > > Oh, and if you want to have the skip, then you can think about converting > test_softdirty() to only perform a single ksft_test_result(), and have a single > skip on top. > > All cleaner IMHO than ksft_test_result_if_softdirty ;) I'll do it the way you recommend above. Like I said, its a lightly held opinion, and your way looks like less effort. ;-) >
On 13.07.23 16:14, Ryan Roberts wrote: > On 13/07/2023 15:09, David Hildenbrand wrote: >> On 13.07.23 16:03, Ryan Roberts wrote: >>> On 13/07/2023 14:56, David Hildenbrand wrote: >>>> On 13.07.23 15:54, Ryan Roberts wrote: >>>>> arm64 does not support the soft-dirty PTE bit. However there are tests >>>>> in `madv_populate` and `soft-dirty` which assume it is supported and >>>>> cause spurious failures to be reported when preferred behaviour would be >>>>> to mark the tests as skipped. >>>>> >>>>> Unfortunately, the only way to determine if the soft-dirty dirty bit is >>>>> supported is to write to a page, then see if the bit is set in >>>>> /proc/self/pagemap. But the tests that we want to conditionally execute >>>>> are testing precicesly this. So if we introduced this feature check, we >>>>> could accedentally turn a real failure (on a system that claims to >>>>> support soft-dirty) into a skip. >>>>> >>>>> So instead, do the check based on architecture; for arm64, we report >>>>> that soft-dirty is not supported. This is wrapped up into a utility >>>>> function `system_has_softdirty()`, which is used to skip the whole >>>>> `soft-dirty` suite, and mark the soft-dirty tests in the `madv_populate` >>>>> suite as skipped. >>>>> >>>>> Signed-off-by: Ryan Roberts <ryan.roberts@arm.com> >>>>> --- >>>>> tools/testing/selftests/mm/madv_populate.c | 18 +++++++++++++----- >>>>> tools/testing/selftests/mm/soft-dirty.c | 3 +++ >>>>> tools/testing/selftests/mm/vm_util.c | 17 +++++++++++++++++ >>>>> tools/testing/selftests/mm/vm_util.h | 1 + >>>>> 4 files changed, 34 insertions(+), 5 deletions(-) >>>>> >>>>> diff --git a/tools/testing/selftests/mm/madv_populate.c >>>>> b/tools/testing/selftests/mm/madv_populate.c >>>>> index 60547245e479..5a8c176d7fec 100644 >>>>> --- a/tools/testing/selftests/mm/madv_populate.c >>>>> +++ b/tools/testing/selftests/mm/madv_populate.c >>>>> @@ -232,6 +232,14 @@ static bool range_is_not_softdirty(char *start, ssize_t >>>>> size) >>>>> return ret; >>>>> } >>>>> >>>>> +#define ksft_test_result_if_softdirty(cond, ...) \ >>>>> +do { \ >>>>> + if (system_has_softdirty()) \ >>>>> + ksft_test_result(cond, __VA_ARGS__); \ >>>>> + else \ >>>>> + ksft_test_result_skip(__VA_ARGS__); \ >>>>> +} while (0) >>>>> + >>>>> static void test_softdirty(void) >>>>> { >>>>> char *addr; >>>>> @@ -246,19 +254,19 @@ static void test_softdirty(void) >>>>> >>>>> /* Clear any softdirty bits. */ >>>>> clear_softdirty(); >>>>> - ksft_test_result(range_is_not_softdirty(addr, SIZE), >>>>> + ksft_test_result_if_softdirty(range_is_not_softdirty(addr, SIZE), >>>>> "range is not softdirty\n"); >>>>> >>>>> /* Populating READ should set softdirty. */ >>>>> ret = madvise(addr, SIZE, MADV_POPULATE_READ); >>>>> - ksft_test_result(!ret, "MADV_POPULATE_READ\n"); >>>>> - ksft_test_result(range_is_not_softdirty(addr, SIZE), >>>>> + ksft_test_result_if_softdirty(!ret, "MADV_POPULATE_READ\n"); >>>>> + ksft_test_result_if_softdirty(range_is_not_softdirty(addr, SIZE), >>>>> "range is not softdirty\n"); >>>>> >>>>> /* Populating WRITE should set softdirty. */ >>>>> ret = madvise(addr, SIZE, MADV_POPULATE_WRITE); >>>>> - ksft_test_result(!ret, "MADV_POPULATE_WRITE\n"); >>>>> - ksft_test_result(range_is_softdirty(addr, SIZE), >>>>> + ksft_test_result_if_softdirty(!ret, "MADV_POPULATE_WRITE\n"); >>>>> + ksft_test_result_if_softdirty(range_is_softdirty(addr, SIZE), >>>>> "range is softdirty\n"); >>>> >>>> We probably want to skip the whole test_*softdirty* test instead of adding this >>>> (IMHO suboptimal) ksft_test_result_if_softdirty. >>> >>> Yeah I thought about doing it that way, but then the output just looks like >>> there were fewer tests and they all passed. But thinking about it now, I guess >>> the TAP header outputs the number of planned tests and the number of tests >>> executed are fewer, so a machine parser would still notice. I just don't like >>> that it outputs skipped:0. >>> >>> But it a lightly held view. Happy to just do: >>> >>> if (system_has_softdirty()) >>> test_softdirty() >>> >>> If you insist. ;-) >> >> diff --git a/tools/testing/selftests/mm/madv_populate.c >> b/tools/testing/selftests/mm/madv_populate.c >> index 60547245e479..33fda0337b32 100644 >> --- a/tools/testing/selftests/mm/madv_populate.c >> +++ b/tools/testing/selftests/mm/madv_populate.c >> @@ -266,12 +266,16 @@ static void test_softdirty(void) >> >> int main(int argc, char **argv) >> { >> + int nr_tests = 16; >> int err; >> >> pagesize = getpagesize(); >> >> + if (system_has_softdirty()) >> + nr_tests += 5; > > This is the opposite of the point I was trying to make; If there are 21 tests in > a suite, I'd like to know that there are 21 tests, 16 of which passed and 5 of > which were skipped. This will hide the 5 from the test report. Well, these test are impossible on that architecture, which is IMHO different to some kind of "impossible in the configuration" like "no swap", "no hugetlb", "no THP available".
On 7/13/23 06:54, Ryan Roberts wrote: > arm64 does not support the soft-dirty PTE bit. However there are tests > in `madv_populate` and `soft-dirty` which assume it is supported and > cause spurious failures to be reported when preferred behaviour would be > to mark the tests as skipped. > > Unfortunately, the only way to determine if the soft-dirty dirty bit is > supported is to write to a page, then see if the bit is set in > /proc/self/pagemap. But the tests that we want to conditionally execute > are testing precicesly this. So if we introduced this feature check, we > could accedentally turn a real failure (on a system that claims to > support soft-dirty) into a skip. ... > diff --git a/tools/testing/selftests/mm/soft-dirty.c b/tools/testing/selftests/mm/soft-dirty.c > index cc5f144430d4..8a2cd161ec4d 100644 > --- a/tools/testing/selftests/mm/soft-dirty.c > +++ b/tools/testing/selftests/mm/soft-dirty.c Hi Ryan, Probably very similar to what David is requesting: given that arm64 definitively does not support soft dirty, I'd suggest that we not even *build* the soft dirty tests on arm64! There is no need to worry about counting, skipping or waiving such tests, either. Because it's just a non-issue: one does not care about test status for something that is documented as "this feature is simply unavailable here". thanks,
On 15/07/2023 01:04, John Hubbard wrote: > On 7/13/23 06:54, Ryan Roberts wrote: >> arm64 does not support the soft-dirty PTE bit. However there are tests >> in `madv_populate` and `soft-dirty` which assume it is supported and >> cause spurious failures to be reported when preferred behaviour would be >> to mark the tests as skipped. >> >> Unfortunately, the only way to determine if the soft-dirty dirty bit is >> supported is to write to a page, then see if the bit is set in >> /proc/self/pagemap. But the tests that we want to conditionally execute >> are testing precicesly this. So if we introduced this feature check, we >> could accedentally turn a real failure (on a system that claims to >> support soft-dirty) into a skip. > > ... > >> diff --git a/tools/testing/selftests/mm/soft-dirty.c >> b/tools/testing/selftests/mm/soft-dirty.c >> index cc5f144430d4..8a2cd161ec4d 100644 >> --- a/tools/testing/selftests/mm/soft-dirty.c >> +++ b/tools/testing/selftests/mm/soft-dirty.c > > Hi Ryan, > > Probably very similar to what David is requesting: given that arm64 > definitively does not support soft dirty, I'd suggest that we not even > *build* the soft dirty tests on arm64! > > There is no need to worry about counting, skipping or waiving such > tests, either. Because it's just a non-issue: one does not care about > test status for something that is documented as "this feature is simply > unavailable here". OK fair enough. I'll follow this approach for v2. Thanks for the review! > > > thanks,
diff --git a/tools/testing/selftests/mm/madv_populate.c b/tools/testing/selftests/mm/madv_populate.c index 60547245e479..5a8c176d7fec 100644 --- a/tools/testing/selftests/mm/madv_populate.c +++ b/tools/testing/selftests/mm/madv_populate.c @@ -232,6 +232,14 @@ static bool range_is_not_softdirty(char *start, ssize_t size) return ret; } +#define ksft_test_result_if_softdirty(cond, ...) \ +do { \ + if (system_has_softdirty()) \ + ksft_test_result(cond, __VA_ARGS__); \ + else \ + ksft_test_result_skip(__VA_ARGS__); \ +} while (0) + static void test_softdirty(void) { char *addr; @@ -246,19 +254,19 @@ static void test_softdirty(void) /* Clear any softdirty bits. */ clear_softdirty(); - ksft_test_result(range_is_not_softdirty(addr, SIZE), + ksft_test_result_if_softdirty(range_is_not_softdirty(addr, SIZE), "range is not softdirty\n"); /* Populating READ should set softdirty. */ ret = madvise(addr, SIZE, MADV_POPULATE_READ); - ksft_test_result(!ret, "MADV_POPULATE_READ\n"); - ksft_test_result(range_is_not_softdirty(addr, SIZE), + ksft_test_result_if_softdirty(!ret, "MADV_POPULATE_READ\n"); + ksft_test_result_if_softdirty(range_is_not_softdirty(addr, SIZE), "range is not softdirty\n"); /* Populating WRITE should set softdirty. */ ret = madvise(addr, SIZE, MADV_POPULATE_WRITE); - ksft_test_result(!ret, "MADV_POPULATE_WRITE\n"); - ksft_test_result(range_is_softdirty(addr, SIZE), + ksft_test_result_if_softdirty(!ret, "MADV_POPULATE_WRITE\n"); + ksft_test_result_if_softdirty(range_is_softdirty(addr, SIZE), "range is softdirty\n"); munmap(addr, SIZE); diff --git a/tools/testing/selftests/mm/soft-dirty.c b/tools/testing/selftests/mm/soft-dirty.c index cc5f144430d4..8a2cd161ec4d 100644 --- a/tools/testing/selftests/mm/soft-dirty.c +++ b/tools/testing/selftests/mm/soft-dirty.c @@ -192,6 +192,9 @@ int main(int argc, char **argv) int pagemap_fd; int pagesize; + if (!system_has_softdirty()) + return KSFT_SKIP; + ksft_print_header(); ksft_set_plan(15); diff --git a/tools/testing/selftests/mm/vm_util.c b/tools/testing/selftests/mm/vm_util.c index 558c9cd8901c..f014fafbd2d3 100644 --- a/tools/testing/selftests/mm/vm_util.c +++ b/tools/testing/selftests/mm/vm_util.c @@ -53,6 +53,23 @@ unsigned long pagemap_get_pfn(int fd, char *start) return -1ul; } +int system_has_softdirty(void) +{ + /* + * There is no way to check if the kernel supports soft-dirty, other + * than by writing to a page and seeing if the bit was set. But the + * tests are intended to check that the bit gets set when it should, so + * doing that check would turn a potentially legitimate fail into a + * skip. Fortunately, we know for sure that arm64 does not support + * soft-dirty. So for now, let's just use the arch as a corse guide. + */ +#if defined(__aarch64__) + return 0; +#else + return 1; +#endif +} + void clear_softdirty(void) { int ret; diff --git a/tools/testing/selftests/mm/vm_util.h b/tools/testing/selftests/mm/vm_util.h index c7fa61f0dff8..5a57314d428a 100644 --- a/tools/testing/selftests/mm/vm_util.h +++ b/tools/testing/selftests/mm/vm_util.h @@ -36,6 +36,7 @@ bool pagemap_is_softdirty(int fd, char *start); bool pagemap_is_swapped(int fd, char *start); bool pagemap_is_populated(int fd, char *start); unsigned long pagemap_get_pfn(int fd, char *start); +int system_has_softdirty(void); void clear_softdirty(void); bool check_for_pattern(FILE *fp, const char *pattern, char *buf, size_t len); uint64_t read_pmd_pagesize(void);
arm64 does not support the soft-dirty PTE bit. However there are tests in `madv_populate` and `soft-dirty` which assume it is supported and cause spurious failures to be reported when preferred behaviour would be to mark the tests as skipped. Unfortunately, the only way to determine if the soft-dirty dirty bit is supported is to write to a page, then see if the bit is set in /proc/self/pagemap. But the tests that we want to conditionally execute are testing precicesly this. So if we introduced this feature check, we could accedentally turn a real failure (on a system that claims to support soft-dirty) into a skip. So instead, do the check based on architecture; for arm64, we report that soft-dirty is not supported. This is wrapped up into a utility function `system_has_softdirty()`, which is used to skip the whole `soft-dirty` suite, and mark the soft-dirty tests in the `madv_populate` suite as skipped. Signed-off-by: Ryan Roberts <ryan.roberts@arm.com> --- tools/testing/selftests/mm/madv_populate.c | 18 +++++++++++++----- tools/testing/selftests/mm/soft-dirty.c | 3 +++ tools/testing/selftests/mm/vm_util.c | 17 +++++++++++++++++ tools/testing/selftests/mm/vm_util.h | 1 + 4 files changed, 34 insertions(+), 5 deletions(-) -- 2.25.1