Re: [PATCH] mm: simplify zone_idx()

[Barry Song]

On Sat, Apr 12, 2025 at 8:34 PM gaoxu <gaoxu2@xxxxxxxxx> wrote:
>
> >
> > On Fri, Apr 11, 2025 at 2:42 AM Mike Rapoport <rppt@xxxxxxxxxx> wrote:
> > >
> > > Hi,
> > >
> > > On Thu, Apr 10, 2025 at 12:03:00PM +0000, gaoxu wrote:
> > > > store zone_idx directly in struct zone to simplify and optimize zone_idx()
> > >
> > > Do you see an actual speed up somewhere?
> Almost negligible. my simple code tests showed the patch provides an average improvement of ~0.02%.
> Thus, in the Android 15-6.6 kernel, I confidently retained the original zone_idx function.
> (https://android-review.googlesource.com/c/kernel/common/+/3578322/2/mm/page_alloc.c#770)
>
> This patch only eliminates 2-3 assembly instructions, making it challenging to
> observe measurable performance benefits.
> However, since the zone struct includes CACHELINE_PADDING (reserving unused space),
> adding a new member variable does not alter the size of zone. This makes the patch
> effectively zero-cost while achieving a cleaner implementation of zone_idx.

The struct zone contains many CONFIG_ options to include or exclude
certain fields.
If we're confident that our new zone_idx doesn't increase cacheline
usage for all those
cases, this seems like a worthwhile cleanup. Have you checked the numbers?

> >
> > +1. Curious if there's data indicating zone_idx is a hot path.
> There are several functions in the memory management code that are frequently
> executed and will call zone_idx:
> rmqueue()->wakeup_kswapd()->zone_idx()
> alloc_pages_bulk_noprof()->__count_zid_vm_events()->zone_idx()
>
> The patch (https://lore.kernel.org/all/20240229183436.4110845-2-yuzhao@xxxxxxxxxx/)
> will add new hotspot paths, with the details as follows:
> __zone_watermark_ok()->zone_is_suitable()->zone_idx()
> zone_watermark_fast()->zone_is_suitable()->zone_idx()
> get_page_from_freelist()->zone_is_suitable()->zone_idx()
> __free_one_page()->zone_max_order()->zone_idx()
>
> Although The patch (https://lore.kernel.org/all/20240229183436.4110845-2-yuzhao@xxxxxxxxxx/)
> has not yet merged into the Linux mainline; it is already included in Android 15-6.6.
> >
>
> > >
> > > > Signed-off-by: gao xu <gaoxu2@xxxxxxxxx>
> > > > ---
> > > >  include/linux/mmzone.h | 3 ++-
> > > >  mm/mm_init.c           | 1 +
> > > >  2 files changed, 3 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/include/linux/mmzone.h b/include/linux/mmzone.h
> > > > index 4c95fcc9e..7b14f577d 100644
> > > > --- a/include/linux/mmzone.h
> > > > +++ b/include/linux/mmzone.h
> > > > @@ -941,6 +941,7 @@ struct zone {
> > > >  #endif
> > > >
> > > >       const char              *name;
> > > > +     enum zone_type  zone_idx;
> > > >
> > > >  #ifdef CONFIG_MEMORY_ISOLATION
> > > >       /*
> > > > @@ -1536,7 +1537,7 @@ static inline int local_memory_node(int node_id)
> > { return node_id; };
> > > >  /*
> > > >   * zone_idx() returns 0 for the ZONE_DMA zone, 1 for the ZONE_NORMAL
> > zone, etc.
> > > >   */
> > > > -#define zone_idx(zone)               ((zone) -
> > (zone)->zone_pgdat->node_zones)
> > > > +#define zone_idx(zone)               ((zone)->zone_idx)
> > > >
> > > >  #ifdef CONFIG_ZONE_DEVICE
> > > >  static inline bool zone_is_zone_device(struct zone *zone)
> > > > diff --git a/mm/mm_init.c b/mm/mm_init.c
> > > > index 9659689b8..a7f7264f1 100644
> > > > --- a/mm/mm_init.c
> > > > +++ b/mm/mm_init.c
> > > > @@ -1425,6 +1425,7 @@ static void __meminit zone_init_internals(struct
> > zone *zone, enum zone_type idx,
> > > >       atomic_long_set(&zone->managed_pages, remaining_pages);
> > > >       zone_set_nid(zone, nid);
> > > >       zone->name = zone_names[idx];
> > > > +     zone->zone_idx = idx;
> > > >       zone->zone_pgdat = NODE_DATA(nid);
> > > >       spin_lock_init(&zone->lock);
> > > >       zone_seqlock_init(zone);
> > > > --
> > > > 2.17.1
> > >
> > > --
> > > Sincerely yours,
> > > Mike.
> >

Thanks
Barry