On Mon, Jun 23, 2025 at 03:49:05PM +0200, David Hildenbrand wrote:
On 23.06.25 14:35, Lorenzo Stoakes wrote:
+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.
We have other tests that should already severely break if MADV_DONTNEED doesn't work ... but sure, we could think about more elaborate functional tests when they provide a clear benefit. (zapping all kinds of memory types, anon/ksm/huge zeropage/pagecache/hugetlb/ ... and using /proc/self/pagemap to see if the page table mappings are already gone)
Yes right, exactly.
I don't think we have a lot of process_madvise selftests, right?
No and that's an issue. It'd be good to have some of those, but not as a benchmark.
I think the only stuff we have right now is in the guard-region tests which I added to assert it worked with MADV_GUARD_INSTALL/REMOVE.
It'd be good to have stuff that tested remote process stuff (tricky to set up, maybe test has to fork itself etc.) as well as the stuff I added allowing self-madvise().
But again we'd probably really want to find a way to exercise this stuff properly.
hugtlb handling that was added recently is already tested to some degree in hugetlb-madvise.c.
In general, I'm not a fan of selftests that measure syscall performance ...
:)
-- Cheers,
David / dhildenb