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

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

[wang lian]

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/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 +--
 5 files changed, 24 insertions(+), 32 deletions(-)

diff --git a/tools/testing/selftests/mm/cow.c b/tools/testing/selftests/mm/cow.c
index 788e82b00b75..d30625c18259 100644
--- a/tools/testing/selftests/mm/cow.c
+++ b/tools/testing/selftests/mm/cow.c
@@ -1534,7 +1534,7 @@ static void test_ro_fast_pin(char *mem, const char *smem, size_t size)
 
 static void run_with_zeropage(non_anon_test_fn fn, const char *desc)
 {
-	char *mem, *smem, tmp;
+	char *mem, *smem;
 
 	log_test_start("%s ... with shared zeropage", desc);
 
@@ -1554,8 +1554,8 @@ static void run_with_zeropage(non_anon_test_fn fn, const char *desc)
 	}
 
 	/* Read from the page to populate the shared zeropage. */
-	tmp = *mem + *smem;
-	asm volatile("" : "+r" (tmp));
+	FORCE_READ(mem);
+	FORCE_READ(smem);
 
 	fn(mem, smem, pagesize);
 munmap:
@@ -1566,7 +1566,7 @@ static void run_with_zeropage(non_anon_test_fn fn, const char *desc)
 
 static void run_with_huge_zeropage(non_anon_test_fn fn, const char *desc)
 {
-	char *mem, *smem, *mmap_mem, *mmap_smem, tmp;
+	char *mem, *smem, *mmap_mem, *mmap_smem;
 	size_t mmap_size;
 	int ret;
 
@@ -1617,8 +1617,8 @@ static void run_with_huge_zeropage(non_anon_test_fn fn, const char *desc)
 	 * the first sub-page and test if we get another sub-page populated
 	 * automatically.
 	 */
-	tmp = *mem + *smem;
-	asm volatile("" : "+r" (tmp));
+	FORCE_READ(mem);
+	FORCE_READ(smem);
 	if (!pagemap_is_populated(pagemap_fd, mem + pagesize) ||
 	    !pagemap_is_populated(pagemap_fd, smem + pagesize)) {
 		ksft_test_result_skip("Did not get THPs populated\n");
@@ -1634,7 +1634,7 @@ static void run_with_huge_zeropage(non_anon_test_fn fn, const char *desc)
 
 static void run_with_memfd(non_anon_test_fn fn, const char *desc)
 {
-	char *mem, *smem, tmp;
+	char *mem, *smem;
 	int fd;
 
 	log_test_start("%s ... with memfd", desc);
@@ -1668,8 +1668,8 @@ static void run_with_memfd(non_anon_test_fn fn, const char *desc)
 	}
 
 	/* Fault the page in. */
-	tmp = *mem + *smem;
-	asm volatile("" : "+r" (tmp));
+	FORCE_READ(mem);
+	FORCE_READ(smem);
 
 	fn(mem, smem, pagesize);
 munmap:
@@ -1682,7 +1682,7 @@ static void run_with_memfd(non_anon_test_fn fn, const char *desc)
 
 static void run_with_tmpfile(non_anon_test_fn fn, const char *desc)
 {
-	char *mem, *smem, tmp;
+	char *mem, *smem;
 	FILE *file;
 	int fd;
 
@@ -1724,8 +1724,8 @@ static void run_with_tmpfile(non_anon_test_fn fn, const char *desc)
 	}
 
 	/* Fault the page in. */
-	tmp = *mem + *smem;
-	asm volatile("" : "+r" (tmp));
+	FORCE_READ(mem);
+	FORCE_READ(smem);
 
 	fn(mem, smem, pagesize);
 munmap:
@@ -1740,7 +1740,7 @@ static void run_with_memfd_hugetlb(non_anon_test_fn fn, const char *desc,
 				   size_t hugetlbsize)
 {
 	int flags = MFD_HUGETLB;
-	char *mem, *smem, tmp;
+	char *mem, *smem;
 	int fd;
 
 	log_test_start("%s ... with memfd hugetlb (%zu kB)", desc,
@@ -1778,8 +1778,8 @@ static void run_with_memfd_hugetlb(non_anon_test_fn fn, const char *desc,
 	}
 
 	/* Fault the page in. */
-	tmp = *mem + *smem;
-	asm volatile("" : "+r" (tmp));
+	FORCE_READ(mem);
+	FORCE_READ(smem);
 
 	fn(mem, smem, hugetlbsize);
 munmap:
diff --git a/tools/testing/selftests/mm/hugetlb-madvise.c b/tools/testing/selftests/mm/hugetlb-madvise.c
index e74107185324..1afe14b9dc0c 100644
--- a/tools/testing/selftests/mm/hugetlb-madvise.c
+++ b/tools/testing/selftests/mm/hugetlb-madvise.c
@@ -47,14 +47,11 @@ void write_fault_pages(void *addr, unsigned long nr_pages)
 
 void read_fault_pages(void *addr, unsigned long nr_pages)
 {
-	volatile unsigned long dummy = 0;
 	unsigned long i;
 
 	for (i = 0; i < nr_pages; i++) {
-		dummy += *((unsigned long *)(addr + (i * huge_page_size)));
-
 		/* Prevent the compiler from optimizing out the entire loop: */
-		asm volatile("" : "+r" (dummy));
+		FORCE_READ(((unsigned long *)(addr + (i * huge_page_size))));
 	}
 }
 
diff --git a/tools/testing/selftests/mm/migration.c b/tools/testing/selftests/mm/migration.c
index a306f8bab087..c5a73617796a 100644
--- a/tools/testing/selftests/mm/migration.c
+++ b/tools/testing/selftests/mm/migration.c
@@ -16,6 +16,7 @@
 #include <sys/types.h>
 #include <signal.h>
 #include <time.h>
+#include "vm_util.h"
 
 #define TWOMEG		(2<<20)
 #define RUNTIME		(20)
@@ -103,15 +104,13 @@ int migrate(uint64_t *ptr, int n1, int n2)
 
 void *access_mem(void *ptr)
 {
-	volatile uint64_t y = 0;
-	volatile uint64_t *x = ptr;
-
 	while (1) {
 		pthread_testcancel();
-		y += *x;
-
-		/* Prevent the compiler from optimizing out the writes to y: */
-		asm volatile("" : "+r" (y));
+		/* Force a read from the memory pointed to by ptr. This ensures
+		 * the memory access actually happens and prevents the compiler
+		 * from optimizing away this entire loop.
+		 */
+		FORCE_READ((uint64_t *)ptr);
 	}
 
 	return NULL;
diff --git a/tools/testing/selftests/mm/pagemap_ioctl.c b/tools/testing/selftests/mm/pagemap_ioctl.c
index c2dcda78ad31..0d4209eef0c3 100644
--- a/tools/testing/selftests/mm/pagemap_ioctl.c
+++ b/tools/testing/selftests/mm/pagemap_ioctl.c
@@ -1525,9 +1525,7 @@ void zeropfn_tests(void)
 
 	ret = madvise(mem, hpage_size, MADV_HUGEPAGE);
 	if (!ret) {
-		char tmp = *mem;
-
-		asm volatile("" : "+r" (tmp));
+		FORCE_READ(mem);
 
 		ret = pagemap_ioctl(mem, hpage_size, &vec, 1, 0,
 				    0, PAGE_IS_PFNZERO, 0, 0, PAGE_IS_PFNZERO);
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));
 
 	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");
-- 
2.43.0

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

[David Hildenbrand]

On 16.07.25 14:31, 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>

Acked-by: David Hildenbrand <david@redhat.com>

-- 
Cheers,

David / dhildenb

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

[Andrew Morton]

On Wed, 16 Jul 2025 20:31:26 +0800 wang lian <lianux.mm@gmail.com> 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
> 
>  tools/testing/selftests/mm/cow.c              | 30 +++++++++----------
>  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 +--
>  5 files changed, 24 insertions(+), 32 deletions(-)

The patch forgot to move the FORCE_READ definition into a header?

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

[Wei Yang]

On Wed, Jul 16, 2025 at 03:15:43PM -0700, Andrew Morton wrote:
>On Wed, 16 Jul 2025 20:31:26 +0800 wang lian <lianux.mm@gmail.com> 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
>> 
>>  tools/testing/selftests/mm/cow.c              | 30 +++++++++----------
>>  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 +--
>>  5 files changed, 24 insertions(+), 32 deletions(-)
>
>The patch forgot to move the FORCE_READ definition into a header?
>

I get this after applying the patch.

cow.c:1559:9: warning: implicit declaration of function ‘FORCE_READ’; did you mean ‘LOCK_READ’? [-Wimplicit-function-declaration]
 1559 |         FORCE_READ(mem);

-- 
Wei Yang
Help you, Help me

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

[wang lian]

> On Wed, 16 Jul 2025 20:31:26 +0800 wang lian <lianux.mm@gmail.com> 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
> > 
> >  tools/testing/selftests/mm/cow.c              | 30 +++++++++----------
> >  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 +--
> >  5 files changed, 24 insertions(+), 32 deletions(-)

> The patch forgot to move the FORCE_READ definition into a header?

Hi Andrew,
You are absolutely right. My apologies for the inconvenience.
This patch was sent standalone based on a suggestion from David during 
the discussion of a previous, larger patch series. In that original series, 
I had already moved the FORCE_READ() macro definition into vm_util.h.

You can find the original patch series and discussion at this link:
https://lore.kernel.org/lkml/20250714130009.14581-1-lianux.mm@gmail.com/
It should also be in your mailing list archive.

To make this easier to review and apply, I can send a new, small patch series 
that first introduces the FORCE_READ() macro in vm_util.h and then applies this refactoring. 
Please let me know if you'd prefer that.
Sorry again for the confusion.


Best regards,
wang lian

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

[David Hildenbrand]

On 17.07.25 12:48, wang lian wrote:
>> On Wed, 16 Jul 2025 20:31:26 +0800 wang lian <lianux.mm@gmail.com> 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
>>>
>>>   tools/testing/selftests/mm/cow.c              | 30 +++++++++----------
>>>   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 +--
>>>   5 files changed, 24 insertions(+), 32 deletions(-)
> 
>> The patch forgot to move the FORCE_READ definition into a header?
> 
> Hi Andrew,
> You are absolutely right. My apologies for the inconvenience.
> This patch was sent standalone based on a suggestion from David during
> the discussion of a previous, larger patch series. In that original series,
> I had already moved the FORCE_READ() macro definition into vm_util.h.
> 
> You can find the original patch series and discussion at this link:
> https://lore.kernel.org/lkml/20250714130009.14581-1-lianux.mm@gmail.com/
> It should also be in your mailing list archive.
> 
> To make this easier to review and apply, I can send a new, small patch series
> that first introduces the FORCE_READ() macro in vm_util.h and then applies this refactoring.

Please simply perform the move of FORCE_READ() in this very patch where 
you also use it elswehere.

I missed that when skimming over this patch.

-- 
Cheers,

David / dhildenb

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

[David Laight]

On Thu, 17 Jul 2025 13:43:45 +0200
David Hildenbrand <david@redhat.com> wrote:

> On 17.07.25 12:48, wang lian wrote:
> >> On Wed, 16 Jul 2025 20:31:26 +0800 wang lian <lianux.mm@gmail.com> 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
> >>>
> >>>   tools/testing/selftests/mm/cow.c              | 30 +++++++++----------
> >>>   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 +--
> >>>   5 files changed, 24 insertions(+), 32 deletions(-)  
> >   
> >> The patch forgot to move the FORCE_READ definition into a header?  
> > 
> > Hi Andrew,
> > You are absolutely right. My apologies for the inconvenience.
> > This patch was sent standalone based on a suggestion from David during
> > the discussion of a previous, larger patch series. In that original series,
> > I had already moved the FORCE_READ() macro definition into vm_util.h.
> > 
> > You can find the original patch series and discussion at this link:
> > https://lore.kernel.org/lkml/20250714130009.14581-1-lianux.mm@gmail.com/
> > It should also be in your mailing list archive.
> > 
> > To make this easier to review and apply, I can send a new, small patch series
> > that first introduces the FORCE_READ() macro in vm_util.h and then applies this refactoring.  
> 
> Please simply perform the move of FORCE_READ() in this very patch where 
> you also use it elswehere.

Why not use READ_ONCE() instead (or even just make all the variables 'volatile char *').
I had to look up the definition to find the hidden indirection in FORCE_READ().

It has to be said that now writes to variables that are READ_ONCE() have to be
WRITE_ONCE() why not just make the variables 'volatile'.
That will stop things bleating about missing READ/WRITE_ONCE() wrappers.
There was a rational for not using volatile, but it is getting to be moot.

	David


> 
> I missed that when skimming over this patch.
> 

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

[Lorenzo Stoakes]

On Sat, Jul 19, 2025 at 10:27:38AM +0100, David Laight wrote:
> On Thu, 17 Jul 2025 13:43:45 +0200
> David Hildenbrand <david@redhat.com> wrote:
>
> > On 17.07.25 12:48, wang lian wrote:
> > >> On Wed, 16 Jul 2025 20:31:26 +0800 wang lian <lianux.mm@gmail.com> 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
> > >>>
> > >>>   tools/testing/selftests/mm/cow.c              | 30 +++++++++----------
> > >>>   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 +--
> > >>>   5 files changed, 24 insertions(+), 32 deletions(-)
> > >
> > >> The patch forgot to move the FORCE_READ definition into a header?
> > >
> > > Hi Andrew,
> > > You are absolutely right. My apologies for the inconvenience.
> > > This patch was sent standalone based on a suggestion from David during
> > > the discussion of a previous, larger patch series. In that original series,
> > > I had already moved the FORCE_READ() macro definition into vm_util.h.
> > >
> > > You can find the original patch series and discussion at this link:
> > > https://lore.kernel.org/lkml/20250714130009.14581-1-lianux.mm@gmail.com/
> > > It should also be in your mailing list archive.
> > >
> > > To make this easier to review and apply, I can send a new, small patch series
> > > that first introduces the FORCE_READ() macro in vm_util.h and then applies this refactoring.
> >
> > Please simply perform the move of FORCE_READ() in this very patch where
> > you also use it elswehere.
>
> Why not use READ_ONCE() instead (or even just make all the variables 'volatile char *').
> I had to look up the definition to find the hidden indirection in FORCE_READ().

Honestly this is an incredible level of nitpicking for a _self test_
patch.

I don't think you need to look up definitions to understand that volatile
prevents the compiler from optimising out a read like this. And what
exactly is hidden? We cast to the volatile type of the pointer, then deref
it in a fashion that cannot be optimised out?

But I mean, maybe I'm missing some complexity here? I am always happy to be
corrected :)

The point is to read fault a page for testing purposes.

This is fine, works, and it's _test code_.

Overall though, this discussion is not helpful and this is a moot point,
sorry.

>
> It has to be said that now writes to variables that are READ_ONCE() have to be
> WRITE_ONCE() why not just make the variables 'volatile'.
> That will stop things bleating about missing READ/WRITE_ONCE() wrappers.
> There was a rational for not using volatile, but it is getting to be moot.

I'm struggling to understand what on earth you're talking about here, what
would bleat about self test code missing READ/WRITE_ONCE() wrappers?

And you're suggesting going through and changing all pointers to be
volatile... because why? What? That'd be awful and very very silly.

Note that checkpatch.pl _will_ bleat about this.

TL;DR: No, absolutely not.

Wang - do not do anything like this, please!

>
> 	David
>
>
> >
> > I missed that when skimming over this patch.
> >
>

Let's all have some empathy for this being one of Wang's first patches. I
appreciate this patch and it's a strict improvement on the past situation
AFAIC.

Cheers, Lorenzo

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

[wang lian]

> Wang - do not do anything like this, please!


> Let's all have some empathy for this being one of Wang's first patches. I
> appreciate this patch and it's a strict improvement on the past situation
> AFAIC.

> Cheers, Lorenzo

Hi Lorenzo,
Thank you so much for your kind words and support. I truly appreciate 
you stepping in to provide clarity and encouragement.

This discussion has been a great learning experience. It's support like yours 
that makes me feel so welcome and passionate about contributing to the kernel.

Please rest assured, I will follow the correct guidance. I am working on the 
updated process_madv patch now.

Thank you again for everything.
Best regards,
Wang Lian