Re: [PATCH v5 4/5] tools/mm: add a standalone GUP microbenchmark

[Dev Jain <dev.jain@arm.com>]

On 21/07/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?
> 
>> 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) <david@kernel.org>
>> Signed-off-by: Sarthak Sharma <sarthak.sharma@arm.com>
>> ---
>>  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?
> 
>> +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

I like your idea, it would reduce churn. But is it okay that there will be an
intermediate state in which gup_test is not there? Wouldn't we call this a
"regression"?


> 
> 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.
>