+cc Liam, David, Vlastimil, Jann
(it might not be obvious from get_maintainers.pl but please cc
maintainers/reviewers of the thing you are adding a test for, thanks!)
Overall I'm not in favour of us taking this patch.
There are a number of issues with it (explained inline below), but those aside,
it seems to be:
- Checking whether a simple anon buffer of arbitrary size is zapped by
MADV_DONTNEED.
- Printing out a dubious microbenchmark that seems to be mostly asserting that
fewer sycalls are faster when using process_madvise() locally.
And I'm struggling to see the value of that.
The test is also slow and will slow down a test run for little benefit.
On Sat, Jun 21, 2025 at 09:30:03PM +0800, wang lian wrote:
> From: Lian Wang
>
> Let's add a simple test for MADV_DONTNEED and PROCESS_MADV_DONTNEED,
I'm not sure what PROCESS_MADV_DONTNEED is?
you mean process_madvise(..., MADV_DONTNEED, ...)?
> and inspired by SeongJae Park's test at GitHub[1] add batch test
> for PROCESS_MADV_DONTNEED,but for now it influence by workload and
> need add some race conditions test.We can add it later.
>
> Signed-off-by: Lian Wang
> References
> ==
>
> [1] https://github.com/sjp38/eval_proc_madvise
>
> ---
> tools/testing/selftests/mm/.gitignore | 1 +
> tools/testing/selftests/mm/Makefile| 1 +
> tools/testing/selftests/mm/madv_dontneed.c | 220 +
> tools/testing/selftests/mm/run_vmtests.sh | 5 +
> 4 files changed, 227 insertions(+)
> create mode 100644 tools/testing/selftests/mm/madv_dontneed.c
>
> diff --git a/tools/testing/selftests/mm/.gitignore
> b/tools/testing/selftests/mm/.gitignore
> index 824266982aa3..911f39d634be 100644
> --- a/tools/testing/selftests/mm/.gitignore
> +++ b/tools/testing/selftests/mm/.gitignore
> @@ -25,6 +25,7 @@ pfnmap
> protection_keys
> protection_keys_32
> protection_keys_64
> +madv_dontneed
> madv_populate
> uffd-stress
> uffd-unit-tests
> diff --git a/tools/testing/selftests/mm/Makefile
> b/tools/testing/selftests/mm/Makefile
> index ae6f994d3add..2352252f3914 100644
> --- a/tools/testing/selftests/mm/Makefile
> +++ b/tools/testing/selftests/mm/Makefile
> @@ -67,6 +67,7 @@ TEST_GEN_FILES += hugepage-mremap
> TEST_GEN_FILES += hugepage-shm
> TEST_GEN_FILES += hugepage-vmemmap
> TEST_GEN_FILES += khugepaged
> +TEST_GEN_FILES += madv_dontneed
> TEST_GEN_FILES += madv_populate
> TEST_GEN_FILES += map_fixed_noreplace
> TEST_GEN_FILES += map_hugetlb
> diff --git a/tools/testing/selftests/mm/madv_dontneed.c
> b/tools/testing/selftests/mm/madv_dontneed.c
> new file mode 100644
> index ..b88444da7f9e
> --- /dev/null
> +++ b/tools/testing/selftests/mm/madv_dontneed.c
> @@ -0,0 +1,220 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * MADV_DONTNEED and PROCESS_MADV_DONTNEED tests
> + *
> + * Copyright (C) 2025, Linx Software Corp.
> + *
> + * Author(s): Lian Wang
> + */
> +#define _GNU_SOURCE
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include "vm_util.h"
> +#include
> +
> +#include "../kselftest.h"
> +
> +/*
> + * For now, we're using 2 MiB of private anonymous memory for all tests.
> + */
> +#define SIZE (256 * 1024 * 1024)
This is 256 MB?
Also you don't need to do:
/*
* foo bar baz bar foo
*/
Just do:
/* foo bar baz bar foo */
> +
> +static size_t pagesize;
> +
> +static void sense_support(void)
> +{
> + char *addr;
> + int ret;
> +
> + addr = mmap(0, pagesize, PROT_READ | PROT_WRITE,
> + MAP_ANONYMOUS | MAP_PRIVATE, 0, 0);
This is incorrect, you need to specify -1 for fd for anon mappings, not zero.
Also the first parameter in mmap() is a pointer, use NULL.
> + if (!addr)
> + ksft_exit_fail_msg("mmap failed\n");
This is incorrect, you need to check for MAP_FAILED.
> +
> + ret = madvise(addr, pagesize, MADV_DONTNEED);
> + if (ret)
> + ksft_exit_skip("MADV_DONTNEED is not available\n");
This is incorrect, you're testing for a current kernel, MADV_DONTNEED is very
definitely always available.
I'm guessing you've copy/pasted from madv_populate.c, which really shouldn't be
'sensing' if this option is available either - the tests are for the most recent
kernel, in which you know the behaviour is supported.
Anyway you should just drop this.
> +
> + munmap(addr, pagesize);
> +}
> +
> +/*
> + * Read pagemap to check page is present in mermory
> + */
> +static bool is_page_present(void *addr)
This seems a flakey thing to test? While unlikely, a page might be swapped
out. So you will want to add a check for this also.
> +{
> + uintptr_t vaddr = (uintptr_t)addr;
> + uintptr_t offset = (vaddr / pagesize) * sizeof(uint64_t);
> + ssize_t bytes_read;
> + uint64_t entry;
> + bool ret;
> + int fd;
> +
> + fd = open("/proc/self/pagemap", O_RDONLY);
> + if (fd < 0) {
> + ksft_exit_fail_msg("opening
