Re: [PATCH v2] selftests/mm: Add process_madvise() tests

Hi Wang,

On Mon, 30 Jun 2025 22:09:57 +0800 wang lian lianux.mm@gmail.com wrote:

This patch adds tests for the process_madvise(), focusing on verifying behavior under various conditions including valid usage and error cases.

Signed-off-by: wang lianlianux.mm@gmail.com Suggested-by: Lorenzo Stoakes lorenzo.stoakes@oracle.com Suggested-by: David Hildenbrand david@redhat.com

I left a few trivial comments below, but overall this looks useful to me. Thank you :)

Acked-by: SeongJae Park sj@kernel.org

FYI, 'git am' of this patch on latest mm-new fails. Maybe this is not based on the latest mm-new? But, I also found Andrew added this to mm tree: https://lore.kernel.org/20250630230052.AB95CC4CEE3@smtp.kernel.org . Maybe Andrew did rebase on his own?

[...]

--- /dev/null +++ b/tools/testing/selftests/mm/process_madv.c

[...]

+/* Try and read from a buffer, return true if no fatal signal. */ +static bool try_read_buf(char *ptr) +{

• return try_access_buf(ptr, false);

Seems this is the only caller of try_access_buf(). How about removing 'write' argument of try_access_buf() and its handling?

+}

+TEST_F(process_madvise, basic) +{

• const unsigned long pagesize = (unsigned long)sysconf(_SC_PAGESIZE);
• const int madvise_pages = 4;
• char *map;
• ssize_t ret;
• struct iovec vec[madvise_pages];
• /*

• * Create a single large mapping. We will pick pages from this
  

• * mapping to advise on. This ensures we test non-contiguous iovecs.
  

• */
  

• map = mmap(NULL, pagesize * 10, PROT_READ | PROT_WRITE,

•    MAP_PRIVATE | MAP_ANONYMOUS, -1, 0);
  

• ASSERT_NE(map, MAP_FAILED);

So this will make this test marked as failed, for mmap() failure, which doesn't really mean something in process_madvise() is wrong. What about using ksft_exit_skip() with failure check, instead?

• /* Fill the entire region with a known pattern. */
• memset(map, 'A', pagesize * 10);
• /*

• * Setup the iovec to point to 4 non-contiguous pages
  

• * within the mapping.
  

• */
  

• vec[0].iov_base = &map[0 * pagesize];
• vec[0].iov_len = pagesize;
• vec[1].iov_base = &map[3 * pagesize];
• vec[1].iov_len = pagesize;
• vec[2].iov_base = &map[5 * pagesize];
• vec[2].iov_len = pagesize;
• vec[3].iov_base = &map[8 * pagesize];
• vec[3].iov_len = pagesize;
• ret = sys_process_madvise(PIDFD_SELF, vec, madvise_pages, MADV_DONTNEED,

• 		  0);
  

• if (ret == -1 && errno == EPERM)

• ksft_exit_skip(
  

• 	"process_madvise() unsupported or permission denied, try running as root.\n");
  

• else if (errno == EINVAL)

• ksft_exit_skip(
  

• 	"process_madvise() unsupported or parameter invalid, please check arguments.\n");
  

• /* The call should succeed and report the total bytes processed. */
• ASSERT_EQ(ret, madvise_pages * pagesize);
• /* Check that advised pages are now zero. */
• for (int i = 0; i < madvise_pages; i++) {

• char *advised_page = (char *)vec[i].iov_base;
  

• /* Access should be successful (kernel provides a new page). */
  

• ASSERT_TRUE(try_read_buf(advised_page));
  

• /* Content must be 0, not 'A'. */
  

• ASSERT_EQ(*advised_page, 0);
  

• }
• /* Check that an un-advised page in between is still 'A'. */
• char *unadvised_page = &map[1 * pagesize];
• ASSERT_TRUE(try_read_buf(unadvised_page));
• ASSERT_EQ(*unadvised_page, 'A');

What about using memcpy() and memcmp() for testing full bytes? That could do more complete test, and reduce unnecessary volatile and helper functions?

• /* Cleanup. */
• ASSERT_EQ(munmap(map, pagesize * 10), 0);

Again, I think ksft_exit_skip() might be a better approach.

+}

+static long get_smaps_anon_huge_pages(pid_t pid, void *addr) +{

• char smaps_path[64];
• char *line = NULL;
• unsigned long start, end;
• long anon_huge_kb;
• size_t len;
• FILE *f;
• bool in_vma;
• in_vma = false;
• sprintf(smaps_path, "/proc/%d/smaps", pid);

I understand this is just a test code, but... What about using the safer one, snprintf()?

• f = fopen(smaps_path, "r");
• if (!f)

• return -1;
  

• while (getline(&line, &len, f) != -1) {

• /* Check if the line describes a VMA range */
  

• if (sscanf(line, "%lx-%lx", &start, &end) == 2) {
  

• 	if ((unsigned long)addr >= start &&
  

• 	    (unsigned long)addr < end)
  

• 		in_vma = true;
  

• 	else
  

• 		in_vma = false;
  

• 	continue;
  

• }
  

• /* If we are in the correct VMA, look for the AnonHugePages field */
  

• if (in_vma &&
  

•     sscanf(line, "AnonHugePages: %ld kB", &anon_huge_kb) == 1)
  

• 	break;
  

• }
• free(line);
• fclose(f);
• return (anon_huge_kb > 0) ? (anon_huge_kb * 1024) : 0;

+}

[...]

+TEST_HARNESS_MAIN \ No newline at end of file

checkpatch.pl complaints having no newline at the end of the file.

diff --git a/tools/testing/selftests/mm/run_vmtests.sh b/tools/testing/selftests/mm/run_vmtests.sh index f96d43153fc0..5c28ebcf1ea9 100755 --- a/tools/testing/selftests/mm/run_vmtests.sh +++ b/tools/testing/selftests/mm/run_vmtests.sh @@ -61,6 +61,8 @@ separated by spaces: ksm tests that require >=2 NUMA nodes

• pkey memory protection key tests

+- process_madvise

Shouldn't this match with the real category name (process_madv) ?

• test process_madvise

• soft_dirty test soft dirty page bit semantics
• pagemap

@@ -424,6 +426,9 @@ CATEGORY="hmm" run_test bash ./test_hmm.sh smoke # MADV_GUARD_INSTALL and MADV_GUARD_REMOVE tests CATEGORY="madv_guard" run_test ./guard-regions +# PROCESS_MADVISE TEST +CATEGORY="process_madv" run_test ./process_madv

# MADV_DONTNEED and PROCESS_DONTNEED tests CATEGORY="madv_dontneed" run_test ./madv_dontneed -- 2.43.0

Thanks, SJ