Re: [PATCH 01/17] mm/sparse: drop power-of-2 size requirement for struct mem_section

[Muchun Song <muchun.song@linux.dev>]

Sorry, there is some problem issue with my email, let me sent again.

> On Jul 15, 2026, at 23:52, David Laight <david.laight.linux@gmail.com> wrote:
> 
> On Wed, 15 Jul 2026 21:15:45 +0800
> Muchun Song <muchun.song@linux.dev> wrote:
> 
>>> On Jul 15, 2026, at 17:34, David Laight <david.laight.linux@gmail.com> wrote:
>>> 
>>> On Thu,  2 Jul 2026 17:38:05 +0800
>>> Muchun Song <songmuchun@bytedance.com> wrote:
>>> 
>>>> struct mem_section is currently forced to a power-of-2 size so the
>>>> section-to-root lookup can use a mask instead of a modulo.
>>>> 
>>>> That requirement adds configuration-dependent padding, especially with
>>>> CONFIG_PAGE_EXTENSION, just to preserve the lookup scheme.
>>>> 
>>>> Drop the constraint and use a plain modulo for the lookup instead. The
>>>> divisor is constant, so the generated code remains cheap while avoiding
>>>> the extra padding. It also removes an unnecessary layout constraint
>>>> from the type.  
>>> 
>>> This has a side effect of changing the size of the 'section' from
>>> PAGE_SIZE to something 'a bit smaller' when CONFIG_PAGE_EXTENSION
>>> is defined.
>>> I don't think it actually matters, the allocation is done by:
>>> 
>>> static noinline struct mem_section __ref *sparse_index_alloc(int nid)
>>> {
>>> struct mem_section *section = NULL;
>>> unsigned long array_size = SECTIONS_PER_ROOT *
>>>    sizeof(struct mem_section);
>>> 
>>> if (slab_is_available()) {
>>> section = kzalloc_node(array_size, GFP_KERNEL, nid);
>>> } else {
>>> section = memblock_alloc_node(array_size, SMP_CACHE_BYTES,
>>>       nid);
>>> 
>>> so the size might get rounded up to PAGE_SIZE anyway.  
>> 
>> I'm not sure I really understand what you mean. You might be asking whether
>> the reduction in the size of the `mem_section` structure does not actually result
>> in memory savings? If so, please let me explain clearly. As you mentioned, the
>> size of memory allocated each time here should be PAGE_SIZE. Before the
>> modification, one page could hold 4096/32 = 128 `struct mem_section` instances;
>> with the modified code, the number of `struct mem_section` instances that can fit
>> is 4096/24 = 170. Therefore, the range of memory sections that a PAGE_SIZE can
>> cover has increased 32%.
>> 
>> Please let me know if I didn’t get your point.
> 
> 170 * 24 is 4080 - 16 bytes less than PAGE_SIZE.
> In principle kmalloc() need not allocate a full page for it which would
> lead to the data crossing a page boundary - which may not be intended.
> 
>> 
>>> 
>>> I also suspect that '% 24u' might be enough slower than '% 32u' to
>>> generate a measurable performance drop.
>>> (It doesn't matter whether you do '& 31' or '% 32u'.)  
>> 
>> David, I agree that % 24u is slower than % 32u — the latter maps to ‘and', while the
>> former requires a multiply-shift sequence. However, since the divisor is a constant,
>> the compiler should uses the magic multiplier approach at -O2, which is ~3-5 cycles
>> instead of 1. So I think the per-lookup overhead is real but small.
> 
> The divide/remainder is by 170 not 24 - but the effect is the same.

Oh, yes, it is 170.

> 
> Indeed, but it might be a hot enough path to be measurable.
> I've just done a pile of test compiles, see https://godbolt.org/z/zq8WjaqGj

Excellent work. Very clear.

> 
> The remainder code has to do the divide, gcc might notice it has already
> done it - but it would be better to do it explicitly.
> However, both sparc64 and looooongarch64 end up executing divides.
> Also some of the others aren't as short as you might expect, both
> generating the constant for the multiply and avoiding a 'mul' instruction
> for the remainder (esp. s390).

I didn't realize that architectures like sparc64 and loongarch64 would still
emit actual hardware divide instructions for constant modulo operations (I
only checked x86 yesterday).

Given these side effects, preserving the power-of-2 size constraint and using
the mask-based lookup is indeed the much safer and faster approach. I will
withdraw this patch to and properly handle the alignment constraint when
introducing the ->order field later.

Thanks again for your valuable review!

Muchun

> 
> David
> 
>> 
>> Thanks for your review.
>> 
>> Muchun
>> 
>>> 
>>> David