Re: [PATCH 00/14] replace call_rcu by kfree_rcu for simple kmem_cache_free callback

[Vlastimil Babka <vbabka@suse.cz>]

On 6/13/24 2:22 PM, Jason A. Donenfeld wrote:
> On Wed, Jun 12, 2024 at 08:38:02PM -0700, Paul E. McKenney wrote:
>> o	Make the current kmem_cache_destroy() asynchronously wait for
>> 	all memory to be returned, then complete the destruction.
>> 	(This gets rid of a valuable debugging technique because
>> 	in normal use, it is a bug to attempt to destroy a kmem_cache
>> 	that has objects still allocated.)

This seems like the best option to me. As Jason already said, the debugging
technique is not affected significantly, if the warning just occurs
asynchronously later. The module can be already unloaded at that point, as
the leak is never checked programatically anyway to control further
execution, it's just a splat in dmesg.

> Specifically what I mean is that we can still claim a memory leak has
> occurred if one batched kfree_rcu freeing grace period has elapsed since
> the last call to kmem_cache_destroy_rcu_wait/barrier() or
> kmem_cache_destroy_rcu(). In that case, you quit blocking, or you quit
> asynchronously waiting, and then you splat about a memleak like we have
> now.

Yes so we'd need the kmem_cache_free_barrier() for a slab kunit test (or the
pessimistic variant waiting for the 21 seconds), and a polling variant of
the same thing for the asynchronous destruction. Or we don't need a polling
variant if it's ok to invoke such a barrier in a schedule_work() workfn.

We should not need any new kmem_cache flag nor kmem_cache_destroy() flag to
burden the users of kfree_rcu() with. We have __kmem_cache_shutdown() that
will try to flush everything immediately and if it doesn't succeed, we can
assume kfree_rcu() might be in flight and try to wait for it asynchronously,
without any flags.

SLAB_TYPESAFE_BY_RCU is still handled specially because it has special
semantics as well.

As for users of call_rcu() with arbitrary callbacks that might be functions
from the module that is about to unload, these should not return from
kmem_cache_destroy() with objects in flight. But those should be using
rcu_barrier() before calling kmem_cache_destroy() already, and probably we
should not try to handle this automagically? Maybe one potential change with
the described approach is that today they would get the "cache not empty"
warning immediately. But that wouldn't stop the module unload so later the
callbacks would try to execute unmapped code anyway. With the new approach
the asynchronous handling might delay the "cache not empty" warnings (or
not, if kmem_cache_free_barrier() would finish before a rcu_barrier() would)
so the unmapped code execution would come first. I don't think that would be
a regression.