Re: [PATCH] selftests/mm: use volatile keyword to not optimize mmap read variable

[David Hildenbrand <david@redhat.com>]

On 06.06.24 22:18, Pankaj Raghav (Samsung) wrote:
> On Thu, Jun 06, 2024 at 05:57:21PM +0200, David Hildenbrand wrote:
>> On 06.06.24 17:56, David Hildenbrand wrote:
>>> On 06.06.24 15:58, Pankaj Raghav (Samsung) wrote:
>>>> From: Pankaj Raghav <p.raghav@samsung.com>
>>>>
>>>> create_pagecache_thp_and_fd() in split_huge_page_test.c used the
>>>> variable dummy to perform mmap read.
>>>>
>>>> However, this test was skipped even on XFS which has large folio
>>>> support. The issue was compiler (gcc 13.2.0) was optimizing out the
>>>> dummy variable, therefore, not creating huge page in the page cache.
>>>>
>>>> Add volatile keyword to force compiler not to optimize out the loop
>>>> where we read from the mmaped addr.
>>>>
>>>> Signed-off-by: Pankaj Raghav <p.raghav@samsung.com>
>>>> ---
>>>>     tools/testing/selftests/mm/split_huge_page_test.c | 2 +-
>>>>     1 file changed, 1 insertion(+), 1 deletion(-)
>>>>
>>>> diff --git a/tools/testing/selftests/mm/split_huge_page_test.c b/tools/testing/selftests/mm/split_huge_page_test.c
>>>> index d3c7f5fb3e7b..c573a58f80ab 100644
>>>> --- a/tools/testing/selftests/mm/split_huge_page_test.c
>>>> +++ b/tools/testing/selftests/mm/split_huge_page_test.c
>>>> @@ -300,7 +300,7 @@ int create_pagecache_thp_and_fd(const char *testfile, size_t fd_size, int *fd,
>>>>     		char **addr)
>>>>     {
>>>>     	size_t i;
>>>> -	int __attribute__((unused)) dummy = 0;
>>>> +	volatile int __attribute__((unused)) dummy = 0;
>>>>     	srand(time(NULL));
>>>>
>>>> base-commit: d97496ca23a2d4ee80b7302849404859d9058bcd
>>>
>>> The rick we do in some other tests is:
>>>
>>> char *tmp;
>>>
>>> tmp = *whatever;
>>> asm volatile("" : "+r" (tmp));
>>
>> char tmp; of course. See cow.c as an example.
> Thanks David! I remember also seeing this when I grepped for volatile in
> the selftests directory.
> 
> Willy gave the idea of making it as a global variable [1]. But your
> trick also works :)
> 
> diff --git a/tools/testing/selftests/mm/split_huge_page_test.c b/tools/testing/selftests/mm/split_huge_page_test.c
> index d3c7f5fb3e7b..9c957703c1f7 100644
> --- a/tools/testing/selftests/mm/split_huge_page_test.c
> +++ b/tools/testing/selftests/mm/split_huge_page_test.c
> @@ -341,6 +341,7 @@ int create_pagecache_thp_and_fd(const char *testfile, size_t fd_size, int *fd,
>   
>          for (size_t i = 0; i < fd_size; i++)
>                  dummy += *(*addr + i);
> +       asm volatile("" : "+r" (dummy));
>   
>          if (!check_huge_file(*addr, fd_size / pmd_pagesize, pmd_pagesize)) {
>                  ksft_print_msg("No large pagecache folio generated, please provide a filesystem supporting large folio\n");
> 
> I am fine with either solutions. But using the trick asm volatile is more
> cleaner than making it a global variable IMO and makes it more uniform
> across the other mm tests.

You can then even the "__attribute__((unused))", because the compile 
must assume that it is used inside the asm statement.

-- 
Cheers,

David / dhildenb