Re: [PATCH] mm/page_alloc: Avoid duplicate NR_FREE_PAGES updates in move_to_free_list()

[Yajun Deng <yajun.deng@linux.dev>]

> 2026年1月11日 22:24，Joshua Hahn <joshua.hahnjy@gmail.com> 写道：
> 
> On Sun, 11 Jan 2026 21:47:42 +0800 Yajun Deng <yajun.deng@linux.dev> wrote:
> 
>> 
>> 
>>> 2026年1月10日 00:31，Joshua Hahn <joshua.hahnjy@gmail.com> 写道：
>>> 
>>> On Fri,  9 Jan 2026 18:51:21 +0800 Yajun Deng <yajun.deng@linux.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.
> 

I think this is a good idea and I will adopt it.
Thank you

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