[PATCH] memcg: decouple memcg_hotplug_cpu_dead from stock_lock

[PATCH] memcg: decouple memcg_hotplug_cpu_dead from stock_lock

[Shakeel Butt]

The function memcg_hotplug_cpu_dead works on the stock of a remote dead
CPU and drain_obj_stock works on the given stock instead of local stock,
so there is no need to take local stock_lock anymore.

Signed-off-by: Shakeel Butt <shakeel.butt@linux.dev>
---
 mm/memcontrol.c | 20 ++++++++++++--------
 1 file changed, 12 insertions(+), 8 deletions(-)

diff --git a/mm/memcontrol.c b/mm/memcontrol.c
index f23a4d0ad239..2178a051bd09 100644
--- a/mm/memcontrol.c
+++ b/mm/memcontrol.c
@@ -1789,7 +1789,7 @@ static DEFINE_PER_CPU(struct memcg_stock_pcp, memcg_stock) = {
 };
 static DEFINE_MUTEX(percpu_charge_mutex);
 
-static void drain_obj_stock(struct memcg_stock_pcp *stock);
+static void __drain_obj_stock(struct memcg_stock_pcp *stock);
 static bool obj_stock_flush_required(struct memcg_stock_pcp *stock,
 				     struct mem_cgroup *root_memcg);
 
@@ -1873,7 +1873,7 @@ static void drain_local_stock(struct work_struct *dummy)
 	local_lock_irqsave(&memcg_stock.stock_lock, flags);
 
 	stock = this_cpu_ptr(&memcg_stock);
-	drain_obj_stock(stock);
+	__drain_obj_stock(stock);
 	drain_stock(stock);
 	clear_bit(FLUSHING_CACHED_CHARGE, &stock->flags);
 
@@ -1964,10 +1964,10 @@ static int memcg_hotplug_cpu_dead(unsigned int cpu)
 
 	stock = &per_cpu(memcg_stock, cpu);
 
-	/* drain_obj_stock requires stock_lock */
-	local_lock_irqsave(&memcg_stock.stock_lock, flags);
-	drain_obj_stock(stock);
-	local_unlock_irqrestore(&memcg_stock.stock_lock, flags);
+	local_irq_save(flag);
+	/* stock of a remote dead cpu, no need for stock_lock. */
+	__drain_obj_stock(stock);
+	local_irq_restore(flag);
 
 	drain_stock(stock);
 
@@ -2837,7 +2837,11 @@ static bool consume_obj_stock(struct obj_cgroup *objcg, unsigned int nr_bytes,
 	return ret;
 }
 
