Re: [PATCH 5/5] mm: memcg: separate slab stat accounting from objcg charge cache

[Hao Li]

On Tue, Mar 03, 2026 at 08:26:31AM -0800, Shakeel Butt wrote:
> On Tue, Mar 03, 2026 at 10:43:29AM -0500, Johannes Weiner wrote:
> > On Tue, Mar 03, 2026 at 05:45:18AM -0800, Shakeel Butt wrote:
> > > On Tue, Mar 03, 2026 at 11:42:31AM +0100, Vlastimil Babka (SUSE) wrote:
> > > > On 3/3/26 09:54, Hao Li wrote:
> > > > > On Mon, Mar 02, 2026 at 02:50:18PM -0500, Johannes Weiner wrote:
> > > > >>  
> > > > >> +static void refill_obj_stock(struct obj_cgroup *objcg,
> > > > >> +			     unsigned int nr_bytes,
> > > > >> +			     bool allow_uncharge)
> > > > >> +{
> > > > >> +	struct obj_stock_pcp *stock = trylock_stock();
> > > > >> +	__refill_obj_stock(objcg, stock, nr_bytes, allow_uncharge);
> > > > >> +	unlock_stock(stock);
> > > > > 
> > > > > Hi Johannes,
> > > > > 
> > > > > I noticed that after this patch, obj_cgroup_uncharge_pages() is now inside
> > > > > the obj_stock.lock critical section. Since obj_cgroup_uncharge_pages() calls
> > > > > refill_stock(), which seems non-trivial, this might increase the lock hold time.
> > > > > In particular, could that lead to more failed trylocks for IRQ handlers on
> > > > > non-RT kernel (or for tasks that preempt others on RT kernel)?
> > 
> > Good catch. I did ponder this, but forgot by the time I wrote the
> > changelog.
> > 
> > > > Yes, it also seems a bit self-defeating? (at least in theory)
> > > > 
> > > > refill_obj_stock()
> > > >   trylock_stock()
> > > >   __refill_obj_stock()
> > > >     obj_cgroup_uncharge_pages()
> > > >       refill_stock()
> > > >         local_trylock() -> nested, will fail
> > > 
> > > Not really as the local_locks are different i.e. memcg_stock.lock in
> > > refill_stock() and obj_stock.lock in refill_obj_stock().
> > 
> > Right, refilling the *byte* stock could produce enough excess that we
> > refill the *page* stock. Which in turn could produce enough excess
> > that we drain that back to the page counters (shared atomics).
> > 
> > > However Hao's concern is valid and I think it can be easily fixed by
> > > moving obj_cgroup_uncharge_pages() out of obj_stock.lock.
> > 
> > Note that we now have multiple callsites of __refill_obj_stock(). Do
> > we care enough to move this to the caller?
> > 
> > There are a few other places with a similar pattern:
> > 
> > - drain_obj_stock(): calls memcg_uncharge() under the lock
> > - drain_stock(): calls memcg_uncharge() under the lock
> > - refill_stock(): still does full drain_stock()
> > 
> > All of these could be more intentional about only updating the per-cpu
> > data under the lock and the page counters outside of it.
> > 
> > Given that IRQ allocations/frees are rare, nested ones even rarer, and
> > the "slowpath" is a few extra atomics, I'm not sure it's worth the
> > code complication. At least until proven otherwise.
> > 
> > What do you think?
> 
> Yes this makes sense. We already have at least one evidence (bug Hao fixed) that
> these are very rare, so optimizing for such cases will just increase complexity
> without real benefit.

Yes, make sense to me too. Thanks for taking a look.

Reviewed-by: Hao Li <hao.li@xxxxxxxxx>