Re: [PATCH-next v3] mm/memcg: Properly handle memcg_stock access for PREEMPT_RT

[Waiman Long]

On 12/17/21 06:42, Sebastian Andrzej Siewior wrote:
> On 2021-12-14 09:44:12 [-0500], Waiman Long wrote:
> > --- a/mm/memcontrol.c
> > +++ b/mm/memcontrol.c
> > @@ -2096,7 +2096,12 @@ struct obj_stock {
> >   #endif
> >   };
> >   
> > +/*
> > + * The local_lock protects the whole memcg_stock_pcp structure including
> > + * the embedded obj_stock structures.
> > + */
> >   struct memcg_stock_pcp {
> > +	local_lock_t lock;
> >   	struct mem_cgroup *cached; /* this never be root cgroup */
> >   	unsigned int nr_pages;
> >   	struct obj_stock task_obj;
> > @@ -2145,7 +2150,7 @@ static bool consume_stock(struct mem_cgroup *memcg, unsigned int nr_pages)
> >   	if (nr_pages > MEMCG_CHARGE_BATCH)
> >   		return ret;
> >   
> > -	local_irq_save(flags);
> > +	local_lock_irqsave(&memcg_stock.lock, flags);
> This still does not explain why the lock is acquired here where it
> appears to be unrelated to memcg_stock.lock.

consume_stock() can be called in both task and irq context. irq context 
may include softirq where interrupt may have been enabled and something 
get interrupt again. The original code just do a local_irq_save() 
without documenting why we are doing so. So I didn't see a need to add 
comment about that.

> >   
> >   	stock = this_cpu_ptr(&memcg_stock);
> >   	if (memcg == stock->cached && stock->nr_pages >= nr_pages) {
> > @@ -2779,29 +2784,34 @@ static struct mem_cgroup *get_mem_cgroup_from_objcg(struct obj_cgroup *objcg)
> >    * which is cheap in non-preempt kernel. The interrupt context object stock
> >    * can only be accessed after disabling interrupt. User context code can
> >    * access interrupt object stock, but not vice versa.
> > + *
> > + * This task and interrupt context optimization is disabled for PREEMPT_RT
> > + * as there is no performance gain in this case and changes will be made to
> > + * irq_obj only.
> > + *
> > + * For non-PREEMPT_RT, we are not replacing preempt_disable() by local_lock()
> > + * as nesting of task_obj and irq_obj are allowed which may cause lockdep
> > + * splat if local_lock() is used. Using separate local locks will complicate
> > + * the interaction between obj_stock and the broader memcg_stock object.
> >    */
> >   static inline struct obj_stock *get_obj_stock(unsigned long *pflags)
> >   {
> > -	struct memcg_stock_pcp *stock;
> > -
> > -	if (likely(in_task())) {
> > +	if (likely(in_task()) && !IS_ENABLED(CONFIG_PREEMPT_RT)) {
> >   		*pflags = 0UL;
> >   		preempt_disable();
> > -		stock = this_cpu_ptr(&memcg_stock);
> > -		return &stock->task_obj;
> > +		return this_cpu_ptr(&memcg_stock.task_obj);
> Do we need to keep the memcg_stock.task_obj for !RT?
> I'm not really convinced that disabling either preemption or interrupts
> is so much better compared to actual locking locking with lockdep
> annotation. Looking at the history, I'm also impressed by that fact that
> disabling/ enabling interrupts is *so* expensive that all this is
> actually worth it.

For !RT with voluntary or no preemption, preempt_disable() is just a 
compiler barrier. So it is definitely cheaper than disabling interrupt. 
The performance benefit is less with preemptible but !RT kernel. 
Microbenchmark testing shows a performance improvement of a few percents 
depending on the exact benchmark.

Cheers,
Longman