Skip to contentSkip to conversation

net: use skb_get_rxhash()

PATCH In review Changli Gao 2 participants 5 messages Aug 24, 2010

Subsystems (2)
Networking Network drivers
lore
Share & tools
lore

Changli Gao

author
#0 lore raw

PATCH 1/1 net: use skb_get_rxhash()

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 <[email protected]>
---
 net/core/dev.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
net/core/dev.c +1 -1
index 859e30f..ea041fd 100644
@@ -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);
#1 lore raw
Quoted 16 lines · 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 <[email protected]>
---
 net/core/dev.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
net/core/dev.c +0 -0
index 859e30f..ea041fd 100644

net/core/dev.c u16 skb_tx_hash(const struct net_device *dev, const struct sk_buff *skb)

Quoted 9 lines
net/core/dev.c +1 -2
@@ -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)

author
#2 lore raw
Quoted 31 lines · Eric Dumazet
Quoted 23 lines · 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 <[email protected]>

---
 net/core/dev.c |    2 +-

 1 file changed, 1 insertion(+), 1 deletion(-)
net/core/dev.c +0 -0
index 859e30f..ea041fd 100644
@@ -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);

Quoted 2 lines

      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 ?

Quoted 5 lines

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.

Signature · 2 lines
Regards,
Changli Gao([email protected])
#3 lore raw
Quoted 4 lines · Changli Gao

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)

author
#4 lore raw
Quoted 11 lines · Eric Dumazet
Quoted 4 lines · Changli Gao

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

Signature · 2 lines
Regards,
Changli Gao([email protected])