diff mbox

[net,v2] bonding: enhance L2 hash helper with packet type

Message ID 1405410083-26489-1-git-send-email-Jianhua.Xie@freescale.com
State Changes Requested, archived
Delegated to: David Miller
Headers show

Commit Message

Jianhua Xie July 15, 2014, 7:41 a.m. UTC
From: Jianhua Xie <jianhua.xie@freescale.com>

Current L2 hash helper calculates destination eth addr and
source ether addr as L2 hash factors. This patch is adding
packet type ID field into hash factors, which can help to
distribute different types of packets like IPv4/IPv6 packets
to different slave devices while only BOND_XMIT_POLICY_LAYER2
is applied.

CC: Jay Vosburgh <j.vosburgh@gmail.com>
CC: Veaceslav Falico <vfalico@gmail.com>
CC: Andy Gospodarek <andy@greyhouse.net>
CC: David S. Miller <davem@davemloft.net>
CC: Pan Jiafei <Jiafei.Pan@freescale.com>

Suggested-by: David S. Miller <davem@davemloft.net>
Signed-off-by: Jianhua Xie <jianhua.xie@freescale.com>
---

v2-changes:
 Use the appropriate interface skb_header_pointer()
 to check skb headlen, fragmented packet or not is
 not distinguished any more.


 drivers/net/bonding/bond_main.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

Comments

Eric Dumazet July 15, 2014, 8:05 a.m. UTC | #1
On Tue, 2014-07-15 at 15:41 +0800, Xie Jianhua wrote:
> From: Jianhua Xie <jianhua.xie@freescale.com>
> 
> Current L2 hash helper calculates destination eth addr and
> source ether addr as L2 hash factors. This patch is adding
> packet type ID field into hash factors, which can help to
> distribute different types of packets like IPv4/IPv6 packets
> to different slave devices while only BOND_XMIT_POLICY_LAYER2
> is applied.
> 
> CC: Jay Vosburgh <j.vosburgh@gmail.com>
> CC: Veaceslav Falico <vfalico@gmail.com>
> CC: Andy Gospodarek <andy@greyhouse.net>
> CC: David S. Miller <davem@davemloft.net>
> CC: Pan Jiafei <Jiafei.Pan@freescale.com>
> 
> Suggested-by: David S. Miller <davem@davemloft.net>

I do not think this patch was suggested by David.

He suggested to use skb_header_pointer() which is an implementation
detail.

Anyway, patch looks fine, even if bond_eth_hash() is no longer a leaf
function.

Acked-by: Eric Dumazet <edumazet@google.com>


--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Veaceslav Falico July 15, 2014, 8:20 a.m. UTC | #2
On Tue, Jul 15, 2014 at 03:41:23PM +0800, Xie Jianhua wrote:
>From: Jianhua Xie <jianhua.xie@freescale.com>
>
>Current L2 hash helper calculates destination eth addr and
>source ether addr as L2 hash factors. This patch is adding
>packet type ID field into hash factors, which can help to
>distribute different types of packets like IPv4/IPv6 packets
>to different slave devices while only BOND_XMIT_POLICY_LAYER2
>is applied.

1) It's also used in BOND_XMIT_POLICY_{LAYER|ENCAP}23, for the 2nd level
hash.
2) The documentation (D/networking/bonding.txt) also needs an update.

Otherwise, looks good.

>
>CC: Jay Vosburgh <j.vosburgh@gmail.com>
>CC: Veaceslav Falico <vfalico@gmail.com>
>CC: Andy Gospodarek <andy@greyhouse.net>
>CC: David S. Miller <davem@davemloft.net>
>CC: Pan Jiafei <Jiafei.Pan@freescale.com>
>
>Suggested-by: David S. Miller <davem@davemloft.net>
>Signed-off-by: Jianhua Xie <jianhua.xie@freescale.com>
>---
>
>v2-changes:
> Use the appropriate interface skb_header_pointer()
> to check skb headlen, fragmented packet or not is
> not distinguished any more.
>
>
> drivers/net/bonding/bond_main.c | 8 ++++----
> 1 file changed, 4 insertions(+), 4 deletions(-)
>
>diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
>index 3a451b6..bcff90c 100644
>--- a/drivers/net/bonding/bond_main.c
>+++ b/drivers/net/bonding/bond_main.c
>@@ -2999,11 +2999,11 @@ static struct notifier_block bond_netdev_notifier = {
> /* L2 hash helper */
> static inline u32 bond_eth_hash(struct sk_buff *skb)
> {
>-	struct ethhdr *data = (struct ethhdr *)skb->data;
>-
>-	if (skb_headlen(skb) >= offsetof(struct ethhdr, h_proto))
>-		return data->h_dest[5] ^ data->h_source[5];
>+	struct ethhdr *ep, hdr_tmp;
>
>+	ep = skb_header_pointer(skb, 0, sizeof(hdr_tmp), &hdr_tmp);
>+	if (ep)
>+		return ep->h_dest[5] ^ ep->h_source[5] ^ ep->h_proto;
> 	return 0;
> }
>
>-- 
>1.8.5
>
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
David Miller July 16, 2014, 10:23 p.m. UTC | #3
From: Veaceslav Falico <vfalico@gmail.com>
Date: Tue, 15 Jul 2014 10:20:28 +0200

> On Tue, Jul 15, 2014 at 03:41:23PM +0800, Xie Jianhua wrote:
>>From: Jianhua Xie <jianhua.xie@freescale.com>
>>
>>Current L2 hash helper calculates destination eth addr and
>>source ether addr as L2 hash factors. This patch is adding
>>packet type ID field into hash factors, which can help to
>>distribute different types of packets like IPv4/IPv6 packets
>>to different slave devices while only BOND_XMIT_POLICY_LAYER2
>>is applied.
> 
> 1) It's also used in BOND_XMIT_POLICY_{LAYER|ENCAP}23, for the 2nd
> level
> hash.
> 2) The documentation (D/networking/bonding.txt) also needs an update.
> 
> Otherwise, looks good.

Please make these adjustments and resubmit this patch, thank you.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
index 3a451b6..bcff90c 100644
--- a/drivers/net/bonding/bond_main.c
+++ b/drivers/net/bonding/bond_main.c
@@ -2999,11 +2999,11 @@  static struct notifier_block bond_netdev_notifier = {
 /* L2 hash helper */
 static inline u32 bond_eth_hash(struct sk_buff *skb)
 {
-	struct ethhdr *data = (struct ethhdr *)skb->data;
-
-	if (skb_headlen(skb) >= offsetof(struct ethhdr, h_proto))
-		return data->h_dest[5] ^ data->h_source[5];
+	struct ethhdr *ep, hdr_tmp;
 
+	ep = skb_header_pointer(skb, 0, sizeof(hdr_tmp), &hdr_tmp);
+	if (ep)
+		return ep->h_dest[5] ^ ep->h_source[5] ^ ep->h_proto;
 	return 0;
 }