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

[Eric Dumazet]

Eric W. Biederman a écrit :
> Eric Dumazet <dada1@xxxxxxxxxxxxx> writes:
>
> > 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 !
>
> Largely I think you have the right of it, that the optimization is
> correct.  My biggest complaint is that the common case is going to be
> several references to the data structure.  Releasing the references
> will always be slow.  To do the read you need to get the value into
> the cache line.
>
> So it looks to me like you are optimizing the wrong case and
> bloating the icache with unnecessary code.

This function is not inlined.

Adding a test and a branch is a matter of 7 bytes.

 'Bloating the icache' is a litle bit off :)

Avoiding an atomic is important. This is already done elsewhere in the kernel, 
in a inlined function with *many* call sites :

(See kfree_skb() in include/linux//skbuff.h )

/*
 * If users == 1, we are the only owner and are can avoid redundant
 * atomic change.
 */

/**
 *      kfree_skb - free an sk_buff
 *      @skb: buffer to free
 *
 *      Drop a reference to the buffer and free it if the usage count has
 *      hit zero.
 */
static inline void kfree_skb(struct sk_buff *skb)
{
        if (likely(atomic_read(&skb->users) == 1))
                smp_rmb();
        else if (likely(!atomic_dec_and_test(&skb->users)))
                return;
        __kfree_skb(skb);
}


This is a valid optimization : an atomic_dec_and_test() is very expensive.

Eric
-
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/