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@xxxxxxxxxxxxx> >> >> 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@xxxxxxxxxxxxxx/ >> > 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 >
