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

[Eric Dumazet]

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 :)
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/