Re: [PATCH for-next v3 3/9] mm/slab: handle the !allow_spin case in kfree_rcu_sheaf()

On 6/17/26 07:32, Harry Yoo wrote:
>> Here we would get a sheaf with kmalloc_nolock() so that's ok even if it's
>> later freed by someone else by kfree(), right.
>> 
>>> +		}
>>> +
>>> +		empty = alloc_empty_sheaf(s, gfp, alloc_flags);
>>>  
>>>  		if (!empty)
>>>  			goto fail;
>>>  
>>>  		if (!local_trylock(&s->cpu_sheaves->lock)) {
>>> -			barn_put_empty_sheaf(barn, empty);
>>> +			__free_empty_sheaf(s, empty, allow_spin);
>> 
>> Well we could still use the barn with allow_spin == true.
> 
> Initially I did
> 
> if (!__barn_put_empty_sheaf(barn, empty, allow_spin))
> 	__free_empty_sheaf(s, empty, allow_spin);
> 
> but I ended up just calling __free_empty_sheaf() because it's
> pretty rare to hit... no strong opinion though.

Yeah we can leave it simpler as it's rare.

>> But more crucially, here we might be freeing with kfree_nolock() a sheaf
>> from the barn previously allocated with kmalloc()?
> 
> Well, we don't release and reacquire the local lock when we got an empty
> sheaf from the barn, so it doesn't free the sheaf from the barn?
> 
> That was indeed very subtle and I got confused :D.
> 
> When we free a sheaf in this function, it's always allocated in current
> context?

Indeed! It's fine then.

>> Maybe we need to track if it's the case and defer-free it or something.
>> 
>> Also maybe there could be a wrapper kfree_maybe_nolock() (~better name?)
>> That means "I want to kfree safely in kfree_nolock() context something that
>> MIGHT have been kmalloc()"
>> And maybe depending on the debugging options that make kmalloc() ->
>> kfree_nolock() incompatible, if those are not enabled, it wouldn't have to
>> defer, but proceed normally?
> 
> But I really like the idea of supporting kmalloc() -> kfree_nolock(),
> and I think it's worth exploring that.

At least my confusion potentially lead to something good then :)

> Thanks!
>