Received-SPF: pass (google.com: domain of linux-kernel+bounces-204984-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
Date: Thu, 6 Jun 2024 20:30:53 +0000
From: "Pankaj Raghav (Samsung)" <kernel@pankajraghav.com>
To: David Hildenbrand <david@redhat.com>
Cc: Shuah Khan <shuah@kernel.org>,
	Andrew Morton <akpm@linux-foundation.org>, willy@infradead.org,
	linux-kernel@vger.kernel.org, linux-mm@kvack.org,
	gost.dev@samsung.com, mcgrof@kernel.org,
	linux-kselftest@vger.kernel.org, Zi Yan <zi.yan@sent.com>,
	Pankaj Raghav <p.raghav@samsung.com>
Subject: Re: [PATCH] selftests/mm: use volatile keyword to not optimize mmap
 read variable
Message-ID: <20240606203053.y5zxa54ndv53d3qj@quentin>
References: <20240606135835.600022-1-kernel@pankajraghav.com>
 <5b4e7ef2-3ced-4d4a-989c-e99b06598d32@redhat.com>
 <f199d120-2347-4bc0-8940-155c3c465de9@redhat.com>
 <20240606201831.ywmawi7xl6jgj3p4@quentin>
 <b50bc8dd-bb1c-4859-83b9-ab90559452e9@redhat.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <b50bc8dd-bb1c-4859-83b9-ab90559452e9@redhat.com>

On Thu, Jun 06, 2024 at 10:21:51PM +0200, David Hildenbrand wrote:
> 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.

Yup! I will send a new version. Thanks David.
> 
> -- 
> Cheers,
> 
> David / dhildenb
>