| Message ID | 20250410210623.1016767-1-shakeel.butt@linux.dev (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | memcg: decouple memcg_hotplug_cpu_dead from stock_lock | expand |
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!
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,
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;
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
(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.
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.
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.
(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.
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.
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
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?
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;
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(-)