Re: [PATCH v3 3/3] kselftest: mm: introduce size_to_shift() helper

[Yeoreum Yun <yeoreum.yun@arm.com>]

On Tue, Sep 15, 2026 at 10:01:58AM +0100, Lorenzo Stoakes (ARM) wrote:
> On Mon, Sep 14, 2026 at 04:08:30PM +0200, David Hildenbrand (Arm) wrote:
> > On 9/14/26 16:06, Yeoreum Yun wrote:
> > > On Mon, Sep 14, 2026 at 04:01:02PM +0200, David Hildenbrand (Arm) wrote:
> > >> On 9/14/26 09:30, Yeoreum Yun wrote:
> > >>> Introduce size_to_shift() for future users that need to
> > >>> obtain shift of a huge-page which is other than pmd_psize().
> > >>>
> > >>> Suggested-by: David Hildenbrand (Arm) <david@kernel.org>
> > >>> Signed-off-by: Yeoreum Yun <yeoreum.yun@arm.com>
> > >>> ---
> > >>>  tools/testing/selftests/mm/vm_util.c | 10 +++++++++-
> > >>>  tools/testing/selftests/mm/vm_util.h |  1 +
> > >>>  2 files changed, 10 insertions(+), 1 deletion(-)
> > >>>
> > >>> diff --git a/tools/testing/selftests/mm/vm_util.c b/tools/testing/selftests/mm/vm_util.c
> > >>> index fd1432dc6152..1fef1266429f 100644
> > >>> --- a/tools/testing/selftests/mm/vm_util.c
> > >>> +++ b/tools/testing/selftests/mm/vm_util.c
> > >>> @@ -157,6 +157,14 @@ bool check_for_pattern(FILE *fp, const char *pattern, char *buf, size_t len)
> > >>>  	return false;
> > >>>  }
> > >>>
> 
> Probably worth a comment here saying it's only for power-of-two sizes.

Okay.

> 
> > >>> +uint64_t size_to_shift(uint64_t size)
> > >>> +{
> > >>> +	if (__builtin_popcountll(size) != 1)
> 
> I wonder if it wouldn't be better to just do the old trick of:
> 
> 	if (!size || (size & (size - 1)))
> 		return 0;
> 
> ?
> 
> > >>> +		return 0;
> > >>> +
> > >>> +	return ffsl(size) - 1;
> > >>> +}
> > >>
> > >> Could likely just be an inline helper?
> > >
> > > Okay. If there is no comments on today, I'll post tomorrow with this.
> > >
> >
> > No need to rush, leave some more days for people to catch up.
> 
> Agree with David seems better as an inline thing, the __builtin_xxx() should
> be... built-in hopefully :P though I suggest something that wouldn't need that.

Whatever is, okay. I'll change with old-classic trick :)
> 
> As long as strings.h is included in the header ofc for ffsl().
> 
> With the stuff above addressed, seems fine to me so:
> 
> Reviewed-by: Lorenzo Stoakes (ARM) <ljs@kernel.org>

Thanks!

> 
> >
> > --
> > Cheers,
> >
> > David
> 
> --
> Cheers, Lorenzo

-- 
Sincerely,
Yeoreum Yun