[PATCH 1/3] net: init_vlan should not copy slave or master flags

[PATCH 1/3] net: init_vlan should not copy slave or master flags

[John Fastabend]

The vlan device should not copy the slave or master flags from
the real device. It is not in the bond until added nor is it
a master.

Signed-off-by: John Fastabend <john.r.fastabend@intel.com>
---

 net/8021q/vlan_dev.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/net/8021q/vlan_dev.c b/net/8021q/vlan_dev.c
index b5249c5..c13d8a3 100644
--- a/net/8021q/vlan_dev.c
+++ b/net/8021q/vlan_dev.c
@@ -708,7 +708,8 @@ static int vlan_dev_init(struct net_device *dev)
 	netif_carrier_off(dev);
 
 	/* IFF_BROADCAST|IFF_MULTICAST; ??? */
-	dev->flags  = real_dev->flags & ~(IFF_UP | IFF_PROMISC | IFF_ALLMULTI);
+	dev->flags  = real_dev->flags & ~(IFF_UP | IFF_PROMISC | IFF_ALLMULTI |
+					  IFF_MASTER | IFF_SLAVE);
 	dev->iflink = real_dev->ifindex;
 	dev->state  = (real_dev->state & ((1<<__LINK_STATE_NOCARRIER) |
 					  (1<<__LINK_STATE_DORMANT))) |

[PATCH 2/3] net: fix conflict between null_or_orig and null_or_bond

[John Fastabend]

If a skb is received on an inactive bond that does not meet
the special cases checked for by skb_bond_should_drop it should
only be delivered to exact matches as the comment in
netif_receive_skb() says.

However because null_or_bond could also be null this is not
always true.  This patch renames null_or_bond to orig_or_bond
and initializes it to orig_dev.  This keeps the intent of
null_or_bond to pass frames received on VLAN interfaces stacked
on bonding interfaces without invalidating the statement for
null_or_orig.

Signed-off-by: John Fastabend <john.r.fastabend@intel.com>
---

 net/core/dev.c |    8 ++++----
 1 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/net/core/dev.c b/net/core/dev.c
index 32611c8..3dc691d 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -2784,7 +2784,7 @@ static int __netif_receive_skb(struct sk_buff *skb)
 	struct net_device *orig_dev;
 	struct net_device *master;
 	struct net_device *null_or_orig;
-	struct net_device *null_or_bond;
+	struct net_device *orig_or_bond;
 	int ret = NET_RX_DROP;
 	__be16 type;
 
@@ -2857,10 +2857,10 @@ ncls:
 	 * device that may have registered for a specific ptype.  The
 	 * handler may have to adjust skb->dev and orig_dev.
 	 */
-	null_or_bond = NULL;
+	orig_or_bond = orig_dev;
 	if ((skb->dev->priv_flags & IFF_802_1Q_VLAN) &&
 	    (vlan_dev_real_dev(skb->dev)->priv_flags & IFF_BONDING)) {
-		null_or_bond = vlan_dev_real_dev(skb->dev);
+		orig_or_bond = vlan_dev_real_dev(skb->dev);
 	}
 
 	type = skb->protocol;
