Re: [PATCH 1/5] pid: Implement task references.

[Eric Dumazet <dada1@cosmosbay.com>]

Greg KH a écrit :
> On Mon, Jan 30, 2006 at 06:19:35AM +0100, Eric Dumazet wrote:
>> Example of improvement in kref_put() :
>>
>> [PATCH] kref : Avoid an atomic operation in kref_put() when the last 
>> reference is dropped. On most platforms, atomic_read() is a plan read of 
>> the counter and involves no atomic at all.
> 
> No, we wat to decrement and test at the same time, to protect against
> any race where someone is incrementing right when we are dropping the
> last reference.

Sorry ? Me confused !

What I am saying is :

If a CPUA is doing a kref_put() and refcount == 1, then another CPU *cannot* 
change the refcount, because only one CPU has a valid reference on the object. 
(CPUA )

Proof :

If CPUB cannot kref_put() as well because only CPUA owns a refcount. (If two 
CPUS could kref_put, then counter would become -1 !)

If CPUB is going to do a kref_get() : Not allowed by kref specs  (Rule 3)
(It would mean that both CPUA/B have a reference)

'The kref_get() does not require a lock,
since we already have a valid pointer that we own a refcount for.'

So CPUA and CPUB cannot both own a refcount on a object having refcount=1

If you allow this, then current implementation is buggy as well.

kref->refcount == 1

CPUA :

if (atomic_dec_and_test(&kref->refcount)) {
    returns TRUE condition
[refcount is now 0]

CPUB :
    atomic_inc(&kref->refcount)
[refcount is now 1]

CPUA :
	release(kref); (object freed)

CPUB :
     doing some work on object freed by CPUA. *kaboom*

CPUB : kref_put()
     refcount back to 0 -> object freed twice *kaboom*

> 
> So, thanks, but I'm not going to apply this.
> 
> greg k-h
> 

Me confused.

Yes, kref abstraction is good :)