Re: [PATCH] rmaplock 2/6 SLAB_DESTROY_BY_RCU

[Manfred Spraul <manfred@colorfullife.com>]

Hugh Dickins wrote:

>On Tue, 13 Jul 2004, Manfred Spraul wrote:
>  
>
>>An interesting idea:
>>The slab caches are object caches. If a rcu user only needs a valid 
>>object but doesn't care which one then there is no need to wait for a 
>>quiescent cycle after free - the quiescent cycle can be delayed until 
>>the destructor is called.
>>    
>>
>
>Sorry, to be honest, I've not understood you there at all.
>I wonder if you're seeing some other use than I intended.
>
>  
>
No, I just tried to reworden your idea and I obviously failed.
Previously, I always assumed that the free call had to be delayed, but 
you have shown that this is wrong: If a rcu user can detect that an 
object changed its identity (or if he doesn't care), then it's not 
necessary to delay the free call, it's sufficient to delay the object 
destruction. And since the slab cache is an object cache delaying 
destruction causes virtually no overhead at all.
A second restriction is that it must be possible to recover from 
identity changes. I think this rules out the dentry cache - the hash 
chains point to wrong positions after a free/alloc/reuse cycle, it's not 
possible to recover from that.

>>But there are two flaws in your patch:
>>- you must disable poisoning and unmapping if SLAB_DESTROY_BY_RCU is set.
>>    
>>
>
>How right you are!  Thank you.  Does the further patch below suit?
>
>  
>
Yes, that should work.

>>- either delay the dtor calls a well or fail if an object has a non-NULL 
>>dtor and SLAB_DESTROY_BY_RCU is set.
>>    
>>
>
>Doesn't that rather depend on what the dtor does?  I'm not used to how
>destructors are commonly used, but I can easily imagine a destructor
>which, say, frees up some attached objects, but still leaves this cache
>object recognizably itself, good enough for the reference-after-free
>which SLAB_DESTROY_BY_RCU is allowing - all we need to avoid is the
>page being freed (or poisoned or unmapped) and reused.
>
This would introduce a concept of a half destroyed object. I think it's 
a bad idea, too easy to introduce bugs.
The correct fix would be to move the whole slab_destroy call into the 
rcu callback instead of just the kmem_cache_free call, but locking would 
be tricky - kmem_cache_destroy would have to wait for completion of 
outstanding rcu calls, etc.
Thus I'd propose a quick fix (fail if there is a dtor - are there any 
slab caches with dtors at all?) and in the long run slab_destroy should 
be moved into the rcu callback.

--
    Manfred