@@ -2868,7 +2868,7 @@ ncls:
 			&ptype_base[ntohs(type) & PTYPE_HASH_MASK], list) {
 		if (ptype->type == type && (ptype->dev == null_or_orig ||
 		     ptype->dev == skb->dev || ptype->dev == orig_dev ||
-		     ptype->dev == null_or_bond)) {
+		     ptype->dev == orig_or_bond)) {
 			if (pt_prev)
 				ret = deliver_skb(skb, pt_prev, orig_dev);
 			pt_prev = ptype;

Re: [PATCH 2/3] net: fix conflict between null_or_orig and null_or_bond

[David Miller]

From: John Fastabend <john.r.fastabend@intel.com>
Date: Thu, 13 May 2010 00:31:11 -0700

> If a skb is received on an inactive bond that does not meet
> the special cases checked for by skb_bond_should_drop it should
> only be delivered to exact matches as the comment in
> netif_receive_skb() says.
> 
> However because null_or_bond could also be null this is not
> always true.  This patch renames null_or_bond to orig_or_bond
> and initializes it to orig_dev.  This keeps the intent of
> null_or_bond to pass frames received on VLAN interfaces stacked
> on bonding interfaces without invalidating the statement for
> null_or_orig.
> 
> Signed-off-by: John Fastabend <john.r.fastabend@intel.com>

Also applied.

[PATCH 3/3] net: deliver skbs on inactive slaves to exact matches

[John Fastabend]

Currently, the accelerated receive path for VLAN's will
drop packets if the real device is an inactive slave and
is not one of the special pkts tested for in
skb_bond_should_drop().  This behavior is different then
the non-accelerated path and for pkts over a bonded vlan.

For example,

vlanx -> bond0 -> ethx

will be dropped in the vlan path and not delivered to any
packet handlers at all.  However,

bond0 -> vlanx -> ethx

and

bond0 -> ethx

will be delivered to handlers that match the exact dev,
because the VLAN path checks the real_dev which is not a
slave and netif_recv_skb() doesn't drop frames but only
delivers them to exact matches.

This patch adds a sk_buff flag which is used for tagging
skbs that would previously been dropped and allows the
skb to continue to skb_netif_recv().  Here we add
logic to check for the bond_should_drop flag and if it
is set only deliver to handlers that match exactly.  This
makes both paths above consistent and gives pkt handlers
a way to identify skbs that come from inactive slaves.
Without this patch in some configurations skbs will be
delivered to handlers with exact matches and in others
be dropped out right in the vlan path.

I have tested the following 4 configurations in failover modes
and load balancing modes.

# bond0 -> ethx

# vlanx -> bond0 -> ethx

# bond0 -> vlanx -> ethx

# bond0 -> ethx
            |
  vlanx -> --

Signed-off-by: John Fastabend <john.r.fastabend@intel.com>
---

 include/linux/skbuff.h |    8 +++++++-
 net/8021q/vlan_core.c  |    8 ++++++--
 net/core/dev.c         |   23 +++++++++++++++++++----
 3 files changed, 32 insertions(+), 7 deletions(-)

diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
index c9525bc..5ba4fd5 100644
--- a/include/linux/skbuff.h
+++ b/include/linux/skbuff.h
@@ -379,8 +379,14 @@ struct sk_buff {
 
 	kmemcheck_bitfield_begin(flags2);
 	__u16			queue_mapping:16;
-#ifdef CONFIG_IPV6_NDISC_NODETYPE
+#if defined(CONFIG_IPV6_NDISC_NODETYPE) && \
+    (defined(CONFIG_BONDING) || defined(CONFIG_BONDING_MODULE))
+	__u8			ndisc_nodetype:2,
+				bond_should_drop:1;
+#elif defined(CONFIG_IPV6_NDISC_NODETYPE)
 	__u8			ndisc_nodetype:2;
+#elif defined(CONFIG_BONDING) || defined(CONFIG_BONDING_MODULE)
+	__u8			bond_should_drop:1;
 #endif
 	kmemcheck_bitfield_end(flags2);
 
diff --git a/net/8021q/vlan_core.c b/net/8021q/vlan_core.c
index c584a0a..57ac2d3 100644
--- a/net/8021q/vlan_core.c
+++ b/net/8021q/vlan_core.c
@@ -11,8 +11,10 @@ int __vlan_hwaccel_rx(struct sk_buff *skb, struct vlan_group *grp,
 	if (netpoll_rx(skb))
 		return NET_RX_DROP;
 
+#if defined(CONFIG_BONDING) || defined(CONFIG_BONDING_MODULE)
 	if (skb_bond_should_drop(skb, ACCESS_ONCE(skb->dev->master)))
-		goto drop;
+		skb->bond_should_drop = 1;
+#endif
 
 	skb->skb_iif = skb->dev->ifindex;
 	__vlan_hwaccel_put_tag(skb, vlan_tci);
@@ -83,8 +85,10 @@ vlan_gro_common(struct napi_struct *napi, struct vlan_group *grp,
 {
 	struct sk_buff *p;
 
+#if defined(CONFIG_BONDING) || defined(CONFIG_BONDING_MODULE)
 	if (skb_bond_should_drop(skb, ACCESS_ONCE(skb->dev->master)))
-		goto drop;
+		skb->bond_should_drop = 1;
+#endif
 
 	skb->skb_iif = skb->dev->ifindex;
 	__vlan_hwaccel_put_tag(skb, vlan_tci);
diff --git a/net/core/dev.c b/net/core/dev.c
index 3dc691d..92fdff4 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -2782,11 +2782,13 @@ static int __netif_receive_skb(struct sk_buff *skb)
 {
 	struct packet_type *ptype, *pt_prev;
 	struct net_device *orig_dev;
-	struct net_device *master;
 	struct net_device *null_or_orig;
 	struct net_device *orig_or_bond;
 	int ret = NET_RX_DROP;
 	__be16 type;
+#if defined(CONFIG_BONDING) || defined(CONFIG_BONDING_MODULE)
+	struct net_device *master;
+#endif
 
 	if (!skb->tstamp.tv64)
 		net_timestamp(skb);
@@ -2801,15 +2803,28 @@ static int __netif_receive_skb(struct sk_buff *skb)
 	if (!skb->skb_iif)
 		skb->skb_iif = skb->dev->ifindex;
 
+	/*
+	 * bonding note: skbs received on inactive slaves should only
+	 * be delivered to pkt handlers that are exact matches.  Also
+	 * the bond_should_drop flag will be set.  If packet handlers
+	 * are sensitive to duplicate packets these skbs will need to
+	 * be dropped at the handler.  The vlan accel path may have
+	 * already set the bond_should_drop flag.
+	 */
 	null_or_orig = NULL;
 	orig_dev = skb->dev;
+#if defined(CONFIG_BONDING) || defined(CONFIG_BONDING_MODULE)
 	master = ACCESS_ONCE(orig_dev->master);
-	if (master) {
-		if (skb_bond_should_drop(skb, master))
+	if (skb->bond_should_drop)
+		null_or_orig = orig_dev;
+	else if (master) {
+		if (skb_bond_should_drop(skb, master)) {
+			skb->bond_should_drop = 1;
 			null_or_orig = orig_dev; /* deliver only exact match */
-		else
+		} else
 			skb->dev = master;
 	}
+#endif
 
 	__get_cpu_var(softnet_data).processed++;
 

Re: [PATCH 3/3] net: deliver skbs on inactive slaves to exact matches

[John Fastabend]

Fastabend, John R wrote:
> Currently, the accelerated receive path for VLAN's will
> drop packets if the real device is an inactive slave and
> is not one of the special pkts tested for in
> skb_bond_should_drop().  This behavior is different then
> the non-accelerated path and for pkts over a bonded vlan.
> 
> For example,
> 
> vlanx -> bond0 -> ethx
> 
> will be dropped in the vlan path and not delivered to any
> packet handlers at all.  However,
> 
> bond0 -> vlanx -> ethx
> 
> and
> 
> bond0 -> ethx
> 
> will be delivered to handlers that match the exact dev,
> because the VLAN path checks the real_dev which is not a
> slave and netif_recv_skb() doesn't drop frames but only
> delivers them to exact matches.
> 
> This patch adds a sk_buff flag which is used for tagging
> skbs that would previously been dropped and allows the
> skb to continue to skb_netif_recv().  Here we add
> logic to check for the bond_should_drop flag and if it
> is set only deliver to handlers that match exactly.  This
> makes both paths above consistent and gives pkt handlers
> a way to identify skbs that come from inactive slaves.
> Without this patch in some configurations skbs will be
> delivered to handlers with exact matches and in others
> be dropped out right in the vlan path.
> 
> I have tested the following 4 configurations in failover modes
> and load balancing modes.
> 
> # bond0 -> ethx
> 
> # vlanx -> bond0 -> ethx
> 
> # bond0 -> vlanx -> ethx
> 
> # bond0 -> ethx
>             |
>   vlanx -> --
> 
> Signed-off-by: John Fastabend <john.r.fastabend@intel.com>
> ---
> 

Jay,

I know you were looking over this series at one point.  Did you have any 
comments?  Sorry for the ping just wanted to keep this on my radar.

Thanks,
John

Re: [PATCH 3/3] net: deliver skbs on inactive slaves to exact matches

[David Miller]

From: John Fastabend <john.r.fastabend@intel.com>
Date: Wed, 26 May 2010 21:52:58 -0700

> I know you were looking over this series at one point.  Did you have
> any comments?  Sorry for the ping just wanted to keep this on my
> radar.

I'll hold this last one until Jay has a chance to comment on it.

Re: [PATCH 3/3] net: deliver skbs on inactive slaves to exact matches

[Jay Vosburgh]

David Miller <davem@davemloft.net> wrote:

>From: John Fastabend <john.r.fastabend@intel.com>
>Date: Wed, 26 May 2010 21:52:58 -0700
>
>> I know you were looking over this series at one point.  Did you have
>> any comments?  Sorry for the ping just wanted to keep this on my
>> radar.
>
>I'll hold this last one until Jay has a chance to comment on it.

	I've looked at it, and was initially hoping to combine this with
Andy/Neil's vaguely similar changes, but I don't see a reasonable way to
do that.

	I think the functionality is reasonable, i.e., adding a facility
to implement "direct bind" delivery of packets on inactive bonding
slaves (where direct bind means that the struct packet_type has a
non-NULL dev).

	I have a couple of concerns about the patch itself:

>From: John Fastabend <john.r.fastabend@intel.com>
>Subject: [PATCH 3/3] net: deliver skbs on inactive slaves to exact matches
>To: andy@greyhouse.net, fubar@us.ibm.com, nhorman@tuxdriver.com,
> bonding-devel@lists.sourceforge.net, netdev@vger.kernel.org
>Cc: john.r.fastabend@intel.com
>Date: Thu, 13 May 2010 00:31:17 -0700
>
>Currently, the accelerated receive path for VLAN's will
>drop packets if the real device is an inactive slave and
>is not one of the special pkts tested for in
>skb_bond_should_drop().  This behavior is different then
>the non-accelerated path and for pkts over a bonded vlan.
>
>For example,
>
>vlanx -> bond0 -> ethx
>
>will be dropped in the vlan path and not delivered to any
>packet handlers at all.  However,
>
>bond0 -> vlanx -> ethx
>
>and
>
>bond0 -> ethx
>
>will be delivered to handlers that match the exact dev,
>because the VLAN path checks the real_dev which is not a
>slave and netif_recv_skb() doesn't drop frames but only
>delivers them to exact matches.
>
>This patch adds a sk_buff flag which is used for tagging
>skbs that would previously been dropped and allows the
>skb to continue to skb_netif_recv().  Here we add
>logic to check for the bond_should_drop flag and if it
>is set only deliver to handlers that match exactly.  This
>makes both paths above consistent and gives pkt handlers
>a way to identify skbs that come from inactive slaves.
>Without this patch in some configurations skbs will be
>delivered to handlers with exact matches and in others
>be dropped out right in the vlan path.
>
>I have tested the following 4 configurations in failover modes
>and load balancing modes.
>
># bond0 -> ethx
>
># vlanx -> bond0 -> ethx
>
># bond0 -> vlanx -> ethx
>
># bond0 -> ethx
>            |
>  vlanx -> --
>
>Signed-off-by: John Fastabend <john.r.fastabend@intel.com>
>---
>
> include/linux/skbuff.h |    8 +++++++-
> net/8021q/vlan_core.c  |    8 ++++++--
> net/core/dev.c         |   23 +++++++++++++++++++----
> 3 files changed, 32 insertions(+), 7 deletions(-)
>
>diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
>index c9525bc..5ba4fd5 100644
>--- a/include/linux/skbuff.h
>+++ b/include/linux/skbuff.h
>@@ -379,8 +379,14 @@ struct sk_buff {
>
> 	kmemcheck_bitfield_begin(flags2);
> 	__u16			queue_mapping:16;
>-#ifdef CONFIG_IPV6_NDISC_NODETYPE
>+#if defined(CONFIG_IPV6_NDISC_NODETYPE) && \
>+    (defined(CONFIG_BONDING) || defined(CONFIG_BONDING_MODULE))
>+	__u8			ndisc_nodetype:2,
>+				bond_should_drop:1;
>+#elif defined(CONFIG_IPV6_NDISC_NODETYPE)
> 	__u8			ndisc_nodetype:2;
>+#elif defined(CONFIG_BONDING) || defined(CONFIG_BONDING_MODULE)
>+	__u8			bond_should_drop:1;
> #endif

	First, this looks like too much #ifdef soup here.  None of the
bonding-specific code in the packet receive processing path has
historically been #ifdefed out; there are more examples further down in
the patch.

	Second, should the field be named something generic, e.g.,
"deliver_no_wcard" to indicate its function?  "bond_should_drop" doesn't
really describe what the field is used for (which is to specify that the
packet should be delivered only to non-wildcard packet handlers).  With
this change, packets are never dropped outright as the result of what
the bonding "should drop" logic says.

	I'm open to alternatives to using a field in the sk_buff; John's
original submission added a PACKET_DROP (to supplant PACKET_LOCAL,
PACKET_BROADCAST, etc), but that seemed like a loss of information to
me.  I haven't thought of a way to efficiently accomplish John's goal
without putting state into the skb somewhere.

	-J

> 	kmemcheck_bitfield_end(flags2);
>
>diff --git a/net/8021q/vlan_core.c b/net/8021q/vlan_core.c
>index c584a0a..57ac2d3 100644
>--- a/net/8021q/vlan_core.c
>+++ b/net/8021q/vlan_core.c
>@@ -11,8 +11,10 @@ int __vlan_hwaccel_rx(struct sk_buff *skb, struct vlan_group *grp,
> 	if (netpoll_rx(skb))
> 		return NET_RX_DROP;
>
>+#if defined(CONFIG_BONDING) || defined(CONFIG_BONDING_MODULE)
> 	if (skb_bond_should_drop(skb, ACCESS_ONCE(skb->dev->master)))
>-		goto drop;
>+		skb->bond_should_drop = 1;
>+#endif
> 	skb->skb_iif = skb->dev->ifindex;
> 	__vlan_hwaccel_put_tag(skb, vlan_tci);
>@@ -83,8 +85,10 @@ vlan_gro_common(struct napi_struct *napi, struct vlan_group *grp,
> {
> 	struct sk_buff *p;
>
>+#if defined(CONFIG_BONDING) || defined(CONFIG_BONDING_MODULE)
> 	if (skb_bond_should_drop(skb, ACCESS_ONCE(skb->dev->master)))
>-		goto drop;
>+		skb->bond_should_drop = 1;
>+#endif
>
> 	skb->skb_iif = skb->dev->ifindex;
> 	__vlan_hwaccel_put_tag(skb, vlan_tci);
>diff --git a/net/core/dev.c b/net/core/dev.c
>index 3dc691d..92fdff4 100644
>--- a/net/core/dev.c
>+++ b/net/core/dev.c
>@@ -2782,11 +2782,13 @@ static int __netif_receive_skb(struct sk_buff *skb)
> {
> 	struct packet_type *ptype, *pt_prev;
> 	struct net_device *orig_dev;
>-	struct net_device *master;
> 	struct net_device *null_or_orig;
> 	struct net_device *orig_or_bond;
> 	int ret = NET_RX_DROP;
> 	__be16 type;
>+#if defined(CONFIG_BONDING) || defined(CONFIG_BONDING_MODULE)
>+	struct net_device *master;
>+#endif
>
> 	if (!skb->tstamp.tv64)
> 		net_timestamp(skb);
>@@ -2801,15 +2803,28 @@ static int __netif_receive_skb(struct sk_buff *skb)
> 	if (!skb->skb_iif)
> 		skb->skb_iif = skb->dev->ifindex;
>
>+	/*
>+	 * bonding note: skbs received on inactive slaves should only
>+	 * be delivered to pkt handlers that are exact matches.  Also
>+	 * the bond_should_drop flag will be set.  If packet handlers
>+	 * are sensitive to duplicate packets these skbs will need to
>+	 * be dropped at the handler.  The vlan accel path may have
>+	 * already set the bond_should_drop flag.
>+	 */
> 	null_or_orig = NULL;
> 	orig_dev = skb->dev;
>+#if defined(CONFIG_BONDING) || defined(CONFIG_BONDING_MODULE)
> 	master = ACCESS_ONCE(orig_dev->master);
>-	if (master) {
>-		if (skb_bond_should_drop(skb, master))
>+	if (skb->bond_should_drop)
>+		null_or_orig = orig_dev;
>+	else if (master) {
>+		if (skb_bond_should_drop(skb, master)) {
>+			skb->bond_should_drop = 1;
> 			null_or_orig = orig_dev; /* deliver only exact match */
>-		else
>+		} else
> 			skb->dev = master;
> 	}
>+#endif
>
> 	__get_cpu_var(softnet_data).processed++;
>
>

---
	-Jay Vosburgh, IBM Linux Technology Center, fubar@us.ibm.com

Re: [PATCH 3/3] net: deliver skbs on inactive slaves to exact matches

[John Fastabend]

Jay Vosburgh wrote:
> David Miller <davem@davemloft.net> wrote:
> 
>> From: John Fastabend <john.r.fastabend@intel.com>
>> Date: Wed, 26 May 2010 21:52:58 -0700
>>
>>> I know you were looking over this series at one point.  Did you have
>>> any comments?  Sorry for the ping just wanted to keep this on my
>>> radar.
>> I'll hold this last one until Jay has a chance to comment on it.
> 
> 	I've looked at it, and was initially hoping to combine this with
> Andy/Neil's vaguely similar changes, but I don't see a reasonable way to
> do that.
> 
> 	I think the functionality is reasonable, i.e., adding a facility
> to implement "direct bind" delivery of packets on inactive bonding
> slaves (where direct bind means that the struct packet_type has a
> non-NULL dev).
> 
> 	I have a couple of concerns about the patch itself:
> 
>> From: John Fastabend <john.r.fastabend@intel.com>
>> Subject: [PATCH 3/3] net: deliver skbs on inactive slaves to exact matches
>> To: andy@greyhouse.net, fubar@us.ibm.com, nhorman@tuxdriver.com,
>> bonding-devel@lists.sourceforge.net, netdev@vger.kernel.org
>> Cc: john.r.fastabend@intel.com
>> Date: Thu, 13 May 2010 00:31:17 -0700
>>
>> Currently, the accelerated receive path for VLAN's will
>> drop packets if the real device is an inactive slave and
>> is not one of the special pkts tested for in
>> skb_bond_should_drop().  This behavior is different then
>> the non-accelerated path and for pkts over a bonded vlan.
>>
>> For example,
>>
>> vlanx -> bond0 -> ethx
>>
>> will be dropped in the vlan path and not delivered to any
>> packet handlers at all.  However,
>>
>> bond0 -> vlanx -> ethx
>>
>> and
>>
>> bond0 -> ethx
>>
>> will be delivered to handlers that match the exact dev,
>> because the VLAN path checks the real_dev which is not a
>> slave and netif_recv_skb() doesn't drop frames but only
>> delivers them to exact matches.
>>
>> This patch adds a sk_buff flag which is used for tagging
>> skbs that would previously been dropped and allows the
>> skb to continue to skb_netif_recv().  Here we add
>> logic to check for the bond_should_drop flag and if it
>> is set only deliver to handlers that match exactly.  This
>> makes both paths above consistent and gives pkt handlers
>> a way to identify skbs that come from inactive slaves.
>> Without this patch in some configurations skbs will be
>> delivered to handlers with exact matches and in others
>> be dropped out right in the vlan path.
>>
>> I have tested the following 4 configurations in failover modes
>> and load balancing modes.
>>
>> # bond0 -> ethx
>>
>> # vlanx -> bond0 -> ethx
>>
>> # bond0 -> vlanx -> ethx
>>
>> # bond0 -> ethx
>>            |
>>  vlanx -> --
>>
>> Signed-off-by: John Fastabend <john.r.fastabend@intel.com>
>> ---
>>
>> include/linux/skbuff.h |    8 +++++++-
>> net/8021q/vlan_core.c  |    8 ++++++--
>> net/core/dev.c         |   23 +++++++++++++++++++----
>> 3 files changed, 32 insertions(+), 7 deletions(-)
>>
>> diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
>> index c9525bc..5ba4fd5 100644
>> --- a/include/linux/skbuff.h
>> +++ b/include/linux/skbuff.h
>> @@ -379,8 +379,14 @@ struct sk_buff {
>>
>> 	kmemcheck_bitfield_begin(flags2);
>> 	__u16			queue_mapping:16;
>> -#ifdef CONFIG_IPV6_NDISC_NODETYPE
>> +#if defined(CONFIG_IPV6_NDISC_NODETYPE) && \
>> +    (defined(CONFIG_BONDING) || defined(CONFIG_BONDING_MODULE))
>> +	__u8			ndisc_nodetype:2,
>> +				bond_should_drop:1;
>> +#elif defined(CONFIG_IPV6_NDISC_NODETYPE)
>> 	__u8			ndisc_nodetype:2;
>> +#elif defined(CONFIG_BONDING) || defined(CONFIG_BONDING_MODULE)
>> +	__u8			bond_should_drop:1;
>> #endif
> 
> 	First, this looks like too much #ifdef soup here.  None of the
> bonding-specific code in the packet receive processing path has
> historically been #ifdefed out; there are more examples further down in
> the patch.
> 

OK, I will remove the #ifdefs.

> 	Second, should the field be named something generic, e.g.,
> "deliver_no_wcard" to indicate its function?  "bond_should_drop" doesn't
> really describe what the field is used for (which is to specify that the
> packet should be delivered only to non-wildcard packet handlers).  With
> this change, packets are never dropped outright as the result of what
> the bonding "should drop" logic says.
> 

Agreed "deliver_no_wcard" is better. Updated patch coming soon.

Thanks,
John

> 	I'm open to alternatives to using a field in the sk_buff; John's
> original submission added a PACKET_DROP (to supplant PACKET_LOCAL,
> PACKET_BROADCAST, etc), but that seemed like a loss of information to
> me.  I haven't thought of a way to efficiently accomplish John's goal
> without putting state into the skb somewhere.
> 
> 	-J
> 
>> 	kmemcheck_bitfield_end(flags2);
>>
>> diff --git a/net/8021q/vlan_core.c b/net/8021q/vlan_core.c
>> index c584a0a..57ac2d3 100644
>> --- a/net/8021q/vlan_core.c
>> +++ b/net/8021q/vlan_core.c
>> @@ -11,8 +11,10 @@ int __vlan_hwaccel_rx(struct sk_buff *skb, struct vlan_group *grp,
>> 	if (netpoll_rx(skb))
>> 		return NET_RX_DROP;
>>
>> +#if defined(CONFIG_BONDING) || defined(CONFIG_BONDING_MODULE)
>> 	if (skb_bond_should_drop(skb, ACCESS_ONCE(skb->dev->master)))
>> -		goto drop;
>> +		skb->bond_should_drop = 1;
>> +#endif
>> 	skb->skb_iif = skb->dev->ifindex;
>> 	__vlan_hwaccel_put_tag(skb, vlan_tci);
>> @@ -83,8 +85,10 @@ vlan_gro_common(struct napi_struct *napi, struct vlan_group *grp,
>> {
>> 	struct sk_buff *p;
>>
>> +#if defined(CONFIG_BONDING) || defined(CONFIG_BONDING_MODULE)
>> 	if (skb_bond_should_drop(skb, ACCESS_ONCE(skb->dev->master)))
>> -		goto drop;
>> +		skb->bond_should_drop = 1;
>> +#endif
>>
>> 	skb->skb_iif = skb->dev->ifindex;
>> 	__vlan_hwaccel_put_tag(skb, vlan_tci);
>> diff --git a/net/core/dev.c b/net/core/dev.c
>> index 3dc691d..92fdff4 100644
>> --- a/net/core/dev.c
>> +++ b/net/core/dev.c
>> @@ -2782,11 +2782,13 @@ static int __netif_receive_skb(struct sk_buff *skb)
>> {
>> 	struct packet_type *ptype, *pt_prev;
>> 	struct net_device *orig_dev;
>> -	struct net_device *master;
>> 	struct net_device *null_or_orig;
>> 	struct net_device *orig_or_bond;
>> 	int ret = NET_RX_DROP;
>> 	__be16 type;
>> +#if defined(CONFIG_BONDING) || defined(CONFIG_BONDING_MODULE)
>> +	struct net_device *master;
>> +#endif
>>
>> 	if (!skb->tstamp.tv64)
>> 		net_timestamp(skb);
>> @@ -2801,15 +2803,28 @@ static int __netif_receive_skb(struct sk_buff *skb)
>> 	if (!skb->skb_iif)
>> 		skb->skb_iif = skb->dev->ifindex;
>>
>> +	/*
>> +	 * bonding note: skbs received on inactive slaves should only
>> +	 * be delivered to pkt handlers that are exact matches.  Also
>> +	 * the bond_should_drop flag will be set.  If packet handlers
>> +	 * are sensitive to duplicate packets these skbs will need to
>> +	 * be dropped at the handler.  The vlan accel path may have
>> +	 * already set the bond_should_drop flag.
>> +	 */
>> 	null_or_orig = NULL;
>> 	orig_dev = skb->dev;
>> +#if defined(CONFIG_BONDING) || defined(CONFIG_BONDING_MODULE)
>> 	master = ACCESS_ONCE(orig_dev->master);
>> -	if (master) {
>> -		if (skb_bond_should_drop(skb, master))
>> +	if (skb->bond_should_drop)
>> +		null_or_orig = orig_dev;
>> +	else if (master) {
>> +		if (skb_bond_should_drop(skb, master)) {
>> +			skb->bond_should_drop = 1;
>> 			null_or_orig = orig_dev; /* deliver only exact match */
>> -		else
>> +		} else
>> 			skb->dev = master;
>> 	}
>> +#endif
>>
>> 	__get_cpu_var(softnet_data).processed++;
>>
>>
> 
> ---
> 	-Jay Vosburgh, IBM Linux Technology Center, fubar@us.ibm.com

Re: [PATCH 1/3] net: init_vlan should not copy slave or master flags

[David Miller]

From: John Fastabend <john.r.fastabend@intel.com>
Date: Thu, 13 May 2010 00:31:06 -0700

> The vlan device should not copy the slave or master flags from
> the real device. It is not in the bond until added nor is it
> a master.
> 
> Signed-off-by: John Fastabend <john.r.fastabend@intel.com>

Applied, thanks John.