selftests/mm: refactor common code and improve test skipping in guard_region

[PATCH] selftests/mm: refactor common code and improve test skipping in guard_region

Posted by wang lian 2 months, 3 weeks ago

Move the generic `FORCE_READ` macro from `guard-regions.c` to the shared
`vm_util.h` header to promote code reuse.

In `guard-regions.c`, replace `ksft_exit_skip()` with the `SKIP()` macro
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>
---
 tools/testing/selftests/mm/guard-regions.c | 9 +--------
 tools/testing/selftests/mm/vm_util.h       | 7 +++++++
 2 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/tools/testing/selftests/mm/guard-regions.c b/tools/testing/selftests/mm/guard-regions.c
index 93af3d3760f9..b0d42eb04e3a 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?
  */
@@ -582,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);
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

Re: [PATCH] selftests/mm: refactor common code and improve test skipping in guard_region

Posted by Lorenzo Stoakes 2 months, 3 weeks ago

On Mon, Jul 14, 2025 at 09:00:09PM +0800, wang lian wrote:
> Move the generic `FORCE_READ` macro from `guard-regions.c` to the shared
> `vm_util.h` header to promote code reuse.
>
> In `guard-regions.c`, replace `ksft_exit_skip()` with the `SKIP()` macro
> 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>

LGTM, so:

Reviewed-by: Lorenzo Stoakes <lorenzo.stoakes@oracle.com>

> ---
>  tools/testing/selftests/mm/guard-regions.c | 9 +--------
>  tools/testing/selftests/mm/vm_util.h       | 7 +++++++
>  2 files changed, 8 insertions(+), 8 deletions(-)
>
> diff --git a/tools/testing/selftests/mm/guard-regions.c b/tools/testing/selftests/mm/guard-regions.c
> index 93af3d3760f9..b0d42eb04e3a 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?
>   */
> @@ -582,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");

Lol oops! :P yes thi is better.

>
>  	/* Returns the number of bytes advised. */
>  	ASSERT_EQ(count, 6 * page_size);
> 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)
> +

Cool makes sense to share this.

If this contradicts previous review, take this review to be the correct one
:P

>  extern unsigned int __page_size;
>  extern unsigned int __page_shift;
>
> --
> 2.43.0
>

Re: [PATCH] selftests/mm: refactor common code and improve test skipping in guard_region

Posted by Mark Brown 2 months, 3 weeks ago

On Mon, Jul 14, 2025 at 09:00:09PM +0800, wang lian wrote:

> Move the generic `FORCE_READ` macro from `guard-regions.c` to the shared
> `vm_util.h` header to promote code reuse.
> 
> In `guard-regions.c`, replace `ksft_exit_skip()` with the `SKIP()` macro
> to ensure only the current test is skipped on permission failure, instead
> of terminating the entire test binary.

These two changes look fine but they're not really related so should be
separate patches.  Looking briefly at guard-regions.c I see a bunch more
use of ksft_exit_ functions that ought to be fixed as well, but your fix
is good.

Re: [PATCH] selftests/mm: refactor common code and improve test skipping in guard_region

Posted by David Hildenbrand 2 months, 3 weeks ago

On 14.07.25 15:39, Mark Brown wrote:
> On Mon, Jul 14, 2025 at 09:00:09PM +0800, wang lian wrote:
> 
>> Move the generic `FORCE_READ` macro from `guard-regions.c` to the shared
>> `vm_util.h` header to promote code reuse.
>>
>> In `guard-regions.c`, replace `ksft_exit_skip()` with the `SKIP()` macro
>> to ensure only the current test is skipped on permission failure, instead
>> of terminating the entire test binary.
> 
> These two changes look fine but they're not really related so should be
> separate patches.  Looking briefly at guard-regions.c I see a bunch more
> use of ksft_exit_ functions that ought to be fixed as well, but your fix
> is good.

The FORCE_READ() could be factored out separately, and as part of the 
same patch, replace the "asm volatile("" : "+r" (XXX));" usage in

* cow.c
* hugetlb-madvise.c
* migration.c
* pagemap_ioctl.c
* split_huge_page_test.c

-- 
Cheers,

David / dhildenb

Re: [PATCH] selftests/mm: refactor common code and improve test skipping in guard_region

Posted by Lorenzo Stoakes 2 months, 3 weeks ago

On Mon, Jul 14, 2025 at 03:44:28PM +0200, David Hildenbrand wrote:
> On 14.07.25 15:39, Mark Brown wrote:
> > On Mon, Jul 14, 2025 at 09:00:09PM +0800, wang lian wrote:
> >
> > > Move the generic `FORCE_READ` macro from `guard-regions.c` to the shared
> > > `vm_util.h` header to promote code reuse.
> > >
> > > In `guard-regions.c`, replace `ksft_exit_skip()` with the `SKIP()` macro
> > > to ensure only the current test is skipped on permission failure, instead
> > > of terminating the entire test binary.
> >
> > These two changes look fine but they're not really related so should be
> > separate patches.  Looking briefly at guard-regions.c I see a bunch more
> > use of ksft_exit_ functions that ought to be fixed as well, but your fix
> > is good.
>
> The FORCE_READ() could be factored out separately, and as part of the same
> patch, replace the "asm volatile("" : "+r" (XXX));" usage in
>
> * cow.c
> * hugetlb-madvise.c
> * migration.c
> * pagemap_ioctl.c
> * split_huge_page_test.c

Wang - Feel free to put my Reviewed-by in any such series assuming you do a
straight up replace.

>
> --
> Cheers,
>
> David / dhildenb
>

Cheers, Lorenzo

Re: [PATCH] selftests/mm: refactor common code and improve test skipping in guard_region

Posted by wang lian 2 months, 3 weeks ago

> On Mon, Jul 14, 2025 at 03:44:28PM +0200, David Hildenbrand wrote:
> > On 14.07.25 15:39, Mark Brown wrote:
> > > On Mon, Jul 14, 2025 at 09:00:09PM +0800, wang lian wrote:
> > >
> > > > Move the generic `FORCE_READ` macro from `guard-regions.c` to the shared
> > > > `vm_util.h` header to promote code reuse.
> > > >
> > > > In `guard-regions.c`, replace `ksft_exit_skip()` with the `SKIP()` macro
> > > > to ensure only the current test is skipped on permission failure, instead
> > > > of terminating the entire test binary.
> > >
> > > These two changes look fine but they're not really related so should be
> > > separate patches.  Looking briefly at guard-regions.c I see a bunch more
> > > use of ksft_exit_ functions that ought to be fixed as well, but your fix
> > > is good.
> >
> > The FORCE_READ() could be factored out separately, and as part of the same
> > patch, replace the "asm volatile("" : "+r" (XXX));" usage in
> >
> > * cow.c
> > * hugetlb-madvise.c
> > * migration.c
> > * pagemap_ioctl.c
> > * split_huge_page_test.c

> Wang - Feel free to put my Reviewed-by in any such series assuming you do a
> straight up replace.

> >
> > --
> > Cheers,
> >
> > David / dhildenb
> > 

> Cheers, Lorenzo

Thanks all for the helpful feedback and encouragement!

I'll prepare  include the straightforward replacements in the other tests as suggested.


Thanks again!

Best regards,
wang lian