Re: [PATCH] mm/mm_init.c: remove obsolete macro HASH_SMALL

[Mike Rapoport]

On Sat, Jun 17, 2023 at 04:39:44PM +0800, Miaohe Lin wrote:
> On 2023/6/17 15:56, Mike Rapoport wrote:
> > On Sat, Jun 17, 2023 at 03:09:55PM +0800, Miaohe Lin wrote:
> >> HASH_SMALL only works when parameter numentries is 0. But the sole caller
> >> futex_init() never calls alloc_large_system_hash() with numentries set to
> >> 0. 
> > 
> 
> Thanks for your quick review.
> 
> > Doesn't it? 
> > What happens when CONFIG_BASE_SMALL is set?
> 
> When CONFIG_BASE_SMALL is set, futex_hashsize is set to 16 and alloc_large_system_hash() is called with
> numentries == 16 && flags == HASH_SMALL. But in the alloc_large_system_hash(), we have the below logic:
> 
> alloc_large_system_hash()
> {
>   if (!numentries) { /* numentries == 16 here, so this code block is skipped. */
>     ...
>     if (unlikely(flags & HASH_SMALL)) { /* So as here. */
>       ...
>   }
>   ...
> }
> 
> So HASH_SMALL is just unused. Or am I miss something?

You are right, I've missed that. 

> Thanks.
> 
> > 
> >> So HASH_SMALL is obsolete and remove it.
> > 
> >> Signed-off-by: Miaohe Lin <linmiaohe@xxxxxxxxxx>
> >> ---
> >>  include/linux/memblock.h |  2 --
> >>  kernel/futex/core.c      |  3 +--
> >>  mm/mm_init.c             | 10 +---------
> >>  3 files changed, 2 insertions(+), 13 deletions(-)
> >>
> >> diff --git a/include/linux/memblock.h b/include/linux/memblock.h
> >> index f71ff9f0ec81..346d80809517 100644
> >> --- a/include/linux/memblock.h
> >> +++ b/include/linux/memblock.h
> >> @@ -581,8 +581,6 @@ extern void *alloc_large_system_hash(const char *tablename,
> >>  				     unsigned long high_limit);
> >>  
> >>  #define HASH_EARLY	0x00000001	/* Allocating during early boot? */
> >> -#define HASH_SMALL	0x00000002	/* sub-page allocation allowed, min
> >> -					 * shift passed via *_hash_shift */
> >>  #define HASH_ZERO	0x00000004	/* Zero allocated hash table */

Can you update HASH_ZERO to 0x2?

> >>  
> >>  /* Only NUMA needs hash distribution. 64bit NUMA architectures have
> >> diff --git a/kernel/futex/core.c b/kernel/futex/core.c
> >> index 514e4582b863..f10587d1d481 100644
> >> --- a/kernel/futex/core.c
> >> +++ b/kernel/futex/core.c
> >> @@ -1132,8 +1132,7 @@ static int __init futex_init(void)
> >>  #endif
> >>  
> >>  	futex_queues = alloc_large_system_hash("futex", sizeof(*futex_queues),
> >> -					       futex_hashsize, 0,
> >> -					       futex_hashsize < 256 ? HASH_SMALL : 0,
> >> +					       futex_hashsize, 0, 0,
> >>  					       &futex_shift, NULL,
> >>  					       futex_hashsize, futex_hashsize);
> >>  	futex_hashsize = 1UL << futex_shift;
> >> diff --git a/mm/mm_init.c b/mm/mm_init.c
> >> index d393631599a7..fab3c4649d5b 100644
> >> --- a/mm/mm_init.c
> >> +++ b/mm/mm_init.c
> >> @@ -2492,15 +2492,7 @@ void *__init alloc_large_system_hash(const char *tablename,
> >>  		else
> >>  			numentries <<= (PAGE_SHIFT - scale);
> >>  
> >> -		/* Make sure we've got at least a 0-order allocation.. */
> >> -		if (unlikely(flags & HASH_SMALL)) {
> >> -			/* Makes no sense without HASH_EARLY */
> >> -			WARN_ON(!(flags & HASH_EARLY));
> >> -			if (!(numentries >> *_hash_shift)) {
> >> -				numentries = 1UL << *_hash_shift;
> >> -				BUG_ON(!numentries);
> >> -			}
> >> -		} else if (unlikely((numentries * bucketsize) < PAGE_SIZE))
> >> +		if (unlikely((numentries * bucketsize) < PAGE_SIZE))
> >>  			numentries = PAGE_SIZE / bucketsize;
> >>  	}
> >>  	numentries = roundup_pow_of_two(numentries);
> >> -- 
> >> 2.27.0
> >>
> > 
> 

-- 
Sincerely yours,
Mike.