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

[Shakeel Butt <shakeel.butt@linux.dev>]

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.