Re: [PATCH 1/2] selftests/mm: reuse FORCE_READ to replace "asm volatile("" : "+r" (XXX));"

[David Laight <david.laight.linux@gmail.com>]

On Tue, 05 Aug 2025 10:26:17 -0400
Zi Yan <ziy@nvidia.com> wrote:

> On 17 Jul 2025, at 9:18, wang lian wrote:
> 
> > Several mm selftests use the `asm volatile("" : "+r" (variable));`
> > construct to force a read of a variable, preventing the compiler from
> > optimizing away the memory access. This idiom is cryptic and duplicated
> > across multiple test files.
> >
> > Following a suggestion from David[1], this patch refactors this
> > common pattern into a FORCE_READ() macro
> >
> > [1] https://lore.kernel.org/lkml/4a3e0759-caa1-4cfa-bc3f-402593f1eee3@redhat.com/
> >
> > Signed-off-by: wang lian <lianux.mm@gmail.com>
> > Reviewed-by: Lorenzo Stoakes <lorenzo.stoakes@oracle.com>
> > ---
> >  tools/testing/selftests/mm/cow.c              | 30 +++++++++----------
> >  tools/testing/selftests/mm/guard-regions.c    |  7 -----
> >  tools/testing/selftests/mm/hugetlb-madvise.c  |  5 +---
> >  tools/testing/selftests/mm/migration.c        | 13 ++++----
> >  tools/testing/selftests/mm/pagemap_ioctl.c    |  4 +--
> >  .../selftests/mm/split_huge_page_test.c       |  4 +--
> >  tools/testing/selftests/mm/vm_util.h          |  7 +++++
> >  7 files changed, 31 insertions(+), 39 deletions(-)
> >  
> 
> <snip>
> 
> > diff --git a/tools/testing/selftests/mm/split_huge_page_test.c b/tools/testing/selftests/mm/split_huge_page_test.c
> > index f0d9c035641d..05de1fc0005b 100644
> > --- a/tools/testing/selftests/mm/split_huge_page_test.c
> > +++ b/tools/testing/selftests/mm/split_huge_page_test.c
> > @@ -399,7 +399,6 @@ int create_pagecache_thp_and_fd(const char *testfile, size_t fd_size, int *fd,
> >  		char **addr)
> >  {
> >  	size_t i;
> > -	int dummy = 0;
> >  	unsigned char buf[1024];
> >
> >  	srand(time(NULL));
> > @@ -441,8 +440,7 @@ int create_pagecache_thp_and_fd(const char *testfile, size_t fd_size, int *fd,
> >  	madvise(*addr, fd_size, MADV_HUGEPAGE);
> >
> >  	for (size_t i = 0; i < fd_size; i++)
> > -		dummy += *(*addr + i);
> > -	asm volatile("" : "+r" (dummy));
> > +		FORCE_READ((*addr + i));  
> 
> I encountered a segfault when running the test on x86_64.
> i is 4194297 and fd_size is 4194304.
> It seems that FORCE_READ() is reading (*addr + i) in 8 byte size
> and i is only 7 bytes away from the end of the memory address.
> This led to segfault.
> 
> (*(volatile char*)(*addr + i)); works fine.
> 
> Both gcc-12 and gcc-14 have the issue.

The definition of FORCE_READ in 6.16 is:
#define FORCE_READ(x) (*(volatile typeof(x) *)x)
this is clearly bogus.
'x' is a pointer - follow it through.
Possibly:
#define FORCE_READ(x) (*(volatile typeof(*(x)) *)(x))
is better,
But why not use READ_ONCE(*addr[i]) ?

	David

> 
> >
> >  	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");
> > diff --git a/tools/testing/selftests/mm/vm_util.h b/tools/testing/selftests/mm/vm_util.h
> > index 2b154c287591..c20298ae98ea  100644
> > --- a/tools/testing/selftests/mm/vm_util.h
> > +++ b/tools/testing/selftests/mm/vm_util.h
> > @@ -18,6 +18,13 @@
> >  #define PM_SWAP                       BIT_ULL(62)
> >  #define PM_PRESENT                    BIT_ULL(63)
> >
> > +/*
> > + * Ignore the checkpatch warning, we must read from x but don't want to do
> > + * anything with it in order to trigger a read page fault. We therefore must use
> > + * volatile to stop the compiler from optimising this away.
> > + */
> > +#define FORCE_READ(x) (*(volatile typeof(x) *)x)
> > +  
> 
> Also, look at FORCE_READ again, it converts x to a pointer to x and
> deferences x as a point. It does not seem right to me.
> 
> Best Regards,
> Yan, Zi
>