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

[David Hildenbrand (Arm)]

On 7/22/26 06:38, Sarthak Sharma wrote:
> 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) <david@xxxxxxxxxx>
>>> Signed-off-by: Sarthak Sharma <sarthak.sharma@xxxxxxx>
>>> ---
>>>  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.

Yes please, thanks!

> 
>>
>>> +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?

Perfectly fine I think!

-- 
Cheers,

David