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

[Mark Brown <broonie@kernel.org>]

[-- Attachment #1: Type: text/plain, Size: 3115 bytes --]

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.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]