[PATCH v2 RESEND 0/2] selftests/mm: reuse FORCE_READ to replace "asm volatile("" : "+r" (XXX));" and some cleanup

[PATCH v2 RESEND 0/2] selftests/mm: reuse FORCE_READ to replace "asm volatile("" : "+r" (XXX));" and some cleanup

[wang lian]

This series introduces a common FORCE_READ() macro to replace 
the cryptic asm volatile("" : "+r" (variable)); 
construct used in several mm selftests. This improves code readability and 
maintainability by removing duplicated, hard-to-understand code.

I previously sent the refactoring patch [1] as a standalone change, following a suggestion from David. 
As Andrew Morton and Wei Yang correctly pointed out, 
that patch was incomplete as it was missing the macro definition itself, 
causing build warnings. My apologies for the noise.

[1] https://lore.kernel.org/lkml/20250716123126.3851-1-lianux.mm@gmail.com/

This v2 series corrects that mistake by properly structuring the changes:
- The first patch introduces the FORCE_READ() macro into the shared vm_util.h header 
  and reuse this new macro to refactor the selftests.
- The second patch guard-regions: Use SKIP() instead of ksft_exit_skip().

changes to v2:
- collected Acked-by and Reviewed-by from  David and Lorenzo Stoakes.


wang lian (2):
  selftests/mm: reuse FORCE_READ to replace "asm volatile("" : "+r"
    (XXX));"
  selftests/mm: guard-regions: Use SKIP() instead of ksft_exit_skip()

 tools/testing/selftests/mm/cow.c              | 30 +++++++++----------
 tools/testing/selftests/mm/guard-regions.c    |  9 +-----
 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, 32 insertions(+), 40 deletions(-)
-- 
2.43.0

[PATCH 1/2] 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/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(-)
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/guard-regions.c b/tools/testing/selftests/mm/guard-regions.c
index 93af3d3760f9..4b76e72e7053  100644
--- a/tools/testing/selftests/mm/guard-regions.c
+++ b/tools/testing/selftests/mm/guard-regions.c
@@ -35,13 +35,6 @@
 static volatile sig_atomic_t signal_jump_set;
 static sigjmp_buf signal_jmp_buf;
 
-/*
- * 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)
-
 /*
  * How is the test backing the mapping being tested?
  */
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");
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)
+
 extern unsigned int __page_size;
 extern unsigned int __page_shift;
 
-- 
2.43.0

[PATCH 2/2] selftests/mm: guard-regions: Use SKIP() instead of ksft_exit_skip()

[wang lian]

To ensure only the current test is skipped on permission failure, instead
of terminating the entire test binary.

Signed-off-by: wang lian <lianux.mm@gmail.com>
Reviewed-by: Lorenzo Stoakes <lorenzo.stoakes@oracle.com>
Acked-by: David Hildenbrand <david@redhat.com>
---
 tools/testing/selftests/mm/guard-regions.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/testing/selftests/mm/guard-regions.c b/tools/testing/selftests/mm/guard-regions.c
index 4b76e72e7053..b0d42eb04e3a 100644
--- a/tools/testing/selftests/mm/guard-regions.c
+++ b/tools/testing/selftests/mm/guard-regions.c
@@ -575,7 +575,7 @@ TEST_F(guard_regions, process_madvise)
 
 	/* OK we don't have permission to do this, skip. */
 	if (count == -1 && errno == EPERM)
-		ksft_exit_skip("No process_madvise() permissions, try running as root.\n");
+		SKIP(return, "No process_madvise() permissions, try running as root.\n");
 
 	/* Returns the number of bytes advised. */
 	ASSERT_EQ(count, 6 * page_size);
-- 
2.43.0

Re: [PATCH 2/2] selftests/mm: guard-regions: Use SKIP() instead of ksft_exit_skip()

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 222 bytes --]

On Thu, Jul 17, 2025 at 09:18:57PM +0800, wang lian wrote:
> To ensure only the current test is skipped on permission failure, instead
> of terminating the entire test binary.

Reviewed-by: Mark Brown <broonie@kernel.org>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

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

[David Hildenbrand]

On 17.07.25 15: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>
> ---

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

-- 
Cheers,

David / dhildenb

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

[Wei Yang]

On Thu, Jul 17, 2025 at 09:18:56PM +0800, 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>

Reviewed-by: Wei Yang <richard.weiyang@gmail.com>

-- 
Wei Yang
Help you, Help me

Re: [PATCH 2/2] selftests/mm: guard-regions: Use SKIP() instead of ksft_exit_skip()

[Wei Yang]

On Thu, Jul 17, 2025 at 09:18:57PM +0800, wang lian wrote:
>To ensure only the current test is skipped on permission failure, instead
>of terminating the entire test binary.
>
>Signed-off-by: wang lian <lianux.mm@gmail.com>
>Reviewed-by: Lorenzo Stoakes <lorenzo.stoakes@oracle.com>
>Acked-by: David Hildenbrand <david@redhat.com>

Reviewed-by: Wei Yang <richard.weiyang@gmail.com>

-- 
Wei Yang
Help you, Help me

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

[Zi Yan]

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(-)

Reviewed-by: Zi Yan <ziy@nvidia.com>
Best Regards,
Yan, Zi

Re: [PATCH 2/2] selftests/mm: guard-regions: Use SKIP() instead of ksft_exit_skip()

[Zi Yan]

On 17 Jul 2025, at 9:18, wang lian wrote:

> To ensure only the current test is skipped on permission failure, instead
> of terminating the entire test binary.
>
> Signed-off-by: wang lian <lianux.mm@gmail.com>
> Reviewed-by: Lorenzo Stoakes <lorenzo.stoakes@oracle.com>
> Acked-by: David Hildenbrand <david@redhat.com>
> ---
>  tools/testing/selftests/mm/guard-regions.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>

Reviewed-by: Zi Yan <ziy@nvidia.com>

Best Regards,
Yan, Zi

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

[wang lian]

> On 17.07.25 15: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>
> > ---

Hi David,

Thank you for the review and the Acked-by tag. I appreciate it.

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

> -- 
> Cheers,

> David / dhildenb


Best regards,
Wang Lian

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

[wang lian]

> On Thu, Jul 17, 2025 at 09:18:56PM +0800, 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>

> Reviewed-by: Wei Yang <richard.weiyang@gmail.com>

Thanks!

> -- 
> Wei Yang
> Help you, Help me


Best regards,
Wang Lian

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

[Zi Yan]

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.

>
>  	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

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

[David Laight]

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
> 

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

[Zi Yan]

On 7 Aug 2025, at 8:16, David Laight wrote:

> 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]) ?

Yeah, that is my fix to this:
https://lore.kernel.org/linux-mm/20250805175140.241656-1-ziy@nvidia.com/


>
>>
>>>
>>>  	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
>>


--
Best Regards,
Yan, Zi