Re: [PATCH 3/4] mm/vmstat: remove unused __node_stat_* wrappers

[Ye Liu <ye.liu@linux.dev>]

在 2026/4/15 10:29, Joshua Hahn 写道:
> On Wed, 15 Apr 2026 08:50:54 +0800 Ye Liu <ye.liu@linux.dev> wrote:
> 
>>
>>
>> 在 2026/4/14 22:59, Joshua Hahn 写道:
>>> On Tue, 14 Apr 2026 17:15:20 +0800 Ye Liu <ye.liu@linux.dev> wrote:
>>>
>>>> From: Ye Liu <liuye@kylinos.cn>
>>>>
>>>> Replace the single call to __node_stat_mod_folio()
>>>> with node_stat_mod_folio(), and remove the dead inline __node_stat_*
>>>> wrapper definitions from include/linux/vmstat.h.
>>>>
>>>> Signed-off-by: Ye Liu <liuye@kylinos.cn>
>>>> ---
>>>>  include/linux/vmstat.h | 18 ------------------
>>>>  mm/page-writeback.c    |  2 +-
>>>>  2 files changed, 1 insertion(+), 19 deletions(-)
>>>>
>>>> diff --git a/include/linux/vmstat.h b/include/linux/vmstat.h
>>>> index 3c9c266cf782..54da7d820f78 100644
>>>> --- a/include/linux/vmstat.h
>>>> +++ b/include/linux/vmstat.h
>>>> @@ -440,24 +440,6 @@ static inline void zone_stat_sub_folio(struct folio *folio,
>>>>  	mod_zone_page_state(folio_zone(folio), item, -folio_nr_pages(folio));
>>>>  }
>>>>  
>>>> -static inline void __node_stat_mod_folio(struct folio *folio,
>>>> -		enum node_stat_item item, long nr)
>>>> -{
>>>> -	__mod_node_page_state(folio_pgdat(folio), item, nr);
>>>> -}
>>>> -
>>>> -static inline void __node_stat_add_folio(struct folio *folio,
>>>> -		enum node_stat_item item)
>>>> -{
>>>> -	__mod_node_page_state(folio_pgdat(folio), item, folio_nr_pages(folio));
>>>> -}
>>>> -
>>>> -static inline void __node_stat_sub_folio(struct folio *folio,
>>>> -		enum node_stat_item item)
>>>> -{
>>>> -	__mod_node_page_state(folio_pgdat(folio), item, -folio_nr_pages(folio));
>>>> -}
>>>> -
>>>>  static inline void node_stat_mod_folio(struct folio *folio,
>>>>  		enum node_stat_item item, long nr)
>>>>  {
>>>> diff --git a/mm/page-writeback.c b/mm/page-writeback.c
>>>> index 6f9b7b081ab7..ed3301753e89 100644
>>>> --- a/mm/page-writeback.c
>>>> +++ b/mm/page-writeback.c
>>>> @@ -2627,7 +2627,7 @@ static void folio_account_dirtied(struct folio *folio,
>>>>  
>>>>  		lruvec_stat_mod_folio(folio, NR_FILE_DIRTY, nr);
>>>>  		__zone_stat_mod_folio(folio, NR_ZONE_WRITE_PENDING, nr);
>>>> -		__node_stat_mod_folio(folio, NR_DIRTIED, nr);
>>>> +		node_stat_mod_folio(folio, NR_DIRTIED, nr);
>>>
>>> Hi Ye, thank you for the patch,
>>>
>>> In addition to what Matthew has pointed out, I also wanted to note that this
>>> substitution isn't trivial; there are differences between the __ prefixed
>>> version of node_stat_mod_folio and the one without. Even though the correctness
>>> of the two versions might be the same, I think that a change like this should
>>> be supplemented by a description of what side effects this change has
>>> (i.e. introducing additional overhead from the cmpxchg loop).
>>>
>>> Thank you, I hope you have a great day!
>>> Joshua
>>
>> Thank you for your review, Joshua. Regarding the difference between
>> __node_stat_mod_folio and node_stat_mod_folio: in the current implementation,
>> both functions ultimately use __mod_node_page_state, as mod_node_page_state
>> is defined as __mod_node_page_state. There is no functional difference
>> between them in terms of atomicity or overhead. The __ prefixed versions
>> were wrappers that are now unused, which is why we're removing them.
> 
> Hello Ye,
> 
> Thanks for the quick response. However, I'm not sure that's true.
> For #ifdef CONFIG_HAVE_CMPXCHG_LOCAL in mm/vmstat.c, I can see that
> mod_node_page_state calls mod_node_state, which includes the cmpxchg loop.
> I think many users enalbe this config, including x86. IMHO, we should document
> this effect of the changes.
> 
> Thank you, I hope you have a great day!
> Joshua

Thank you for your review, Joshua. You are correct that there is a difference.
The __node_stat_mod_folio call goes directly to __mod_node_page_state, which
assumes the caller already has the necessary serialization. By contrast,
node_stat_mod_folio goes through mod_node_page_state, which on systems with
CONFIG_HAVE_CMPXCHG_LOCAL uses a this_cpu_try_cmpxchg() loop and on other
systems serializes via local_irq_save/restore.

That means the wrapper removal is not purely cosmetic: it changes the
update path from a direct per-cpu diff update to a serializing update
path, and therefore incurs extra overhead.

-- 
Thanks,
Ye Liu