Re: [PATCH v3] selftests/mm: add process_madvise() tests

[wang lian <lianux.mm@gmail.com>]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset=yes, Size: 3755 bytes --]

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