On Thu, Jul 10, 2025 at 07:22:49PM +0800, wang lian wrote:
Add tests for process_madvise(), focusing on verifying behavior under various conditions including valid usage and error cases.
--- a/tools/testing/selftests/mm/guard-regions.c +++ b/tools/testing/selftests/mm/guard-regions.c
-static void handle_fatal(int c) -{
- if (!signal_jump_set)
return;- siglongjmp(signal_jmp_buf, c);
-}
I see from looking later in the patch that you're factoring this out of the guard regions test into vm_util.c so that it can be used by your new test. This is good and sensible but it's a bit surprising, especially since your changelog only said you were adding a new test. It would be better to split this out into a separate refactoring patch that just does the code motion, as covered in submitting-patches.rst it's better if changes just do one thing.
+#include <linux/pidfd.h> +#include <linux/uio.h>
Does this work without 'make headers_install' for the systems that were affectd by missing headers? Lorenzo mentioned that we shouldn't depend on that for the mm tests (I'm not enthusiastic about that approach myself, but if it's what mm needs).
- ret = read(pipe_info[0], &info, sizeof(info));
- if (ret <= 0) {
waitpid(self->child_pid, NULL, 0);ksft_exit_skip("Failed to read child info from pipe.\n");- }
If you're using the harness you should use SKIP() rather than the ksft APIs for reporting test results. Don't mix and match the result reporting APIs, harness will call the ksft_ APIs appropriately for you.
/** MADV_COLLAPSE lost the race to khugepaged, which* likely held a page lock. The kernel correctly* reports this temporary contention with EAGAIN.*/if (errno == EAGAIN) {ksft_test_result_skip("THP is 'always', process_madvise returned EAGAIN due to an expected race with khugepaged.\n");} else {ksft_test_result_fail("process_madvise failed with unexpected errno %d in 'always' mode.\n",errno);}
Similarly, to fail use an ASSERT or EXPECT. Note also that when using the ksft_ API for reporting results each test should report a consistent test name as the string, if you want to report an error message print it separately to the test result.
This applies throughout the program.
+/*
- Test process_madvise() with various invalid pidfds to ensure correct error
- handling. This includes negative fds, non-pidfd fds, and pidfds for
- processes that no longer exist.
- */
This sounds like it should be a series of small tests rather than a single omnibus test, that'd result in clearer error reporting from test frameworks since they will say which operation failed directly rather than having to look at the logs then match them to the source.
- pidfd = syscall(__NR_pidfd_open, child_pid, 0);
- ASSERT_GE(pidfd, 0);
This is particularly the case given the use of ASSERTs, we'll not report any issues other than the first one we hit.