Hi Wang, On Mon, 30 Jun 2025 22:09:57 +0800 wang lian <[email protected]> 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 lian<[email protected]> > Suggested-by: Lorenzo Stoakes <[email protected]> > Suggested-by: David Hildenbrand <[email protected]> I left a few trivial comments below, but overall this looks useful to me. Thank you :) Acked-by: SeongJae Park <[email protected]> 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/[email protected] . 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
