[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

[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?