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

[Pankaj Raghav (Samsung)]

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@xxxxxxxxxxx>
> > > 
> > > 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@xxxxxxxxxxx>
> > > ---
> > >    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.

Let me know what others think.


[1] https://lore.kernel.org/linux-mm/20240606154428.672643-1-kernel@xxxxxxxxxxxxxxxx/