Hi Mark Brown,
> 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.
Thanks for the suggestion. I???ll split this out into a separate patch
that just moves the helper to vm_util.c, and follow up with the new
test in a second patch.
> > +#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).
You're right, and I???ve seen build issues due to that as well. I???ll drop
<linux/pidfd.h> and define PIDFD_SELF locally to avoid requiring
installed headers.
> > + 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.
Understood. I???ll convert this and other cases to use SKIP() and ensure
the test consistently uses the test harness macros.
> > + 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.
I???ll revise this to use ASSERT/EXPECT, and separate error output from
test result strings, as you suggested.
> > + * 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.
That makes sense. I???ll break this out into multiple smaller tests so
each case reports independently.
> > + 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.
Thanks, I???ll switch to EXPECT_* where appropriate to allow multiple
checks per test case.
Thanks again for the detailed review!
Best regards,
Wang Lian
