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