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.
