Re: [PATCH v2] memcg: trim the per-cpu charge stock instead of draining it

[Michal Hocko]

On Thu 20-08-26 13:50:32, Shakeel Butt wrote:
> On Thu, Aug 20, 2026 at 08:44:34PM +0200, Michal Hocko wrote:
> > On Thu 20-08-26 09:06:29, Shakeel Butt wrote:
> > > On Thu, Aug 20, 2026 at 09:06:18AM +0200, Michal Hocko wrote:
> > > > On Wed 19-08-26 18:20:10, Shakeel Butt wrote:
> > > > > @@ -2254,9 +2264,12 @@ static void refill_stock(struct mem_cgroup *memcg, unsigned int nr_pages)
> > > > >  			empty_slot = i;
> > > > >  		if (memcg == READ_ONCE(stock->cached[i])) {
> > > > >  			stock_pages = READ_ONCE(stock->nr_pages[i]) + nr_pages;
> > > > > +			if (stock_pages > MEMCG_STOCK_HIGH) {
> > > > > +				memcg_uncharge(memcg,
> > > > > +					       stock_pages - MEMCG_STOCK_LOW);
> > > > > +				stock_pages = MEMCG_STOCK_LOW;
> > > > 
> > > > I would find it easier to read to keep the update in sync with
> > > > memcg_uncharge, i.e.
> > > > 				stock_pages = WRITE_ONCE(stock_pages - MEMCG_STOCK_LOW)
> > > 
> > > Sorry I am not sure I understand your suggestion. The WRITE_ONCE() is throwing
> > > me off.
> > 
> > Sorry, brainfarth on my end. I meant to say that stock_pages should be
> > updated by the uncharged decrement rather than capping it at MEMCG_STOCK_LOW.
> > So in fact I meant this
> > 				stock_pages = stock_pages - MEMCG_STOCK_LOW;
> > but then shortcuted it to a nonsense.
> 
> To fully understand your suggestion, let me walkthrough the code:
> 
> Let's suppose stock_pages = 65
> 
> 	// Original code does
> 	memcg_uncharge(memcg, stock_pages-MEMCG_STOCK_LOW); // uncharge 33 pages
> 	stock_pages = MEMCG_STOCK_LOW;			    // stock 32 pages
> 
> -----
> 	// You want instead
> 	memcg_uncharge(memcg, MEMCG_STOCK_LOW); 	// uncharge 32 pages
> 	stock_pages = stock_pages-MEMCG_STOCK_LOW	// stock 33 pages
> 	
> 
> Am I understanding you correctly?

Nope you are right, I was clearly not thinking straight.
Sorry about the noise.

-- 
Michal Hocko
SUSE Labs