Message-ID: <20260111142425.2783953-1-joshua.hahnjy@gmail.com>
Date: Sun, 11 Jan 2026 06:24:21 -0800
From: Joshua Hahn <joshua.hahnjy@...il.com>
To: Yajun Deng <yajun.deng@...ux.dev>
Cc: akpm@...ux-foundation.org,
	vbabka@...e.cz,
	surenb@...gle.com,
	mhocko@...e.com,
	jackmanb@...gle.com,
	hannes@...xchg.org,
	ziy@...dia.com,
	linux-mm@...ck.org,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH] mm/page_alloc: Avoid duplicate NR_FREE_PAGES updates in move_to_free_list()

On Sun, 11 Jan 2026 21:47:42 +0800 Yajun Deng <yajun.deng@...ux.dev> wrote:

> 
> 
> > 2026年1月10日 00:31，Joshua Hahn <joshua.hahnjy@...il.com> 写道：
> > 
> > On Fri,  9 Jan 2026 18:51:21 +0800 Yajun Deng <yajun.deng@...ux.dev> wrote:
> > 
> >> In move_to_free_list(), when a page block changes its migration type,
> >> we need to update free page counts for both the old and new types.
> >> Originally, this was done by two calls to account_freepages(), which
> >> updates NR_FREE_PAGES and also type-specific counters. However, this
> >> causes NR_FREE_PAGES to be updated twice, while the net change is zero
> >> in most cases.
> >> 
> >> This patch introduces a new function account_freepages_both() that
> >> updates the statistics for both old and new migration types in one go.
> >> It avoids the double update of NR_FREE_PAGES by computing the net change
> >> only when the isolation status changes.
> >> 
> >> The optimization avoid duplicate NR_FREE_PAGES updates in
> >> move_to_free_list().
> > 
> > Hi Yajun,
> > 
> > I hope you are doing well, thank you for the patch! I was hoping to better
> > understand the motivation behind this patch.
> > 
> > From my perspective, I believe that the current state of the code is
> > not optimal, but it is also not problematic. account_freepages seems like
> > a relatively cheap function (at the core, it's just some atomic operations).
> > Personally I also think that semantically, the code currently makes sense;
> > we are doing the accounting for the old mounttype, then for the new mounttype,
> > in a way that cancels out. And given that there is still some cases where
> > the work doesn't end up canceling out due to one of the mounttypes being
> > MIGRATE_ISOLATE, I think that there is enough purpose in making the two
> > calls to do the accounting twice.
> > 
> > On the other hand I think there is only one place in the codebase that
> > will use account_freepages_both, so it might make the burden to understand
> > the code a bit higher.
> > 
> > What do you think? I don't have a strong stance on whether the performance
> > effects are big here (if this change indeed has a big performance implication,
> > then we should definitely go forth with this!) but I do believe the current
> > code is quite semantically sound and more readable. 
> > 
> Hey Joshua,
> 
> Thank you for sharing your thoughts. 
> 
> I currently don’t have any performance data, I just noticed from looking at the code
> that there may be room for optimization.
> You’re right. The original code is indeed more straightforward. I think we can add some
> comments in the  account_freepages_both to make it easier to understand.

Hi Yajun, I hope you are doing well!

On second thought, I did notice that at the end of move_to_free_list, we have
some additional conditionals that depend on the migratetype of the mounttypes.

What if we open-code the account_freepages_both, and skip doing the
isolation checks twice? Your idea to use the ternary operator gave me this idea!

@@ -869,14 +877,17 @@ static inline void move_to_free_list(struct page *page, struct zone *zone,

        list_move_tail(&page->buddy_list, &area->free_list[new_mt]);

-       account_freepages(zone, -nr_pages, old_mt);
-       account_freepages(zone, nr_pages, new_mt);
-
-       if (order >= pageblock_order &&
-           is_migrate_isolate(old_mt) != is_migrate_isolate(new_mt)) {
-               if (!is_migrate_isolate(old_mt))
-                       nr_pages = -nr_pages;
-               __mod_zone_page_state(zone, NR_FREE_PAGES_BLOCKS, nr_pages);
+       if (!old_isolated)
+               account_specific_freepages(zone, -nr_pages, old_mt);
+       if !(new_isolated)
+               account_specific_freepages(zone, nr_pages, new_mt);
+
+       if (old_isolated != new_isolated) {
+               nr_pages = old_isolated ? nr_pages : -nr_pages;
+               __mod_zone_page_state(zone, NR_FREE_PAGES, nr_pages);
+               if (order >= pageblock_order)
+                       __mod_zone_page_state(zone, NR_FREE_PAGES_BLOCKS,
+                                             nr_pages);
        }
 }
 
I don't think it matters that we reorder the __mod_zone_page_state to be
after the account_specific_freepages here, so hopefully it is OK here.

So we can achieve the best of both worlds by preventing the duplicate adjustment
and also keep the control flow simple! (We can also just include that
additional check inside your account_freepages_both as well).

This is just my small idea : -) Of course, please feel free to ignore it if
you feel that it makes the code more confusing. I think that what is "simple"
is mostly subjective, so this was just my thought.

Thank you for your thoughts, I hope you have a great day!
Joshua

Powered by blists - more mailing lists