Re: [RFC PATCH 10/10] memcg: no more irq disabling for stock locks

[Sebastian Andrzej Siewior]

On 2025-03-14 11:54:34 [+0100], Vlastimil Babka wrote:
> On 3/14/25 07:15, Shakeel Butt wrote:
> > Let's switch all memcg_stock locks acquire and release places to not
> > disable and enable irqs. There are two still functions (i.e.
> > mod_objcg_state() and drain_obj_stock) which needs to disable irqs to
> > update the stats on non-RT kernels. For now add a simple wrapper for
> > that functionality.
> 
> BTW, which part of __mod_objcg_mlstate() really needs disabled irqs and not
> just preemption? I see it does rcu_read_lock() anyway, which disables
> preemption. Then in __mod_memcg_lruvec_state() we do some __this_cpu_add()
> updates. I think these also are fine with just disabled preemption as they
> are atomic vs irqs (but don't need LOCK prefix to be atomic vs other cpus
> updates).

__this_cpu_add() is not safe if also used in interrupt context. Some
architectures (not x86) implemented as read, add, write.
this_cpu_add()() does the same but disables interrupts during the
operation.
So __this_cpu_add() should not be used if interrupts are not disabled
and a modification can happen from interrupt context.

> Is it just memcg_rstat_updated() which does READ_ONCE/WRITE_ONCE? Could we
> perhaps just change it to operations where disabled preemption is enough?
> 
> > Signed-off-by: Shakeel Butt <shakeel.butt@xxxxxxxxx>
…
> > @@ -2757,6 +2745,28 @@ static void replace_stock_objcg(struct memcg_stock_pcp *stock,
> >  	WRITE_ONCE(stock->cached_objcg, objcg);
> >  }
> >  
> > +static unsigned long rt_lock(void)
> > +{

No, we don't name it rt_lock(). We have local_lock() for this exact
reason. And migrate_disable() does not protect vs re-enter of the
function on the CPU while local_irq_save() does.

> > +#ifdef CONFIG_PREEMPT_RT
> > +	migrate_disable();
> > +	return 0;
> > +#else
> > +	unsigned long flags = 0;
> > +
> > +	local_irq_save(flags);
> > +	return flags;
> > +#endif
> > +}
> > +
> > +static void rt_unlock(unsigned long flags)
> > +{
> > +#ifdef CONFIG_PREEMPT_RT
> > +	migrate_enable();
> > +#else
> > +	local_irq_restore(flags);
> > +#endif
> > +}
> > +
> >  static void mod_objcg_state(struct obj_cgroup *objcg, struct pglist_data *pgdat,
> >  		     enum node_stat_item idx, int nr)
> >  {

Sebastian