Re: [PATCH 3/4] mm/memcg: Add a local_lock_t for IRQ and TASK object.

[Sebastian Andrzej Siewior <bigeasy@linutronix.de>]

On 2022-01-26 17:57:14 [+0100], Vlastimil Babka wrote:
> > - drain_obj_stock() gets a memcg_stock_pcp passed if the stock_lock has been
> >   acquired (instead of the task_obj_lock) to avoid recursive locking later
> >   in refill_stock().
> 
> Looks like this was maybe true in some previous version but now
> drain_obj_stock() gets a bool parameter that is passed to
> obj_cgroup_uncharge_pages(). But drain_local_stock() uses a NULL or
> stock_pcp for that bool parameter which is weird.

buh. Sorry, that is a left over and should have been true/false instead.

> > - drain_all_stock() disables preemption via get_cpu() and then invokes
> >   drain_local_stock() if it is the local CPU to avoid scheduling a worker
> >   (which invokes the same function). Disabling preemption here is
> >   problematic due to the sleeping locks in drain_local_stock().
> >   This can be avoided by always scheduling a worker, even for the local
> >   CPU. Using cpus_read_lock() stabilizes cpu_online_mask which ensures
> >   that no worker is scheduled for an offline CPU. Since there is no
> >   flush_work(), it is still possible that a worker is invoked on the wrong
> >   CPU but it is okay since it operates always on the local-CPU data.
> > 
> > - drain_local_stock() is always invoked as a worker so it can be optimized
> >   by removing in_task() (it is always true) and avoiding the "irq_save"
> >   variant because interrupts are always enabled here. Operating on
> >   task_obj first allows to acquire the lock_lock_t without lockdep
> >   complains.
> > 
> > Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
> 
> The problem is that this pattern where get_obj_stock() sets a
> stock_lock_acquried bool and this is passed down and acted upon elsewhere,
> is a well known massive red flag for Linus :/
> Maybe we should indeed just revert 559271146efc, as Michal noted there were
> no hard numbers to justify it, and in previous discussion it seemed to
> surface that the costs of irq disable/enable are not that bad on recent cpus
> as assumed?

I added some number, fell free re-run.
Let me know if a revert is preferred or you want to keep that so that I
can prepare the patches accordingly before posting.

> > diff --git a/mm/memcontrol.c b/mm/memcontrol.c
> > --- a/mm/memcontrol.c
> > +++ b/mm/memcontrol.c
> > @@ -260,8 +260,10 @@ bool mem_cgroup_kmem_disabled(void)
> >  	return cgroup_memory_nokmem;
> >  }
> >  
> > +struct memcg_stock_pcp;
> 
> Seems this forward declaration is unused.
you, thanks.

Sebastian