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

["David Hildenbrand (Arm)" <david@kernel.org>]

On 7/24/26 14:05, Dev Jain wrote:
> 
> 
> 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"?

I don't really see why that as a problem as we are effectively removing the old
test and replacing it by a new one?

In fact, while at it, we can just call the new test "gup.c", as the "_test" is
superfluous.

-- 
Cheers,

David