| Message ID | 20250315174930.1769599-8-shakeel.butt@linux.dev (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | memcg: cleanup per-cpu stock | expand |
On 3/15/25 18:49, Shakeel Butt wrote: > For non-PREEMPT_RT kernels, drain_obj_stock() is always called with irq > disabled, so we can use __mod_memcg_state() instead of > mod_memcg_state(). For PREEMPT_RT, we need to add memcg_stats_[un]lock > in __mod_memcg_state(). > > Signed-off-by: Shakeel Butt <shakeel.butt@linux.dev> > Reviewed-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de> I've asked in the RFC and from Sebastian's answer I think my question was misunderstod, so let me try again. After this patch we'll have from mod_memcg_state(): mod_memcg_state() local_irq_save(flags); __mod_memcg_state() memcg_stats_lock(); <- new and unnecessary? Instead of modifying __mod_memcg_state() we could be more targetted and just do in drain_obj_stock(): memcg_stats_lock(); __mod_memcg_state(); memcg_stats_unlock(); Am I missing something? > --- > mm/memcontrol.c | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > > diff --git a/mm/memcontrol.c b/mm/memcontrol.c > index 3c4de384b5a0..dfe9c2eb7816 100644 > --- a/mm/memcontrol.c > +++ b/mm/memcontrol.c > @@ -707,10 +707,12 @@ void __mod_memcg_state(struct mem_cgroup *memcg, enum memcg_stat_item idx, > if (WARN_ONCE(BAD_STAT_IDX(i), "%s: missing stat item %d\n", __func__, idx)) > return; > > + memcg_stats_lock(); > __this_cpu_add(memcg->vmstats_percpu->state[i], val); > val = memcg_state_val_in_pages(idx, val); > memcg_rstat_updated(memcg, val); > trace_mod_memcg_state(memcg, idx, val); > + memcg_stats_unlock(); > } > > #ifdef CONFIG_MEMCG_V1 > @@ -2845,7 +2847,7 @@ static void drain_obj_stock(struct memcg_stock_pcp *stock) > > memcg = get_mem_cgroup_from_objcg(old); > > - mod_memcg_state(memcg, MEMCG_KMEM, -nr_pages); > + __mod_memcg_state(memcg, MEMCG_KMEM, -nr_pages); > memcg1_account_kmem(memcg, -nr_pages); > if (!mem_cgroup_is_root(memcg)) > memcg_uncharge(memcg, nr_pages);
On Mon, Mar 17, 2025 at 09:56:39PM +0100, Vlastimil Babka wrote: > On 3/15/25 18:49, Shakeel Butt wrote: > > For non-PREEMPT_RT kernels, drain_obj_stock() is always called with irq > > disabled, so we can use __mod_memcg_state() instead of > > mod_memcg_state(). For PREEMPT_RT, we need to add memcg_stats_[un]lock > > in __mod_memcg_state(). > > > > Signed-off-by: Shakeel Butt <shakeel.butt@linux.dev> > > Reviewed-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de> > > I've asked in the RFC and from Sebastian's answer I think my question was > misunderstod, so let me try again. > > After this patch we'll have from mod_memcg_state(): > > mod_memcg_state() > local_irq_save(flags); > __mod_memcg_state() > memcg_stats_lock(); <- new and unnecessary? > > Instead of modifying __mod_memcg_state() we could be more targetted and just > do in drain_obj_stock(): > > memcg_stats_lock(); > __mod_memcg_state(); > memcg_stats_unlock(); > > Am I missing something? This seems unnecessary because this patch is adding the first user of __mod_memcg_state() but I think maintainability is better with memcg_stats_[un]lock() inside __mod_memcg_state(). Let's take the example of __mod_memcg_lruvec_state(). It is being called from places where non-RT kernel, the irqs are disabled through spin_lock_irq*, so on RT kernel, the irq would not be disabled and thus explicitly need preemption disabled. What if in future __mod_memcg_state() is being used by a caller which assumes preemption is disabled through irq disable. The RT kernel would be buggy there. I am not sure if it is easy to force the future users to explicitly add memcg_stats_[un]lock() across the call to __mod_memcg_state().
On Sat, Mar 15, 2025 at 10:49:28AM -0700, Shakeel Butt wrote: > For non-PREEMPT_RT kernels, drain_obj_stock() is always called with irq > disabled, so we can use __mod_memcg_state() instead of > mod_memcg_state(). For PREEMPT_RT, we need to add memcg_stats_[un]lock > in __mod_memcg_state(). > > Signed-off-by: Shakeel Butt <shakeel.butt@linux.dev> > Reviewed-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de> > --- > mm/memcontrol.c | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > > diff --git a/mm/memcontrol.c b/mm/memcontrol.c > index 3c4de384b5a0..dfe9c2eb7816 100644 > --- a/mm/memcontrol.c > +++ b/mm/memcontrol.c > @@ -707,10 +707,12 @@ void __mod_memcg_state(struct mem_cgroup *memcg, enum memcg_stat_item idx, > if (WARN_ONCE(BAD_STAT_IDX(i), "%s: missing stat item %d\n", __func__, idx)) > return; > > + memcg_stats_lock(); > __this_cpu_add(memcg->vmstats_percpu->state[i], val); > val = memcg_state_val_in_pages(idx, val); > memcg_rstat_updated(memcg, val); > trace_mod_memcg_state(memcg, idx, val); > + memcg_stats_unlock(); > } > > #ifdef CONFIG_MEMCG_V1 > @@ -2845,7 +2847,7 @@ static void drain_obj_stock(struct memcg_stock_pcp *stock) VM_WARN_ON_IRQS_ENABLED() ? Reviewed-by: Roman Gushchin <roman.gushchin@linux.dev>
On 3/18/25 02:13, Roman Gushchin wrote: > On Sat, Mar 15, 2025 at 10:49:28AM -0700, Shakeel Butt wrote: >> For non-PREEMPT_RT kernels, drain_obj_stock() is always called with irq >> disabled, so we can use __mod_memcg_state() instead of >> mod_memcg_state(). For PREEMPT_RT, we need to add memcg_stats_[un]lock >> in __mod_memcg_state(). >> >> Signed-off-by: Shakeel Butt <shakeel.butt@linux.dev> >> Reviewed-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de> >> --- >> mm/memcontrol.c | 4 +++- >> 1 file changed, 3 insertions(+), 1 deletion(-) >> >> diff --git a/mm/memcontrol.c b/mm/memcontrol.c >> index 3c4de384b5a0..dfe9c2eb7816 100644 >> --- a/mm/memcontrol.c >> +++ b/mm/memcontrol.c >> @@ -707,10 +707,12 @@ void __mod_memcg_state(struct mem_cgroup *memcg, enum memcg_stat_item idx, >> if (WARN_ONCE(BAD_STAT_IDX(i), "%s: missing stat item %d\n", __func__, idx)) >> return; >> >> + memcg_stats_lock(); >> __this_cpu_add(memcg->vmstats_percpu->state[i], val); >> val = memcg_state_val_in_pages(idx, val); >> memcg_rstat_updated(memcg, val); >> trace_mod_memcg_state(memcg, idx, val); >> + memcg_stats_unlock(); >> } >> >> #ifdef CONFIG_MEMCG_V1 >> @@ -2845,7 +2847,7 @@ static void drain_obj_stock(struct memcg_stock_pcp *stock) > > VM_WARN_ON_IRQS_ENABLED() ? It's part of memcg_stats_lock() > Reviewed-by: Roman Gushchin <roman.gushchin@linux.dev>
On 3/17/25 22:54, Shakeel Butt wrote: > On Mon, Mar 17, 2025 at 09:56:39PM +0100, Vlastimil Babka wrote: >> On 3/15/25 18:49, Shakeel Butt wrote: >> > For non-PREEMPT_RT kernels, drain_obj_stock() is always called with irq >> > disabled, so we can use __mod_memcg_state() instead of >> > mod_memcg_state(). For PREEMPT_RT, we need to add memcg_stats_[un]lock >> > in __mod_memcg_state(). >> > >> > Signed-off-by: Shakeel Butt <shakeel.butt@linux.dev> >> > Reviewed-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de> >> >> I've asked in the RFC and from Sebastian's answer I think my question was >> misunderstod, so let me try again. >> >> After this patch we'll have from mod_memcg_state(): >> >> mod_memcg_state() >> local_irq_save(flags); >> __mod_memcg_state() >> memcg_stats_lock(); <- new and unnecessary? >> >> Instead of modifying __mod_memcg_state() we could be more targetted and just >> do in drain_obj_stock(): >> >> memcg_stats_lock(); >> __mod_memcg_state(); >> memcg_stats_unlock(); >> >> Am I missing something? > > This seems unnecessary because this patch is adding the first user of > __mod_memcg_state() You mean first other user than mod_memcg_state() itself. > but I think maintainability is better with > memcg_stats_[un]lock() inside __mod_memcg_state(). > > Let's take the example of __mod_memcg_lruvec_state(). It is being > called from places where non-RT kernel, the irqs are disabled through > spin_lock_irq*, so on RT kernel, the irq would not be disabled and > thus explicitly need preemption disabled. What if in future > __mod_memcg_state() is being used by a caller which assumes preemption > is disabled through irq disable. The RT kernel would be buggy there. > > I am not sure if it is easy to force the future users to explicitly add > memcg_stats_[un]lock() across the call to __mod_memcg_state(). I see the point. Well least memcg_stats_lock() isn't expensive, and it's a no-op on non-debug !RT anyway. Acked-by: Vlastimil Babka <vbabka@suse.cz>
diff --git a/mm/memcontrol.c b/mm/memcontrol.c index 3c4de384b5a0..dfe9c2eb7816 100644 --- a/mm/memcontrol.c +++ b/mm/memcontrol.c @@ -707,10 +707,12 @@ void __mod_memcg_state(struct mem_cgroup *memcg, enum memcg_stat_item idx, if (WARN_ONCE(BAD_STAT_IDX(i), "%s: missing stat item %d\n", __func__, idx)) return; + memcg_stats_lock(); __this_cpu_add(memcg->vmstats_percpu->state[i], val); val = memcg_state_val_in_pages(idx, val); memcg_rstat_updated(memcg, val); trace_mod_memcg_state(memcg, idx, val); + memcg_stats_unlock(); } #ifdef CONFIG_MEMCG_V1 @@ -2845,7 +2847,7 @@ static void drain_obj_stock(struct memcg_stock_pcp *stock) memcg = get_mem_cgroup_from_objcg(old); - mod_memcg_state(memcg, MEMCG_KMEM, -nr_pages); + __mod_memcg_state(memcg, MEMCG_KMEM, -nr_pages); memcg1_account_kmem(memcg, -nr_pages); if (!mem_cgroup_is_root(memcg)) memcg_uncharge(memcg, nr_pages);