-static void drain_obj_stock(struct memcg_stock_pcp *stock)
+/*
+ * Works on the given stock. The callers are responsible for the proper locking
+ * for the local or remote stocks.
+ */
+static void __drain_obj_stock(struct memcg_stock_pcp *stock)
 {
 	struct obj_cgroup *old = READ_ONCE(stock->cached_objcg);
 
@@ -2925,7 +2929,7 @@ static void refill_obj_stock(struct obj_cgroup *objcg, unsigned int nr_bytes,
 
 	stock = this_cpu_ptr(&memcg_stock);
 	if (READ_ONCE(stock->cached_objcg) != objcg) { /* reset if necessary */
-		drain_obj_stock(stock);
+		__drain_obj_stock(stock);
 		obj_cgroup_get(objcg);
 		stock->nr_bytes = atomic_read(&objcg->nr_charged_bytes)
 				? atomic_xchg(&objcg->nr_charged_bytes, 0) : 0;
-- 
2.47.1

Re: [PATCH] memcg: decouple memcg_hotplug_cpu_dead from stock_lock

[Roman Gushchin]

Shakeel Butt <shakeel.butt@linux.dev> writes:

> The function memcg_hotplug_cpu_dead works on the stock of a remote dead
> CPU and drain_obj_stock works on the given stock instead of local stock,
> so there is no need to take local stock_lock anymore.
>
> Signed-off-by: Shakeel Butt <shakeel.butt@linux.dev>

Acked-by: Roman Gushchin <roman.gushchin@linux.dev>

Thanks!

Re: [PATCH] memcg: decouple memcg_hotplug_cpu_dead from stock_lock

[Andrew Morton]

On Thu, 10 Apr 2025 14:06:23 -0700 Shakeel Butt <shakeel.butt@linux.dev> wrote:

> The function memcg_hotplug_cpu_dead works on the stock of a remote dead
> CPU and drain_obj_stock works on the given stock instead of local stock,
> so there is no need to take local stock_lock anymore.
> 
> @@ -1964,10 +1964,10 @@ static int memcg_hotplug_cpu_dead(unsigned int cpu)
>  
>  	stock = &per_cpu(memcg_stock, cpu);
>  
> -	/* drain_obj_stock requires stock_lock */
> -	local_lock_irqsave(&memcg_stock.stock_lock, flags);
> -	drain_obj_stock(stock);
> -	local_unlock_irqrestore(&memcg_stock.stock_lock, flags);
> +	local_irq_save(flag);
> +	/* stock of a remote dead cpu, no need for stock_lock. */
> +	__drain_obj_stock(stock);
> +	local_irq_restore(flag);
>  

s/flag/flags/

Obviously what-i-got isn't what-you-tested.  Please check what happened
here,

Re: [PATCH] memcg: decouple memcg_hotplug_cpu_dead from stock_lock

[Vlastimil Babka]

On 4/10/25 23:06, Shakeel Butt wrote:
> The function memcg_hotplug_cpu_dead works on the stock of a remote dead
> CPU and drain_obj_stock works on the given stock instead of local stock,
> so there is no need to take local stock_lock anymore.
> 
> Signed-off-by: Shakeel Butt <shakeel.butt@linux.dev>
> ---
>  mm/memcontrol.c | 20 ++++++++++++--------
>  1 file changed, 12 insertions(+), 8 deletions(-)
> 
> diff --git a/mm/memcontrol.c b/mm/memcontrol.c
> index f23a4d0ad239..2178a051bd09 100644
> --- a/mm/memcontrol.c
> +++ b/mm/memcontrol.c
> @@ -1789,7 +1789,7 @@ static DEFINE_PER_CPU(struct memcg_stock_pcp, memcg_stock) = {
>  };
>  static DEFINE_MUTEX(percpu_charge_mutex);
>  
> -static void drain_obj_stock(struct memcg_stock_pcp *stock);
> +static void __drain_obj_stock(struct memcg_stock_pcp *stock);
>  static bool obj_stock_flush_required(struct memcg_stock_pcp *stock,
>  				     struct mem_cgroup *root_memcg);
>  
> @@ -1873,7 +1873,7 @@ static void drain_local_stock(struct work_struct *dummy)
>  	local_lock_irqsave(&memcg_stock.stock_lock, flags);
>  
>  	stock = this_cpu_ptr(&memcg_stock);
> -	drain_obj_stock(stock);
> +	__drain_obj_stock(stock);
>  	drain_stock(stock);
>  	clear_bit(FLUSHING_CACHED_CHARGE, &stock->flags);
>  
> @@ -1964,10 +1964,10 @@ static int memcg_hotplug_cpu_dead(unsigned int cpu)
>  
>  	stock = &per_cpu(memcg_stock, cpu);
>  
> -	/* drain_obj_stock requires stock_lock */
> -	local_lock_irqsave(&memcg_stock.stock_lock, flags);
> -	drain_obj_stock(stock);
> -	local_unlock_irqrestore(&memcg_stock.stock_lock, flags);
> +	local_irq_save(flag);

I think for RT this is not great? At least in theory, probably it's not
actually used together with cpu hotplug? As it relies on memcg_stats_lock()
I think no irq save/enable is necessary there. local_lock_irqsave wasn't
actually a irq disable on RT. I don't know if there's a handy wrapper for this.

> +	/* stock of a remote dead cpu, no need for stock_lock. */
> +	__drain_obj_stock(stock);
> +	local_irq_restore(flag);
>  
>  	drain_stock(stock);
>  
> @@ -2837,7 +2837,11 @@ static bool consume_obj_stock(struct obj_cgroup *objcg, unsigned int nr_bytes,
>  	return ret;
>  }
>  
> -static void drain_obj_stock(struct memcg_stock_pcp *stock)
> +/*
> + * Works on the given stock. The callers are responsible for the proper locking
> + * for the local or remote stocks.
> + */
> +static void __drain_obj_stock(struct memcg_stock_pcp *stock)
>  {
>  	struct obj_cgroup *old = READ_ONCE(stock->cached_objcg);
>  
> @@ -2925,7 +2929,7 @@ static void refill_obj_stock(struct obj_cgroup *objcg, unsigned int nr_bytes,
>  
>  	stock = this_cpu_ptr(&memcg_stock);
>  	if (READ_ONCE(stock->cached_objcg) != objcg) { /* reset if necessary */
> -		drain_obj_stock(stock);
> +		__drain_obj_stock(stock);
>  		obj_cgroup_get(objcg);
>  		stock->nr_bytes = atomic_read(&objcg->nr_charged_bytes)
>  				? atomic_xchg(&objcg->nr_charged_bytes, 0) : 0;

Re: [PATCH] memcg: decouple memcg_hotplug_cpu_dead from stock_lock

[Sebastian Andrzej Siewior]

On 2025-04-11 10:55:31 [+0200], Vlastimil Babka wrote:
> > @@ -1964,10 +1964,10 @@ static int memcg_hotplug_cpu_dead(unsigned int cpu)
> >  
> >  	stock = &per_cpu(memcg_stock, cpu);
> >  
> > -	/* drain_obj_stock requires stock_lock */
> > -	local_lock_irqsave(&memcg_stock.stock_lock, flags);
> > -	drain_obj_stock(stock);
> > -	local_unlock_irqrestore(&memcg_stock.stock_lock, flags);
> > +	local_irq_save(flag);
> 
> I think for RT this is not great? At least in theory, probably it's not
> actually used together with cpu hotplug? As it relies on memcg_stats_lock()
> I think no irq save/enable is necessary there. local_lock_irqsave wasn't
> actually a irq disable on RT. I don't know if there's a handy wrapper for this.

No seeing the whole context but memcg_hotplug_cpu_dead() should be
invoked the control CPU while "cpu" is already gone. So the local_lock
should be acquired and the target CPU needs no locks since it is
offline. local_irq_save() will break things.

Sebastian

Re: [PATCH] memcg: decouple memcg_hotplug_cpu_dead from stock_lock

[Shakeel Butt]

(my migadu/linux.dev stopped working and I have to send through gmail,
sorry for any formatting issue)

On Fri, Apr 11, 2025 at 04:06:46PM +0200, Sebastian Andrzej Siewior wrote:
>
> On 2025-04-11 10:55:31 [+0200], Vlastimil Babka wrote:
>
>  > @@ -1964,10 +1964,10 @@ static int memcg_hotplug_cpu_dead(unsigned int cpu)
>
>  >
>
>  > stock = &per_cpu(memcg_stock, cpu);
>
>  >
>
>  > - /* drain_obj_stock requires stock_lock */
>
>  > - local_lock_irqsave(&memcg_stock.stock_lock, flags);
>
>  > - drain_obj_stock(stock);
>
>  > - local_unlock_irqrestore(&memcg_stock.stock_lock, flags);
>
>  > + local_irq_save(flag);
>
>  I think for RT this is not great?
>

This is not a performance critical function, so I would not worry about
that.

>
> At least in theory, probably it's not
>
>  actually used together with cpu hotplug? As it relies on memcg_stats_lock()
>
>  I think no irq save/enable is necessary there. local_lock_irqsave wasn't
>
>  actually a irq disable on RT. I don't know if there's a handy wrapper for this.
>
>  No seeing the whole context but memcg_hotplug_cpu_dead() should be
>
>  invoked the control CPU while "cpu" is already gone. So the local_lock
>
>  should be acquired and the target CPU needs no locks since it is
>
>  offline. local_irq_save() will break things.
>

I don't see how local_irq_save() will break anything. We are working on
a stock of a dead remote cpu. We actually don't even need to disable irq
or need local cpu's local_lock. It is actually the calls to
__mod_memcg_lruvec_state() and __mod_memcg_state() in
__drain_obj_stock() which need irq-disabled on non-RT kernels and for
RT-kernels they already have preempt_disable_nested().

Disabling irq even on RT seems excessive but this is not a performance
critical code, so I don't see an issue unless there is
local_lock_irqsave() alternative which does not disables irqs on RT
kernels.

Re: [PATCH] memcg: decouple memcg_hotplug_cpu_dead from stock_lock

[Vlastimil Babka]

On 4/11/25 19:54, Shakeel Butt wrote:
> (my migadu/linux.dev stopped working and I have to send through gmail,
> sorry for any formatting issue)
> 
> I don't see how local_irq_save() will break anything. We are working on
> a stock of a dead remote cpu. We actually don't even need to disable irq
> or need local cpu's local_lock. It is actually the calls to
> __mod_memcg_lruvec_state() and __mod_memcg_state() in
> __drain_obj_stock() which need irq-disabled on non-RT kernels and for
> RT-kernels they already have preempt_disable_nested().
> 
> Disabling irq even on RT seems excessive but this is not a performance
> critical code, so I don't see an issue unless there is
> local_lock_irqsave() alternative which does not disables irqs on RT
> kernels.

local_lock_irqsave() does not disable irqs on RT kernels :) so keeping
local_lock as is would do the irq disable on !RT and be more RT-friendly on
RT. It's just wrong from the logical scope of the lock to perform it on a
different cpu than the stock we modify. If one day we have some runtime
checks for that, they would complain.

Re: [PATCH] memcg: decouple memcg_hotplug_cpu_dead from stock_lock

[Shakeel Butt]

On Fri, Apr 11, 2025 at 2:06 PM Vlastimil Babka <vbabka@suse.cz> wrote:
>
> On 4/11/25 19:54, Shakeel Butt wrote:
> > (my migadu/linux.dev stopped working and I have to send through gmail,
> > sorry for any formatting issue)
> >
> > I don't see how local_irq_save() will break anything. We are working on
> > a stock of a dead remote cpu. We actually don't even need to disable irq
> > or need local cpu's local_lock. It is actually the calls to
> > __mod_memcg_lruvec_state() and __mod_memcg_state() in
> > __drain_obj_stock() which need irq-disabled on non-RT kernels and for
> > RT-kernels they already have preempt_disable_nested().
> >
> > Disabling irq even on RT seems excessive but this is not a performance
> > critical code, so I don't see an issue unless there is
> > local_lock_irqsave() alternative which does not disables irqs on RT
> > kernels.
>
> local_lock_irqsave() does not disable irqs on RT kernels :)

Sorry, I wanted to say local_irq_save() here instead of local_lock_irqsave().

> so keeping
> local_lock as is would do the irq disable on !RT and be more RT-friendly on
> RT. It's just wrong from the logical scope of the lock to perform it on a
> different cpu than the stock we modify. If one day we have some runtime
> checks for that, they would complain.

Basically I don't want to use stock_lock here. Maybe I should explore
adding a new local_lock for __mod_memcg_lruvec_state and
__mod_memcg_state.

Re: [PATCH] memcg: decouple memcg_hotplug_cpu_dead from stock_lock

[kernel test robot]

Hi Shakeel,

kernel test robot noticed the following build errors:

[auto build test ERROR on akpm-mm/mm-everything]
[cannot apply to linus/master v6.15-rc1 next-20250411]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Shakeel-Butt/memcg-decouple-memcg_hotplug_cpu_dead-from-stock_lock/20250411-050829
base:   https://git.kernel.org/pub/scm/linux/kernel/git/akpm/mm.git mm-everything
patch link:    https://lore.kernel.org/r/20250410210623.1016767-1-shakeel.butt%40linux.dev
patch subject: [PATCH] memcg: decouple memcg_hotplug_cpu_dead from stock_lock
config: x86_64-buildonly-randconfig-001-20250412 (https://download.01.org/0day-ci/archive/20250412/202504120739.K2ifymlR-lkp@intel.com/config)
compiler: gcc-12 (Debian 12.2.0-14) 12.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20250412/202504120739.K2ifymlR-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202504120739.K2ifymlR-lkp@intel.com/

All error/warnings (new ones prefixed by >>):

   In file included from include/linux/bitops.h:7,
                    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 'memcg_hotplug_cpu_dead':
>> mm/memcontrol.c:1967:24: error: 'flag' undeclared (first use in this function); did you mean 'flags'?
    1967 |         local_irq_save(flag);
         |                        ^~~~
   include/linux/typecheck.h:11:16: note: in definition of macro 'typecheck'
      11 |         typeof(x) __dummy2; \
         |                ^
   include/linux/irqflags.h:239:38: note: in expansion of macro 'raw_local_irq_save'
     239 | #define local_irq_save(flags)   do { raw_local_irq_save(flags); } while (0)
         |                                      ^~~~~~~~~~~~~~~~~~
   mm/memcontrol.c:1967:9: note: in expansion of macro 'local_irq_save'
    1967 |         local_irq_save(flag);
         |         ^~~~~~~~~~~~~~
   mm/memcontrol.c:1967:24: note: each undeclared identifier is reported only once for each function it appears in
    1967 |         local_irq_save(flag);
         |                        ^~~~
   include/linux/typecheck.h:11:16: note: in definition of macro 'typecheck'
      11 |         typeof(x) __dummy2; \
         |                ^
   include/linux/irqflags.h:239:38: note: in expansion of macro 'raw_local_irq_save'
     239 | #define local_irq_save(flags)   do { raw_local_irq_save(flags); } while (0)
         |                                      ^~~~~~~~~~~~~~~~~~
   mm/memcontrol.c:1967:9: note: in expansion of macro 'local_irq_save'
    1967 |         local_irq_save(flag);
         |         ^~~~~~~~~~~~~~
>> include/linux/typecheck.h:12:25: warning: comparison of distinct pointer types lacks a cast
      12 |         (void)(&__dummy == &__dummy2); \
         |                         ^~
   include/linux/irqflags.h:172:17: note: in expansion of macro 'typecheck'
     172 |                 typecheck(unsigned long, flags);        \
         |                 ^~~~~~~~~
   include/linux/irqflags.h:239:38: note: in expansion of macro 'raw_local_irq_save'
     239 | #define local_irq_save(flags)   do { raw_local_irq_save(flags); } while (0)
         |                                      ^~~~~~~~~~~~~~~~~~
   mm/memcontrol.c:1967:9: note: in expansion of macro 'local_irq_save'
    1967 |         local_irq_save(flag);
         |         ^~~~~~~~~~~~~~
>> include/linux/typecheck.h:12:25: warning: comparison of distinct pointer types lacks a cast
      12 |         (void)(&__dummy == &__dummy2); \
         |                         ^~
   include/linux/irqflags.h:177:17: note: in expansion of macro 'typecheck'
     177 |                 typecheck(unsigned long, flags);        \
         |                 ^~~~~~~~~
   include/linux/irqflags.h:240:39: note: in expansion of macro 'raw_local_irq_restore'
     240 | #define local_irq_restore(flags) do { raw_local_irq_restore(flags); } while (0)
         |                                       ^~~~~~~~~~~~~~~~~~~~~
   mm/memcontrol.c:1970:9: note: in expansion of macro 'local_irq_restore'
    1970 |         local_irq_restore(flag);
         |         ^~~~~~~~~~~~~~~~~
>> mm/memcontrol.c:1963:23: warning: unused variable 'flags' [-Wunused-variable]
    1963 |         unsigned long flags;
         |                       ^~~~~


vim +1967 mm/memcontrol.c

  1959	
  1960	static int memcg_hotplug_cpu_dead(unsigned int cpu)
  1961	{
  1962		struct memcg_stock_pcp *stock;
> 1963		unsigned long flags;
  1964	
  1965		stock = &per_cpu(memcg_stock, cpu);
  1966	
> 1967		local_irq_save(flag);
  1968		/* stock of a remote dead cpu, no need for stock_lock. */
  1969		__drain_obj_stock(stock);
  1970		local_irq_restore(flag);
  1971	
  1972		drain_stock(stock);
  1973	
  1974		return 0;
  1975	}
  1976	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH] memcg: decouple memcg_hotplug_cpu_dead from stock_lock

[kernel test robot]

Hi Shakeel,

kernel test robot noticed the following build errors:

[auto build test ERROR on akpm-mm/mm-everything]
[cannot apply to linus/master v6.15-rc1 next-20250411]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Shakeel-Butt/memcg-decouple-memcg_hotplug_cpu_dead-from-stock_lock/20250411-050829
base:   https://git.kernel.org/pub/scm/linux/kernel/git/akpm/mm.git mm-everything
patch link:    https://lore.kernel.org/r/20250410210623.1016767-1-shakeel.butt%40linux.dev
patch subject: [PATCH] memcg: decouple memcg_hotplug_cpu_dead from stock_lock
config: i386-buildonly-randconfig-002-20250412 (https://download.01.org/0day-ci/archive/20250412/202504120759.hMxys5DQ-lkp@intel.com/config)
compiler: clang version 20.1.2 (https://github.com/llvm/llvm-project 58df0ef89dd64126512e4ee27b4ac3fd8ddf6247)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20250412/202504120759.hMxys5DQ-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202504120759.hMxys5DQ-lkp@intel.com/

All errors (new ones prefixed by >>):

>> mm/memcontrol.c:1967:17: error: use of undeclared identifier 'flag'; did you mean 'flags'?
    1967 |         local_irq_save(flag);
         |                        ^~~~
         |                        flags
   include/linux/irqflags.h:216:22: note: expanded from macro 'local_irq_save'
     216 |                 raw_local_irq_save(flags);              \
         |                                    ^
   include/linux/irqflags.h:172:28: note: expanded from macro 'raw_local_irq_save'
     172 |                 typecheck(unsigned long, flags);        \
         |                                          ^
   include/linux/typecheck.h:11:9: note: expanded from macro 'typecheck'
      11 |         typeof(x) __dummy2; \
         |                ^
   mm/memcontrol.c:1963:16: note: 'flags' declared here
    1963 |         unsigned long flags;
         |                       ^
>> mm/memcontrol.c:1967:17: error: use of undeclared identifier 'flag'; did you mean 'flags'?
    1967 |         local_irq_save(flag);
         |                        ^~~~
         |                        flags
   include/linux/irqflags.h:216:22: note: expanded from macro 'local_irq_save'
     216 |                 raw_local_irq_save(flags);              \
         |                                    ^
   include/linux/irqflags.h:173:3: note: expanded from macro 'raw_local_irq_save'
     173 |                 flags = arch_local_irq_save();          \
         |                 ^
   mm/memcontrol.c:1963:16: note: 'flags' declared here
    1963 |         unsigned long flags;
         |                       ^
>> mm/memcontrol.c:1967:17: error: use of undeclared identifier 'flag'; did you mean 'flags'?
    1967 |         local_irq_save(flag);
         |                        ^~~~
         |                        flags
   include/linux/irqflags.h:217:32: note: expanded from macro 'local_irq_save'
     217 |                 if (!raw_irqs_disabled_flags(flags))    \
         |                                              ^
   include/linux/irqflags.h:188:28: note: expanded from macro 'raw_irqs_disabled_flags'
     188 |                 typecheck(unsigned long, flags);        \
         |                                          ^
   include/linux/typecheck.h:11:9: note: expanded from macro 'typecheck'
      11 |         typeof(x) __dummy2; \
         |                ^
   include/linux/compiler.h:55:47: note: expanded from macro 'if'
      55 | #define if(cond, ...) if ( __trace_if_var( !!(cond , ## __VA_ARGS__) ) )
         |                                               ^
   include/linux/compiler.h:57:52: note: expanded from macro '__trace_if_var'
      57 | #define __trace_if_var(cond) (__builtin_constant_p(cond) ? (cond) : __trace_if_value(cond))
         |                                                    ^
   mm/memcontrol.c:1963:16: note: 'flags' declared here
    1963 |         unsigned long flags;
         |                       ^
>> mm/memcontrol.c:1967:17: error: use of undeclared identifier 'flag'; did you mean 'flags'?
    1967 |         local_irq_save(flag);
         |                        ^~~~
         |                        flags
   include/linux/irqflags.h:217:32: note: expanded from macro 'local_irq_save'
     217 |                 if (!raw_irqs_disabled_flags(flags))    \
         |                                              ^
   include/linux/irqflags.h:189:28: note: expanded from macro 'raw_irqs_disabled_flags'
     189 |                 arch_irqs_disabled_flags(flags);        \
         |                                          ^
   include/linux/compiler.h:55:47: note: expanded from macro 'if'
      55 | #define if(cond, ...) if ( __trace_if_var( !!(cond , ## __VA_ARGS__) ) )
         |                                               ^
   include/linux/compiler.h:57:52: note: expanded from macro '__trace_if_var'
      57 | #define __trace_if_var(cond) (__builtin_constant_p(cond) ? (cond) : __trace_if_value(cond))
         |                                                    ^
   mm/memcontrol.c:1963:16: note: 'flags' declared here
    1963 |         unsigned long flags;
         |                       ^
>> mm/memcontrol.c:1967:17: error: use of undeclared identifier 'flag'; did you mean 'flags'?
    1967 |         local_irq_save(flag);
         |                        ^~~~
         |                        flags
   include/linux/irqflags.h:217:32: note: expanded from macro 'local_irq_save'
     217 |                 if (!raw_irqs_disabled_flags(flags))    \
         |                                              ^
   include/linux/irqflags.h:188:28: note: expanded from macro 'raw_irqs_disabled_flags'
     188 |                 typecheck(unsigned long, flags);        \
         |                                          ^
   include/linux/typecheck.h:11:9: note: expanded from macro 'typecheck'
      11 |         typeof(x) __dummy2; \
         |                ^
   include/linux/compiler.h:55:47: note: expanded from macro 'if'
      55 | #define if(cond, ...) if ( __trace_if_var( !!(cond , ## __VA_ARGS__) ) )
         |                                               ^
   include/linux/compiler.h:57:61: note: expanded from macro '__trace_if_var'
      57 | #define __trace_if_var(cond) (__builtin_constant_p(cond) ? (cond) : __trace_if_value(cond))
         |                                                             ^
   mm/memcontrol.c:1963:16: note: 'flags' declared here
    1963 |         unsigned long flags;
         |                       ^
>> mm/memcontrol.c:1967:17: error: use of undeclared identifier 'flag'; did you mean 'flags'?
    1967 |         local_irq_save(flag);
         |                        ^~~~
         |                        flags
   include/linux/irqflags.h:217:32: note: expanded from macro 'local_irq_save'
     217 |                 if (!raw_irqs_disabled_flags(flags))    \
         |                                              ^
   include/linux/irqflags.h:189:28: note: expanded from macro 'raw_irqs_disabled_flags'
     189 |                 arch_irqs_disabled_flags(flags);        \
         |                                          ^
   include/linux/compiler.h:55:47: note: expanded from macro 'if'
      55 | #define if(cond, ...) if ( __trace_if_var( !!(cond , ## __VA_ARGS__) ) )
         |                                               ^
   include/linux/compiler.h:57:61: note: expanded from macro '__trace_if_var'
      57 | #define __trace_if_var(cond) (__builtin_constant_p(cond) ? (cond) : __trace_if_value(cond))
         |                                                             ^
   mm/memcontrol.c:1963:16: note: 'flags' declared here
    1963 |         unsigned long flags;
         |                       ^
>> mm/memcontrol.c:1967:17: error: use of undeclared identifier 'flag'; did you mean 'flags'?
    1967 |         local_irq_save(flag);
         |                        ^~~~
         |                        flags
   include/linux/irqflags.h:217:32: note: expanded from macro 'local_irq_save'
     217 |                 if (!raw_irqs_disabled_flags(flags))    \
         |                                              ^
   include/linux/irqflags.h:188:28: note: expanded from macro 'raw_irqs_disabled_flags'
     188 |                 typecheck(unsigned long, flags);        \
         |                                          ^
   include/linux/typecheck.h:11:9: note: expanded from macro 'typecheck'
      11 |         typeof(x) __dummy2; \
         |                ^
   include/linux/compiler.h:55:47: note: expanded from macro 'if'
      55 | #define if(cond, ...) if ( __trace_if_var( !!(cond , ## __VA_ARGS__) ) )
         |                                               ^
   include/linux/compiler.h:57:86: note: expanded from macro '__trace_if_var'
      57 | #define __trace_if_var(cond) (__builtin_constant_p(cond) ? (cond) : __trace_if_value(cond))
         |                                                                                      ^
   include/linux/compiler.h:68:3: note: expanded from macro '__trace_if_value'
      68 |         (cond) ?                                        \
         |          ^
   mm/memcontrol.c:1963:16: note: 'flags' declared here
    1963 |         unsigned long flags;
         |                       ^
>> mm/memcontrol.c:1967:17: error: use of undeclared identifier 'flag'; did you mean 'flags'?
    1967 |         local_irq_save(flag);
         |                        ^~~~
         |                        flags
   include/linux/irqflags.h:217:32: note: expanded from macro 'local_irq_save'
     217 |                 if (!raw_irqs_disabled_flags(flags))    \
         |                                              ^
   include/linux/irqflags.h:189:28: note: expanded from macro 'raw_irqs_disabled_flags'
     189 |                 arch_irqs_disabled_flags(flags);        \
         |                                          ^
   include/linux/compiler.h:55:47: note: expanded from macro 'if'
      55 | #define if(cond, ...) if ( __trace_if_var( !!(cond , ## __VA_ARGS__) ) )
         |                                               ^
   include/linux/compiler.h:57:86: note: expanded from macro '__trace_if_var'
      57 | #define __trace_if_var(cond) (__builtin_constant_p(cond) ? (cond) : __trace_if_value(cond))
         |                                                                                      ^
   include/linux/compiler.h:68:3: note: expanded from macro '__trace_if_value'
      68 |         (cond) ?                                        \
         |          ^
   mm/memcontrol.c:1963:16: note: 'flags' declared here
    1963 |         unsigned long flags;
         |                       ^
   mm/memcontrol.c:1970:20: error: use of undeclared identifier 'flag'; did you mean 'flags'?
    1970 |         local_irq_restore(flag);
         |                           ^~~~
         |                           flags
   include/linux/irqflags.h:223:32: note: expanded from macro 'local_irq_restore'
     223 |                 if (!raw_irqs_disabled_flags(flags))    \
         |                                              ^
   include/linux/irqflags.h:188:28: note: expanded from macro 'raw_irqs_disabled_flags'
     188 |                 typecheck(unsigned long, flags);        \
         |                                          ^
   include/linux/typecheck.h:11:9: note: expanded from macro 'typecheck'
      11 |         typeof(x) __dummy2; \
         |                ^
   include/linux/compiler.h:55:47: note: expanded from macro 'if'
      55 | #define if(cond, ...) if ( __trace_if_var( !!(cond , ## __VA_ARGS__) ) )
         |                                               ^
   include/linux/compiler.h:57:52: note: expanded from macro '__trace_if_var'
      57 | #define __trace_if_var(cond) (__builtin_constant_p(cond) ? (cond) : __trace_if_value(cond))
         |                                                    ^
   mm/memcontrol.c:1963:16: note: 'flags' declared here
    1963 |         unsigned long flags;
         |                       ^
   mm/memcontrol.c:1970:20: error: use of undeclared identifier 'flag'; did you mean 'flags'?
    1970 |         local_irq_restore(flag);
         |                           ^~~~
         |                           flags
   include/linux/irqflags.h:223:32: note: expanded from macro 'local_irq_restore'
     223 |                 if (!raw_irqs_disabled_flags(flags))    \
         |                                              ^
   include/linux/irqflags.h:189:28: note: expanded from macro 'raw_irqs_disabled_flags'
     189 |                 arch_irqs_disabled_flags(flags);        \
         |                                          ^
   include/linux/compiler.h:55:47: note: expanded from macro 'if'
      55 | #define if(cond, ...) if ( __trace_if_var( !!(cond , ## __VA_ARGS__) ) )
         |                                               ^
   include/linux/compiler.h:57:52: note: expanded from macro '__trace_if_var'
      57 | #define __trace_if_var(cond) (__builtin_constant_p(cond) ? (cond) : __trace_if_value(cond))
         |                                                    ^
   mm/memcontrol.c:1963:16: note: 'flags' declared here
    1963 |         unsigned long flags;
         |                       ^
   mm/memcontrol.c:1970:20: error: use of undeclared identifier 'flag'; did you mean 'flags'?
    1970 |         local_irq_restore(flag);
         |                           ^~~~
         |                           flags
   include/linux/irqflags.h:223:32: note: expanded from macro 'local_irq_restore'
     223 |                 if (!raw_irqs_disabled_flags(flags))    \
         |                                              ^
   include/linux/irqflags.h:188:28: note: expanded from macro 'raw_irqs_disabled_flags'
     188 |                 typecheck(unsigned long, flags);        \
         |                                          ^
   include/linux/typecheck.h:11:9: note: expanded from macro 'typecheck'
      11 |         typeof(x) __dummy2; \
         |                ^
   include/linux/compiler.h:55:47: note: expanded from macro 'if'
      55 | #define if(cond, ...) if ( __trace_if_var( !!(cond , ## __VA_ARGS__) ) )
         |                                               ^
   include/linux/compiler.h:57:61: note: expanded from macro '__trace_if_var'
      57 | #define __trace_if_var(cond) (__builtin_constant_p(cond) ? (cond) : __trace_if_value(cond))
         |                                                             ^
   mm/memcontrol.c:1963:16: note: 'flags' declared here
    1963 |         unsigned long flags;
         |                       ^
   mm/memcontrol.c:1970:20: error: use of undeclared identifier 'flag'; did you mean 'flags'?
    1970 |         local_irq_restore(flag);
         |                           ^~~~
         |                           flags
   include/linux/irqflags.h:223:32: note: expanded from macro 'local_irq_restore'
     223 |                 if (!raw_irqs_disabled_flags(flags))    \
         |                                              ^
   include/linux/irqflags.h:189:28: note: expanded from macro 'raw_irqs_disabled_flags'
     189 |                 arch_irqs_disabled_flags(flags);        \
         |                                          ^
   include/linux/compiler.h:55:47: note: expanded from macro 'if'
      55 | #define if(cond, ...) if ( __trace_if_var( !!(cond , ## __VA_ARGS__) ) )
         |                                               ^
   include/linux/compiler.h:57:61: note: expanded from macro '__trace_if_var'
      57 | #define __trace_if_var(cond) (__builtin_constant_p(cond) ? (cond) : __trace_if_value(cond))
         |                                                             ^


vim +1967 mm/memcontrol.c

  1959	
  1960	static int memcg_hotplug_cpu_dead(unsigned int cpu)
  1961	{
  1962		struct memcg_stock_pcp *stock;
  1963		unsigned long flags;
  1964	
  1965		stock = &per_cpu(memcg_stock, cpu);
  1966	
> 1967		local_irq_save(flag);
  1968		/* stock of a remote dead cpu, no need for stock_lock. */
  1969		__drain_obj_stock(stock);
  1970		local_irq_restore(flag);
  1971	
  1972		drain_stock(stock);
  1973	
  1974		return 0;
  1975	}
  1976	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH] memcg: decouple memcg_hotplug_cpu_dead from stock_lock

[Shakeel Butt]

(I was trying to reply last week but my email stopped working, so trying
again)

On Thu, Apr 10, 2025 at 10:06:18PM -0700, Andrew Morton wrote:
> On Thu, 10 Apr 2025 14:06:23 -0700 Shakeel Butt <shakeel.butt@linux.dev> wrote:
> 
> > The function memcg_hotplug_cpu_dead works on the stock of a remote dead
> > CPU and drain_obj_stock works on the given stock instead of local stock,
> > so there is no need to take local stock_lock anymore.
> > 
> > @@ -1964,10 +1964,10 @@ static int memcg_hotplug_cpu_dead(unsigned int cpu)
> >  
> >  	stock = &per_cpu(memcg_stock, cpu);
> >  
> > -	/* drain_obj_stock requires stock_lock */
> > -	local_lock_irqsave(&memcg_stock.stock_lock, flags);
> > -	drain_obj_stock(stock);
> > -	local_unlock_irqrestore(&memcg_stock.stock_lock, flags);
> > +	local_irq_save(flag);
> > +	/* stock of a remote dead cpu, no need for stock_lock. */
> > +	__drain_obj_stock(stock);
> > +	local_irq_restore(flag);
> >  
> 
> s/flag/flags/
> 
> Obviously what-i-got isn't what-you-tested.  Please check what happened
> here,

Sorry about that. I tested the previous version where I had
drain_obj_stock() as a separate function but after seeing that there is
just one caller, I manually inlined it but forgot to test before
sending.

Re: [PATCH] memcg: decouple memcg_hotplug_cpu_dead from stock_lock

[Shakeel Butt]

On Fri, Apr 11, 2025 at 02:12:46PM -0400, Shakeel Butt wrote:
> On Fri, Apr 11, 2025 at 2:06 PM Vlastimil Babka <vbabka@suse.cz> wrote:
> >
> > On 4/11/25 19:54, Shakeel Butt wrote:
> > > (my migadu/linux.dev stopped working and I have to send through gmail,
> > > sorry for any formatting issue)
> > >
> > > I don't see how local_irq_save() will break anything. We are working on
> > > a stock of a dead remote cpu. We actually don't even need to disable irq
> > > or need local cpu's local_lock. It is actually the calls to
> > > __mod_memcg_lruvec_state() and __mod_memcg_state() in
> > > __drain_obj_stock() which need irq-disabled on non-RT kernels and for
> > > RT-kernels they already have preempt_disable_nested().
> > >
> > > Disabling irq even on RT seems excessive but this is not a performance
> > > critical code, so I don't see an issue unless there is
> > > local_lock_irqsave() alternative which does not disables irqs on RT
> > > kernels.
> >
> > local_lock_irqsave() does not disable irqs on RT kernels :)
> 
> Sorry, I wanted to say local_irq_save() here instead of local_lock_irqsave().
> 
> > so keeping
> > local_lock as is would do the irq disable on !RT and be more RT-friendly on
> > RT. It's just wrong from the logical scope of the lock to perform it on a
> > different cpu than the stock we modify. If one day we have some runtime
> > checks for that, they would complain.
> 
> Basically I don't want to use stock_lock here. Maybe I should explore
> adding a new local_lock for __mod_memcg_lruvec_state and
> __mod_memcg_state.

Vlastimil & Sebastian, if you don't have a strong opinion/push-back on
this patch then I will keep it as is. However I am planning to rework
the memcg stats (& vmstats) to see if I can use dedicated local_lock for
them and able to modify them in any context.

Re: [PATCH] memcg: decouple memcg_hotplug_cpu_dead from stock_lock

[Sebastian Andrzej Siewior]

On 2025-04-14 10:55:31 [-0700], Shakeel Butt wrote:
> Vlastimil & Sebastian, if you don't have a strong opinion/push-back on
> this patch then I will keep it as is. However I am planning to rework
> the memcg stats (& vmstats) to see if I can use dedicated local_lock for
> them and able to modify them in any context.

Please don't use local_irq_save().

Sebastian

Re: [PATCH] memcg: decouple memcg_hotplug_cpu_dead from stock_lock

[Shakeel Butt]

On Tue, Apr 15, 2025 at 08:30:54AM +0200, Sebastian Andrzej Siewior wrote:
> On 2025-04-14 10:55:31 [-0700], Shakeel Butt wrote:
> > Vlastimil & Sebastian, if you don't have a strong opinion/push-back on
> > this patch then I will keep it as is. However I am planning to rework
> > the memcg stats (& vmstats) to see if I can use dedicated local_lock for
> > them and able to modify them in any context.
> 
> Please don't use local_irq_save().

Sounds good. Andrew, can you please drop this patch (I think it was
picked into mm-new).

BTW I think using local_irq_save() is not wrong and just not preferred
for RT kernel, is that correct?