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;
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.
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
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
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
linux-kselftest-mirror@lists.linaro.org
-
David Hildenbrand -
Lorenzo Stoakes -
Mark Brown -
wang lian