[PATCH] net: use skb_get_rxhash()

[PATCH] net: use skb_get_rxhash()

[Changli Gao]

Although it is rare the line contains skb->rxhash is executed:

 * The forwarded skbs should have the the rx queue recoreded.
 * The local generated skbs should have non zero sk_hash in most of cases.

when it is executed, a non zero rxhash will be better.

Signed-off-by: Changli Gao <xiaosuo@gmail.com>
---
 net/core/dev.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/net/core/dev.c b/net/core/dev.c
index 859e30f..ea041fd 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -2034,7 +2034,7 @@ u16 skb_tx_hash(const struct net_device *dev, const struct sk_buff *skb)
 	if (skb->sk && skb->sk->sk_hash)
 		hash = skb->sk->sk_hash;
 	else
-		hash = (__force u16) skb->protocol ^ skb->rxhash;
+		hash = (__force u16) skb->protocol ^ skb_get_rxhash(skb);
 	hash = jhash_1word(hash, hashrnd);
 
 	return (u16) (((u64) hash * dev->real_num_tx_queues) >> 32);

Re: [PATCH] net: use skb_get_rxhash()

[Eric Dumazet]

Le mardi 24 août 2010 à 17:52 +0800, Changli Gao a écrit :
> Although it is rare the line contains skb->rxhash is executed:
> 
>  * The forwarded skbs should have the the rx queue recoreded.
>  * The local generated skbs should have non zero sk_hash in most of cases.
> 
> when it is executed, a non zero rxhash will be better.
> 
> Signed-off-by: Changli Gao <xiaosuo@gmail.com>
> ---
>  net/core/dev.c |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> diff --git a/net/core/dev.c b/net/core/dev.c
> index 859e30f..ea041fd 100644
> --- a/net/core/dev.c
> +++ b/net/core/dev.c
> @@ -2034,7 +2034,7 @@ u16 skb_tx_hash(const struct net_device *dev, const struct sk_buff *skb)
>  	if (skb->sk && skb->sk->sk_hash)
>  		hash = skb->sk->sk_hash;
>  	else
> -		hash = (__force u16) skb->protocol ^ skb->rxhash;
> +		hash = (__force u16) skb->protocol ^ skb_get_rxhash(skb);
>  	hash = jhash_1word(hash, hashrnd);
>  
>  	return (u16) (((u64) hash * dev->real_num_tx_queues) >> 32);
> --

Instead of adding hacks and growing kernel code (considering your
previous patch), could we sit down and fix existing code, if really
needed ?

Spirit of existing code is that tcp/udp sockets have a sk_hash field,
and this feeds skb->rxhash in transmit path.

(check commit 87fd308c for an example of a fix)

I do think its better to chose a hash value per socket, instead of doing
it per packet (duplicated work, and slowdown mono threaded apps because
their working set will be shared by several queues, then several cpus)

Re: [PATCH] net: use skb_get_rxhash()

[Changli Gao]

On Tue, Aug 24, 2010 at 6:21 PM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
> Le mardi 24 août 2010 à 17:52 +0800, Changli Gao a écrit :
>> Although it is rare the line contains skb->rxhash is executed:
>>
>>  * The forwarded skbs should have the the rx queue recoreded.
>>  * The local generated skbs should have non zero sk_hash in most of cases.
>>
>> when it is executed, a non zero rxhash will be better.
>>
>> Signed-off-by: Changli Gao <xiaosuo@gmail.com>
>> ---
>>  net/core/dev.c |    2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>> diff --git a/net/core/dev.c b/net/core/dev.c
>> index 859e30f..ea041fd 100644
>> --- a/net/core/dev.c
>> +++ b/net/core/dev.c
>> @@ -2034,7 +2034,7 @@ u16 skb_tx_hash(const struct net_device *dev, const struct sk_buff *skb)
>>       if (skb->sk && skb->sk->sk_hash)
>>               hash = skb->sk->sk_hash;
>>       else
>> -             hash = (__force u16) skb->protocol ^ skb->rxhash;
>> +             hash = (__force u16) skb->protocol ^ skb_get_rxhash(skb);
>>       hash = jhash_1word(hash, hashrnd);
>>
>>       return (u16) (((u64) hash * dev->real_num_tx_queues) >> 32);
>> --
>
> Instead of adding hacks and growing kernel code (considering your
> previous patch), could we sit down and fix existing code, if really
> needed ?
>
> Spirit of existing code is that tcp/udp sockets have a sk_hash field,
> and this feeds skb->rxhash in transmit path.
>
> (check commit 87fd308c for an example of a fix)
>

Thanks. Got it. skb->rxhash should be only used when skb->sk is NULL,
and skb->rxhash should save the value copied from skb->sk->sk_hash at
that time.

       if (skb->sk && skb->sk->sk_hash)
               hash = skb->sk->sk_hash;

However, do we really need to check if skb->sk->sk_hash is zero? It is
a rare case, and this check may mislead someone to think a non zero
sk_hash means that sk_hash is set.

-- 
Regards,
Changli Gao(xiaosuo@gmail.com)

Re: [PATCH] net: use skb_get_rxhash()

[Eric Dumazet]

Le mardi 24 août 2010 à 21:53 +0800, Changli Gao a écrit :

> However, do we really need to check if skb->sk->sk_hash is zero? It is
> a rare case, and this check may mislead someone to think a non zero
> sk_hash means that sk_hash is set.
> 

Not sure what you mean ;)

sk_hash is not always filled, only for some kind of sockets (tcp, udp)

Re: [PATCH] net: use skb_get_rxhash()

[Changli Gao]

On Tue, Aug 24, 2010 at 10:09 PM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
> Le mardi 24 août 2010 à 21:53 +0800, Changli Gao a écrit :
>
>> However, do we really need to check if skb->sk->sk_hash is zero? It is
>> a rare case, and this check may mislead someone to think a non zero
>> sk_hash means that sk_hash is set.
>>
>
> Not sure what you mean ;)
>
> sk_hash is not always filled, only for some kind of sockets (tcp, udp)
>

Hmm, if so, I think maybe addr_fold(skb->sk) is enough and more common.

if (skb->sk)
   hash = addr_fold(skb->sk);
else
   hash = skb->rxhash; /* saved addr_fold(skb->sk) by skb_orphan_try()
previous */

-- 
Regards,
Changli Gao(xiaosuo@gmail.com)