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

[Vlastimil Babka <vbabka@suse.cz>]

On 1/31/22 16:06, Sebastian Andrzej Siewior wrote:
>> > - 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

I see that's discussed in the subthread with Michal Hocko, so I would be
also leaning towards revert unless convincing numbers are provided.

> can prepare the patches accordingly before posting.

An acceptable form of this would have to basically replace the bool
stock_lock_acquried with two variants of the code paths that rely on it,
feel free to read though the previous occurence :)
https://lore.kernel.org/all/CAHk-=wiJLqL2cUhJbvpyPQpkbVOu1rVSzgO2=S2jC55hneLtfQ@mail.gmail.com/

>> > 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
>