Linux-mm Archive on lore.kernel.org
 help / color / mirror / Atom feed
From: David Laight <david.laight.linux@gmail.com>
To: Muchun Song <muchun.song@linux.dev>
Cc: Muchun Song <songmuchun@bytedance.com>,
	Andrew Morton <akpm@linux-foundation.org>,
	Oscar Salvador <osalvador@suse.de>,
	David Hildenbrand <david@kernel.org>,
	linux-mm@kvack.org, Mike Rapoport <rppt@kernel.org>,
	Vlastimil Babka <vbabka@kernel.org>,
	Lorenzo Stoakes <ljs@kernel.org>, Michal Hocko <mhocko@suse.com>,
	linux-kernel@vger.kernel.org
Subject: Re: [PATCH 01/17] mm/sparse: drop power-of-2 size requirement for struct mem_section
Date: Wed, 15 Jul 2026 16:52:22 +0100	[thread overview]
Message-ID: <20260715165222.0164a76c@pumpkin> (raw)
In-Reply-To: <71E70E00-98A1-4769-A167-5F4CB59EC96B@linux.dev>

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.

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

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).

	David

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



  reply	other threads:[~2026-07-15 15:52 UTC|newest]

Thread overview: 16+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
     [not found] <20260702093821.2740183-1-songmuchun@bytedance.com>
     [not found] ` <20260702093821.2740183-5-songmuchun@bytedance.com>
2026-07-09 10:45   ` [PATCH 04/17] mm/mm_init: skip initializing shared vmemmap tail pages Mike Rapoport
2026-07-09 12:31     ` Muchun Song
2026-07-09 13:05       ` Mike Rapoport
2026-07-09 13:23         ` Muchun Song
2026-07-15  5:08   ` Mike Rapoport
     [not found] ` <20260702093821.2740183-9-songmuchun@bytedance.com>
2026-07-09 10:54   ` [PATCH 08/17] mm/sparse: mark memory sections present earlier Mike Rapoport
2026-07-09 12:35     ` Muchun Song
     [not found] ` <20260702093821.2740183-3-songmuchun@bytedance.com>
2026-07-15  5:08   ` [PATCH 02/17] mm/sparse-vmemmap: track compound page order in struct mem_section Mike Rapoport
     [not found]   ` <20260716101559.2e640e99@pumpkin>
2026-07-16 10:57     ` Muchun Song
     [not found] ` <20260702093821.2740183-7-songmuchun@bytedance.com>
2026-07-15  5:08   ` [PATCH 06/17] mm/sparse-vmemmap: support section-based vmemmap accounting Mike Rapoport
2026-07-15 14:44     ` Muchun Song
     [not found] ` <20260702093821.2740183-2-songmuchun@bytedance.com>
2026-07-15  5:08   ` [PATCH 01/17] mm/sparse: drop power-of-2 size requirement for struct mem_section Mike Rapoport
     [not found]   ` <20260715103445.31b2f727@pumpkin>
2026-07-15 13:15     ` Muchun Song
2026-07-15 15:52       ` David Laight [this message]
     [not found] ` <20260702093821.2740183-4-songmuchun@bytedance.com>
2026-07-15  5:08   ` [PATCH 03/17] mm/sparse-vmemmap: introduce folio-oriented vmemmap optimization macros Mike Rapoport
     [not found] ` <20260702093821.2740183-8-songmuchun@bytedance.com>
2026-07-15  5:08   ` [PATCH 07/17] mm/sparse-vmemmap: support section-based vmemmap optimization Mike Rapoport

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20260715165222.0164a76c@pumpkin \
    --to=david.laight.linux@gmail.com \
    --cc=akpm@linux-foundation.org \
    --cc=david@kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-mm@kvack.org \
    --cc=ljs@kernel.org \
    --cc=mhocko@suse.com \
    --cc=muchun.song@linux.dev \
    --cc=osalvador@suse.de \
    --cc=rppt@kernel.org \
    --cc=songmuchun@bytedance.com \
    --cc=vbabka@kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox