Re: [PATCH 4/4] memcg: multi objcg charge support

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

On Thu, May 21, 2026 at 10:43:11AM +0900, Harry Yoo wrote:
> 
> 
> On 5/21/26 10:05 AM, Shakeel Butt wrote:
> > On Wed, May 20, 2026 at 06:35:30PM +0900, Harry Yoo wrote:
> > > > @@ -3350,19 +3405,45 @@ static void __refill_obj_stock(struct obj_cgroup *objcg,
> > > >    		goto out;
> > > >    	}
> > > > -	stock_nr_bytes = stock->nr_bytes;
> > > > -	if (READ_ONCE(stock->cached_objcg) != objcg) { /* reset if necessary */
> > > > -		drain_obj_stock(stock);
> > > > +	for (i = 0; i < NR_OBJ_STOCK; ++i) {
> > > > +		struct obj_cgroup *cached = READ_ONCE(stock->cached[i]);
> > > > +
> > > > +		if (!cached) {
> > > > +			if (empty_slot == -1)
> > > > +				empty_slot = i;
> > > > +			continue;
> > > > +		}
> > > > +		if (cached == objcg) {
> > > > +			slot = i;
> > > > +			break;
> > > > +		}
> > > > +	}
> > > > +
> > > > +	if (slot == -1) {
> > > > +		slot = empty_slot;
> > > > +		if (slot == -1) {
> > > > +			slot = get_random_u32_below(NR_OBJ_STOCK);
> > > 
> > > It would break kmalloc_nolock() because _get_random_bytes() uses a spinlock.
> > > perhaps prandom_u32_state() should be sufficient in this case.
> 
> s/spinlock/local_lock/

I do see spinlock in crng_make_state() for some code paths.

> 
> > > Is there a reason why it uses random eviction, unlike multi-memcg percpu
> > > charge cache?
> > 
> > Oh I didn't know and actually we are already using get_random_u32_below() in
> > refill_stock(). So, it need fixing as well. That would be a separate patch.
> 
> Ouch, I see.
> > I will explore prandom_u32_state().
> 
> Thanks!
> 
> FYI, SLUB had a similar issue that was recently fixed:
> commit a1e244a9f1778 ("mm/slab: use prandom if !allow_spin").
> 
> It uses prandom if spinning is not allowed when shuffling slab freelist.

The drain does not really need a random number. Fixing an index like 0 makes it
much simpler but it might expose some corner cases. Round robin might be enough
for this though. I will see what would be the easiest way forward.