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