Re: [PATCH 1/4] memcg: use mod_node_page_state to update stats

[Dev Jain <dev.jain@arm.com>]

On 11/11/25 4:50 am, Shakeel Butt wrote:
> The memcg stats are safe against irq (and nmi) context and thus does not
> require disabling irqs. However some code paths for memcg stats also
> update the node level stats and use irq unsafe interface and thus
> require the users to disable irqs. However node level stats, on
> architectures with HAVE_CMPXCHG_LOCAL (all major ones), has interface
> which does not require irq disabling. Let's move memcg stats code to
> start using that interface for node level stats.
>
> Signed-off-by: Shakeel Butt <shakeel.butt@linux.dev>
> ---

Hello Shakeel,

We are seeing a regression in micromm/munmap benchmark with this patch, on arm64 -
the benchmark mmmaps a lot of memory, memsets it, and measures the time taken
to munmap. Please see below if my understanding of this patch is correct.

>  include/linux/memcontrol.h | 2 +-
>  include/linux/vmstat.h     | 4 ++--
>  mm/memcontrol.c            | 6 +++---
>  3 files changed, 6 insertions(+), 6 deletions(-)
>
> diff --git a/include/linux/memcontrol.h b/include/linux/memcontrol.h
> index 8c0f15e5978f..f82fac2fd988 100644
> --- a/include/linux/memcontrol.h
> +++ b/include/linux/memcontrol.h
> @@ -1408,7 +1408,7 @@ static inline void __mod_lruvec_kmem_state(void *p, enum node_stat_item idx,
>  {
>  	struct page *page = virt_to_head_page(p);
>  
> -	__mod_node_page_state(page_pgdat(page), idx, val);
> +	mod_node_page_state(page_pgdat(page), idx, val);
>  }
>  
>  static inline void mod_lruvec_kmem_state(void *p, enum node_stat_item idx,
> diff --git a/include/linux/vmstat.h b/include/linux/vmstat.h
> index c287998908bf..11a37aaa4dd9 100644
> --- a/include/linux/vmstat.h
> +++ b/include/linux/vmstat.h
> @@ -557,7 +557,7 @@ static inline void mod_lruvec_page_state(struct page *page,
>  static inline void __mod_lruvec_state(struct lruvec *lruvec,
>  				      enum node_stat_item idx, int val)
>  {
> -	__mod_node_page_state(lruvec_pgdat(lruvec), idx, val);
> +	mod_node_page_state(lruvec_pgdat(lruvec), idx, val);
>  }
>  
>  static inline void mod_lruvec_state(struct lruvec *lruvec,
> @@ -569,7 +569,7 @@ static inline void mod_lruvec_state(struct lruvec *lruvec,
>  static inline void __lruvec_stat_mod_folio(struct folio *folio,
>  					 enum node_stat_item idx, int val)
>  {
> -	__mod_node_page_state(folio_pgdat(folio), idx, val);
> +	mod_node_page_state(folio_pgdat(folio), idx, val);
>  }

See folio_remove_rmap_ptes -> __folio_mod_stat -> __lruvec_stat_mod_folio. This path now
has the unconditional overhead of doing this_cpu_try_cmpxchg(). AFAIU the purpose of
this patch was to remove local_irq_save and optimize it by using a cmpxchg atomic
(coupled with the fact that the caller will have ensured preempt_disable), but
there are code paths which are not doing local_irq_save in the first place, so
those get regressed.

>  
>  static inline void lruvec_stat_mod_folio(struct folio *folio,
> diff --git a/mm/memcontrol.c b/mm/memcontrol.c
> index 025da46d9959..f4b8a6414ed3 100644
> --- a/mm/memcontrol.c
> +++ b/mm/memcontrol.c
> @@ -770,7 +770,7 @@ void __mod_lruvec_state(struct lruvec *lruvec, enum node_stat_item idx,
>  			int val)
>  {
>  	/* Update node */
> -	__mod_node_page_state(lruvec_pgdat(lruvec), idx, val);
> +	mod_node_page_state(lruvec_pgdat(lruvec), idx, val);
>  
>  	/* Update memcg and lruvec */
>  	if (!mem_cgroup_disabled())
> @@ -789,7 +789,7 @@ void __lruvec_stat_mod_folio(struct folio *folio, enum node_stat_item idx,
>  	/* Untracked pages have no memcg, no lruvec. Update only the node */
>  	if (!memcg) {
>  		rcu_read_unlock();
> -		__mod_node_page_state(pgdat, idx, val);
> +		mod_node_page_state(pgdat, idx, val);
>  		return;
>  	}
>  
> @@ -815,7 +815,7 @@ void __mod_lruvec_kmem_state(void *p, enum node_stat_item idx, int val)
>  	 * vmstats to keep it correct for the root memcg.
>  	 */
>  	if (!memcg) {
> -		__mod_node_page_state(pgdat, idx, val);
> +		mod_node_page_state(pgdat, idx, val);
>  	} else {
>  		lruvec = mem_cgroup_lruvec(memcg, pgdat);
>  		__mod_lruvec_state(lruvec, idx, val);