kfree_skb: unnecessary check ?

kfree_skb: unnecessary check ?

[Patrick McHardy]

Hi everyone.

In skbuff.h one sees:

static inline void kfree_skb(struct sk_buff *skb)
{
        if (atomic_read(&skb->users) == 1 || 
atomic_dec_and_test(&skb->users))
                __kfree_skb(skb);
}

Is atomic_dec_and_test really necessary ? atomic_dec_and_test only 
returns true if its argument has hit zero, so it was 1 before.
If atomic_dec is only a bit cheaper than atomic_dec_and_test (which i 
guess it is), wouldn't it make more sense to use something like this:

static inline void kfree_skb(struct sk_buff *skb)
{
    if (atomic_read(&skb->users) == 1)
        __kfree(skb);
    else
        atomic_dec(&skb->users);
}

Bye,
Patrick

Re: kfree_skb: unnecessary check ?

[Thomas 'Dent' Mirlacher]

patrick,

--snip/snip
> static inline void kfree_skb(struct sk_buff *skb)
> {
-     if (atomic_read(&skb->users) == 1)
+     if (likely(atomic_read(&skb->users) == 1))
>         __kfree(skb);
>     else
>         atomic_dec(&skb->users);
> }

is even faster for the likely case by 1 2 jmps (gcc3.1).
(well, is it the likely case?)

	tm

-- 
in some way i do, and in some way i don't.

Re: kfree_skb: unnecessary check ?

[kuznet]

Hello!

> Is atomic_dec_and_test really necessary ? atomic_dec_and_test only 
> returns true if its argument has hit zero, so it was 1 before.
> If atomic_dec is only a bit cheaper than atomic_dec_and_test (which i 
> guess it is), wouldn't it make more sense to use something like this:

Does the prefix "atomic" not scare you? :-)


> static inline void kfree_skb(struct sk_buff *skb)
> {
>     if (atomic_read(&skb->users) == 1)
>         __kfree(skb);
>     else
>         atomic_dec(&skb->users);
> }

      else if (atomic_dec_and_test(&skb->users))
	   __kfree_skb(skb);

If you still do not understand think why it does not look like:

static inline void kfree_skb(struct sk_buff *skb)
{
    int users = skb->users;
    if (users == 1)
        __kfree(skb);
    else
        skb->users = users - 1;
}

Alexey

Re: kfree_skb: unnecessary check ?

[Patrick McHardy]

kuznet@ms2.inr.ac.ru wrote:

>Hello!
>
>>Is atomic_dec_and_test really necessary ? atomic_dec_and_test only 
>>returns true if its argument has hit zero, so it was 1 before.
>>If atomic_dec is only a bit cheaper than atomic_dec_and_test (which i 
>>guess it is), wouldn't it make more sense to use something like this:
>>
>
>Does the prefix "atomic" not scare you? :-)
>
>
>>static inline void kfree_skb(struct sk_buff *skb)
>>{
>>    if (atomic_read(&skb->users) == 1)
>>        __kfree(skb);
>>    else
>>        atomic_dec(&skb->users);
>>}
>>
>
>      else if (atomic_dec_and_test(&skb->users))
>	   __kfree_skb(skb);
>
>If you still do not understand think why it does not look like:
>
>static inline void kfree_skb(struct sk_buff *skb)
>{
>    int users = skb->users;
>    if (users == 1)
>        __kfree(skb);
>    else
>        skb->users = users - 1;
>}
>
>Alexey
>

Hehe got it :)
Thanks
Patrick