Hi David! On 7/21/26 7:22 PM, David Hildenbrand (Arm) wrote: > On 7/16/26 14:32, Sarthak Sharma wrote: >> Add a command-line tool for benchmarking get_user_pages fast-path >> (GUP_FAST), pin_user_pages fast-path (PIN_FAST), and pin_user_pages >> longterm (PIN_LONGTERM) via the CONFIG_GUP_TEST debugfs interface. >> >> When invoked without arguments, gup_bench runs the benchmark subset >> of configurations covered by run_gup_matrix() in run_vmtests.sh: all > > To we really need the matrix in there? The matrix was good for selftests, but > not when using it as a benchmarking tool, no?
Yeah makes sense, I'll remove the matrix part. > >> three GUP commands across read/write, private/shared mappings, and a >> range of page counts, with THP on/off for regular mappings and hugetlb >> for huge page mappings. Restore HugeTLB settings after each hugetlb >> benchmark run. Validate numeric command-line arguments instead of >> relying on atoi(). >> >> This tool is a mix of reused and new logic. The mapping/setup path comes >> from selftests/mm/gup_test.c, while the default benchmark matrix matches >> the benchmark subset of run_gup_matrix() in run_vmtests.sh. The standalone >> CLI and tools/mm integration are added here and the HugeTLB setup code is >> shared via tools/lib/mm. >> >> Add gup_bench to BUILD_TARGETS and INSTALL_TARGETS in tools/mm/Makefile, >> link it against the shared tools/lib/mm hugepage helpers, and ignore the >> resulting binary in tools/mm/.gitignore. While here, also add the >> missing thp_swap_allocator_test entry to .gitignore. >> >> Add tools/mm/gup_bench.c to the GUP entry in MAINTAINERS. >> >> Suggested-by: David Hildenbrand (Arm) <[email protected]> >> Signed-off-by: Sarthak Sharma <[email protected]> >> --- >> MAINTAINERS | 1 + >> tools/mm/.gitignore | 2 + >> tools/mm/Makefile | 10 +- >> tools/mm/gup_bench.c | 482 +++++++++++++++++++++++++++++++++++++++++++ >> 4 files changed, 492 insertions(+), 3 deletions(-) >> create mode 100644 tools/mm/gup_bench.c >> >> diff --git a/MAINTAINERS b/MAINTAINERS >> index 26b06d80ab45..7d121dee9f0f 100644 >> --- a/MAINTAINERS >> +++ b/MAINTAINERS >> @@ -17024,6 +17024,7 @@ T: git >> git://git.kernel.org/pub/scm/linux/kernel/git/akpm/mm >> F: mm/gup.c >> F: mm/gup_test.c >> F: mm/gup_test.h >> +F: tools/mm/gup_bench.c >> F: tools/testing/selftests/mm/gup_longterm.c >> F: tools/testing/selftests/mm/gup_test.c >> >> diff --git a/tools/mm/.gitignore b/tools/mm/.gitignore >> index 922879f93fc8..154d740be02e 100644 >> --- a/tools/mm/.gitignore >> +++ b/tools/mm/.gitignore >> @@ -2,3 +2,5 @@ >> slabinfo >> page-types >> page_owner_sort >> +thp_swap_allocator_test > > ^ Does that belong into this patch? No it's just that when I was working on v1 of this patchset, I saw that this binary hadn't been added to .gitignore so I added this, since I was already modifying .gitignore to add gup_bench. It has stuck around since then :). Will drop this from here and send a patch separately for this. > >> +gup_bench > > I wonder if it makes sense to "git mv" the existing tool (only slightly fixing > header inclusion to make it compile), essentially removing the old benchmark. > > So it would be > > 1) Move tools, fixing up includes and removing the gup testing logic+matrix to > make it compile > > 2) Extend tool by XYZ > > 3) Add completely new selftest > > Because looking at patch #5, there is not really any code reuse there, but I > suspect there should be some in this patch here when moving instead of > copying. > > That would make this a lot easier to review. > I agree that this would make it easier to review, but then there would be an intermediate commit where there is no gup_test selftest. Is this fine from a bisectability point of view?
