Re: [PATCH v2 1/7] memcg: memcg_rstat_updated re-entrant safe against irqs

[Lorenzo Stoakes <lorenzo.stoakes@oracle.com>]

Shakeel - This breaks the build in mm-new for me:

  CC      mm/pt_reclaim.o
In file included from ./arch/x86/include/asm/rmwcc.h:5,
                 from ./arch/x86/include/asm/bitops.h:18,
                 from ./include/linux/bitops.h:68,
                 from ./include/linux/radix-tree.h:11,
                 from ./include/linux/idr.h:15,
                 from ./include/linux/cgroup-defs.h:13,
                 from mm/memcontrol.c:28:
mm/memcontrol.c: In function ‘mem_cgroup_alloc’:
./arch/x86/include/asm/percpu.h:39:45: error: expected identifier or ‘(’ before ‘__seg_gs’
   39 | #define __percpu_seg_override   CONCATENATE(__seg_, __percpu_seg)
      |                                             ^~~~~~
./include/linux/args.h:25:24: note: in definition of macro ‘__CONCAT’
   25 | #define __CONCAT(a, b) a ## b
      |                        ^
./arch/x86/include/asm/percpu.h:39:33: note: in expansion of macro ‘CONCATENATE’
   39 | #define __percpu_seg_override   CONCATENATE(__seg_, __percpu_seg)
      |                                 ^~~~~~~~~~~
./arch/x86/include/asm/percpu.h:93:33: note: in expansion of macro ‘__percpu_seg_override’
   93 | # define __percpu_qual          __percpu_seg_override
      |                                 ^~~~~~~~~~~~~~~~~~~~~
././include/linux/compiler_types.h:60:25: note: in expansion of macro ‘__percpu_qual’
   60 | # define __percpu       __percpu_qual BTF_TYPE_TAG(percpu)
      |                         ^~~~~~~~~~~~~
mm/memcontrol.c:3700:45: note: in expansion of macro ‘__percpu’
 3700 |         struct memcg_vmstats_percpu *statc, __percpu *pstatc_pcpu;
      |                                             ^~~~~~~~
mm/memcontrol.c:3731:25: error: ‘pstatc_pcpu’ undeclared (first use in this function); did you mean ‘kstat_cpu’?
 3731 |                         pstatc_pcpu = parent->vmstats_percpu;
      |                         ^~~~~~~~~~~
      |                         kstat_cpu
mm/memcontrol.c:3731:25: note: each undeclared identifier is reported only once for each function it appears in

The __percpu macro seems to be a bit screwy with comma-delimited decls, as it
seems that putting this on its own line fixes this problem:


From 28275e5d054506746d310cf5ebd1fafdb0881dba Mon Sep 17 00:00:00 2001
From: Lorenzo Stoakes <lorenzo.stoakes@oracle.com>
Date: Thu, 15 May 2025 13:43:46 +0100
Subject: [PATCH] fix

Signed-off-by: Lorenzo Stoakes <lorenzo.stoakes@oracle.com>
---
 mm/memcontrol.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/mm/memcontrol.c b/mm/memcontrol.c
index 2464a58fbf17..40fcc2259e5f 100644
--- a/mm/memcontrol.c
+++ b/mm/memcontrol.c
@@ -3697,7 +3697,8 @@ static void mem_cgroup_free(struct mem_cgroup *memcg)

 static struct mem_cgroup *mem_cgroup_alloc(struct mem_cgroup *parent)
 {
-	struct memcg_vmstats_percpu *statc, __percpu *pstatc_pcpu;
+	struct memcg_vmstats_percpu *statc;
+	struct memcg_vmstats_percpu __percpu *pstatc_pcpu;
 	struct mem_cgroup *memcg;
 	int node, cpu;
 	int __maybe_unused i;
--
2.49.0


I have duplicated this again at the end of this mail for easy application.

Could we get this fix in or drop the series so the build is fixed for
mm-new? Thanks!


On Wed, May 14, 2025 at 11:41:52AM -0700, Shakeel Butt wrote:
> The function memcg_rstat_updated() is used to track the memcg stats
> updates for optimizing the flushes. At the moment, it is not re-entrant
> safe and the callers disabled irqs before calling. However to achieve
> the goal of updating memcg stats without irqs, memcg_rstat_updated()
> needs to be re-entrant safe against irqs.
>
> This patch makes memcg_rstat_updated() re-entrant safe using this_cpu_*
> ops. On archs with CONFIG_ARCH_HAS_NMI_SAFE_THIS_CPU_OPS, this patch is
> also making memcg_rstat_updated() nmi safe.
>
> Signed-off-by: Shakeel Butt <shakeel.butt@linux.dev>
> Reviewed-by: Vlastimil Babka <vbabka@suse.cz>
> ---
>  mm/memcontrol.c | 28 +++++++++++++++++-----------
>  1 file changed, 17 insertions(+), 11 deletions(-)
>
> diff --git a/mm/memcontrol.c b/mm/memcontrol.c
> index 89476a71a18d..2464a58fbf17 100644
> --- a/mm/memcontrol.c
> +++ b/mm/memcontrol.c
> @@ -505,8 +505,8 @@ struct memcg_vmstats_percpu {
>  	unsigned int			stats_updates;
>
>  	/* Cached pointers for fast iteration in memcg_rstat_updated() */
> -	struct memcg_vmstats_percpu	*parent;
> -	struct memcg_vmstats		*vmstats;
> +	struct memcg_vmstats_percpu __percpu	*parent_pcpu;
> +	struct memcg_vmstats			*vmstats;
>
>  	/* The above should fit a single cacheline for memcg_rstat_updated() */
>
> @@ -588,16 +588,21 @@ static bool memcg_vmstats_needs_flush(struct memcg_vmstats *vmstats)
>
>  static inline void memcg_rstat_updated(struct mem_cgroup *memcg, int val)
>  {
> +	struct memcg_vmstats_percpu __percpu *statc_pcpu;
>  	struct memcg_vmstats_percpu *statc;
> -	int cpu = smp_processor_id();
> +	int cpu;
>  	unsigned int stats_updates;
>
>  	if (!val)
>  		return;
>
> +	/* Don't assume callers have preemption disabled. */
> +	cpu = get_cpu();
> +
>  	cgroup_rstat_updated(memcg->css.cgroup, cpu);
> -	statc = this_cpu_ptr(memcg->vmstats_percpu);
> -	for (; statc; statc = statc->parent) {
> +	statc_pcpu = memcg->vmstats_percpu;
> +	for (; statc_pcpu; statc_pcpu = statc->parent_pcpu) {
> +		statc = this_cpu_ptr(statc_pcpu);
>  		/*
>  		 * If @memcg is already flushable then all its ancestors are
>  		 * flushable as well and also there is no need to increase
> @@ -606,14 +611,15 @@ static inline void memcg_rstat_updated(struct mem_cgroup *memcg, int val)
>  		if (memcg_vmstats_needs_flush(statc->vmstats))
>  			break;
>
> -		stats_updates = READ_ONCE(statc->stats_updates) + abs(val);
> -		WRITE_ONCE(statc->stats_updates, stats_updates);
> +		stats_updates = this_cpu_add_return(statc_pcpu->stats_updates,
> +						    abs(val));
>  		if (stats_updates < MEMCG_CHARGE_BATCH)
>  			continue;
>
> +		stats_updates = this_cpu_xchg(statc_pcpu->stats_updates, 0);
>  		atomic64_add(stats_updates, &statc->vmstats->stats_updates);
> -		WRITE_ONCE(statc->stats_updates, 0);
>  	}
> +	put_cpu();
>  }
>
>  static void __mem_cgroup_flush_stats(struct mem_cgroup *memcg, bool force)
> @@ -3691,7 +3697,7 @@ static void mem_cgroup_free(struct mem_cgroup *memcg)
>
>  static struct mem_cgroup *mem_cgroup_alloc(struct mem_cgroup *parent)
>  {
> -	struct memcg_vmstats_percpu *statc, *pstatc;
> +	struct memcg_vmstats_percpu *statc, __percpu *pstatc_pcpu;
>  	struct mem_cgroup *memcg;
>  	int node, cpu;
>  	int __maybe_unused i;
> @@ -3722,9 +3728,9 @@ static struct mem_cgroup *mem_cgroup_alloc(struct mem_cgroup *parent)
>
>  	for_each_possible_cpu(cpu) {
>  		if (parent)
> -			pstatc = per_cpu_ptr(parent->vmstats_percpu, cpu);
> +			pstatc_pcpu = parent->vmstats_percpu;
>  		statc = per_cpu_ptr(memcg->vmstats_percpu, cpu);
> -		statc->parent = parent ? pstatc : NULL;
> +		statc->parent_pcpu = parent ? pstatc_pcpu : NULL;
>  		statc->vmstats = memcg->vmstats;
>  	}
>
> --
> 2.47.1
>
>

----8<----
From 28275e5d054506746d310cf5ebd1fafdb0881dba Mon Sep 17 00:00:00 2001
From: Lorenzo Stoakes <lorenzo.stoakes@oracle.com>
Date: Thu, 15 May 2025 13:43:46 +0100
Subject: [PATCH] fix

Signed-off-by: Lorenzo Stoakes <lorenzo.stoakes@oracle.com>
---
 mm/memcontrol.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/mm/memcontrol.c b/mm/memcontrol.c
index 2464a58fbf17..40fcc2259e5f 100644
--- a/mm/memcontrol.c
+++ b/mm/memcontrol.c
@@ -3697,7 +3697,8 @@ static void mem_cgroup_free(struct mem_cgroup *memcg)

 static struct mem_cgroup *mem_cgroup_alloc(struct mem_cgroup *parent)
 {
-	struct memcg_vmstats_percpu *statc, __percpu *pstatc_pcpu;
+	struct memcg_vmstats_percpu *statc;
+	struct memcg_vmstats_percpu __percpu *pstatc_pcpu;
 	struct mem_cgroup *memcg;
 	int node, cpu;
 	int __maybe_unused i;
--
2.49.0