Re: [PATCH] selftest/vm: Use correct PAGE_SHIFT value for ppc64

[Shuah Khan <skhan@linuxfoundation.org>]

On 2/10/22 8:03 AM, Aneesh Kumar K V wrote:
> On 2/10/22 20:09, Shuah Khan wrote:
>> On 2/9/22 9:12 PM, Aneesh Kumar K.V wrote:
>>> Shuah Khan <skhan@linuxfoundation.org> writes:
>>>
>>>> On 2/9/22 8:43 AM, Aneesh Kumar K.V wrote:
>>>>> Keep it simple by using a #define and limiting hugepage size to 2M.
>>>>> This keeps the test simpler instead of dynamically finding the page size
>>>>> and huge page size.
>>>>>
>>>>> Without this tests are broken w.r.t reading /proc/self/pagemap
>>>>>
>>>>>     if (pread(pagemap_fd, ent, sizeof(ent),
>>>>>             (uintptr_t)ptr >> (PAGE_SHIFT - 3)) != sizeof(ent))
>>>>>         err(2, "read pagemap");
>>>>>
>>>>> Cc: Shuah Khan <shuah@kernel.org>
>>>>> Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@linux.ibm.com>
>>>>> ---
>>>>>    tools/testing/selftests/vm/ksm_tests.c        | 8 ++++++++
>>>>>    tools/testing/selftests/vm/transhuge-stress.c | 8 ++++++++
>>>>>    2 files changed, 16 insertions(+)
>>>>>
>>>>> diff --git a/tools/testing/selftests/vm/ksm_tests.c b/tools/testing/selftests/vm/ksm_tests.c
>>>>> index 1436e1a9a3d3..8200328ff018 100644
>>>>> --- a/tools/testing/selftests/vm/ksm_tests.c
>>>>> +++ b/tools/testing/selftests/vm/ksm_tests.c
>>>>> @@ -22,8 +22,16 @@
>>>>>    #define KSM_MERGE_ACROSS_NODES_DEFAULT true
>>>>>    #define MB (1ul << 20)
>>>>> +#ifdef __powerpc64__
>>>>> +#define PAGE_SHIFT    16
>>>>> +/*
>>>>> + * This will only work with radix 2M hugepage size
>>>>> + */
>>>>> +#define HPAGE_SHIFT 21
>>>>> +#else
>>>>>    #define PAGE_SHIFT 12
>>>>>    #define HPAGE_SHIFT 21
>>>>> +#endif
>>>>>    #define PAGE_SIZE (1 << PAGE_SHIFT)
>>>>>    #define HPAGE_SIZE (1 << HPAGE_SHIFT)
>>>>> diff --git a/tools/testing/selftests/vm/transhuge-stress.c b/tools/testing/selftests/vm/transhuge-stress.c
>>>>> index 5e4c036f6ad3..f04c8aa4bcf6 100644
>>>>> --- a/tools/testing/selftests/vm/transhuge-stress.c
>>>>> +++ b/tools/testing/selftests/vm/transhuge-stress.c
>>>>> @@ -16,8 +16,16 @@
>>>>>    #include <string.h>
>>>>>    #include <sys/mman.h>
>>>>> +#ifdef __powerpc64__
>>>>> +#define PAGE_SHIFT    16
>>>>> +/*
>>>>> + * This will only work with radix 2M hugepage size
>>>>> + */
>>>>> +#define HPAGE_SHIFT 21
>>>>
>>>> Why not have this is in common code?
>>>
>>> Can you suggest where I can move that. We also have helper functions
>>> like allocate_transhuge() duplicated between tests. I didn't find
>>> libutil.a or anything similar supported by the selftets build.
>>>
>>>>
>>
>> I noticed that HPAGE_SHIFT is defined in #ifdef __powerpc64__ block
>> as well as #else. I am asking is it necessary to be part of both
>> blocks.
>>
>> +#ifdef __powerpc64__
>> +#define PAGE_SHIFT    16
>> +/*
>> + * This will only work with radix 2M hugepage size
>> + */
>> +#define HPAGE_SHIFT 21  --- this one
>> +#else
>>    #define PAGE_SHIFT 12
>>    #define HPAGE_SHIFT 21   --- this one
>> +#endif
>>
> 
> 
> The reason I did that was to add the comment which is relevant only for ppc64. ppc64 supports two hugepage sizes, 2M and 16M. The test won't work correctly with 16M hugepage size. We do have other tests in selftest/vm/ with similar restrictions.
> 
> 

Right. You don't have to duplicate code for the comment. You can add the
comment and then clarify in the comment that it is only relevant to ppc64.

This way the comment is clear and we can avoid duplicate code that makes it
hard to maintain in the future.

thanks,
-- Shuah