Re: [PATCH v1 3/9] selftests/mm: Skip soft-dirty tests on arm64

[Ryan Roberts]

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@xxxxxxx>
>>>>> ---
>>>>>     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. ;-)


>