PATCH Fix bonding active-backup behavior for VLAN interfaces

PATCH Fix bonding active-backup behavior for VLAN interfaces

[Christophe Devriese]

diff -rU3 linux-2.6.17.7/net/core/dev.c linux-2.6.17.7-wapper/net/core/dev.c
--- linux-2.6.17.7/net/core/dev.c       2006-07-25 05:36:01.000000000 +0200
+++ linux-2.6.17.7-wapper/net/core/dev.c        2006-07-27 20:16:36.000000000 +0200
@@ -88,6 +88,7 @@
 #include <linux/sockios.h>
 #include <linux/errno.h>
 #include <linux/interrupt.h>
+#include <linux/if_bonding.h>
 #include <linux/if_ether.h>
 #include <linux/netdevice.h>
 #include <linux/etherdevice.h>
@@ -1518,37 +1519,6 @@

 EXPORT_SYMBOL(netif_rx_ni);

-static inline struct net_device *skb_bond(struct sk_buff *skb)
-{
-       struct net_device *dev = skb->dev;
-
-       if (dev->master) {
-               /*
-                * On bonding slaves other than the currently active
-                * slave, suppress duplicates except for 802.3ad
-                * ETH_P_SLOW and alb non-mcast/bcast.
-                */
-               if (dev->priv_flags & IFF_SLAVE_INACTIVE) {
-                       if (dev->master->priv_flags & IFF_MASTER_ALB) {
-                               if (skb->pkt_type != PACKET_BROADCAST &&
-                                   skb->pkt_type != PACKET_MULTICAST)
-                                       goto keep;
-                       }
-
-                       if (dev->master->priv_flags & IFF_MASTER_8023AD &&
-                           skb->protocol == __constant_htons(ETH_P_SLOW))
-                               goto keep;
-
-                       kfree_skb(skb);
-                       return NULL;
-               }
-keep:
-               skb->dev = dev->master;
-       }
-
-       return dev;
-}
-
 static void net_tx_action(struct softirq_action *h)
 {
        struct softnet_data *sd = &__get_cpu_var(softnet_data);

-- 
-----------------------------------------------------------------------
Christophe Devriese                                               EURiD
Network Adminstrator / Developer
christophe.devriese@eth1.org
---------------------------- http://www.eth1.org ----------------------

Re: PATCH Fix bonding active-backup behavior for VLAN interfaces

[Ben Greear]

This patch by itself does nothing useful, other than remove a method.

If we assume you did the patch backwards, and wanted to add the method
instead, then where is this method ever called?

Ben


Christophe Devriese wrote:
> diff -rU3 linux-2.6.17.7/net/core/dev.c linux-2.6.17.7-wapper/net/core/dev.c
> --- linux-2.6.17.7/net/core/dev.c       2006-07-25 05:36:01.000000000 +0200
> +++ linux-2.6.17.7-wapper/net/core/dev.c        2006-07-27 20:16:36.000000000 +0200
> @@ -88,6 +88,7 @@
>  #include <linux/sockios.h>
>  #include <linux/errno.h>
>  #include <linux/interrupt.h>
> +#include <linux/if_bonding.h>
>  #include <linux/if_ether.h>
>  #include <linux/netdevice.h>
>  #include <linux/etherdevice.h>
> @@ -1518,37 +1519,6 @@
> 
>  EXPORT_SYMBOL(netif_rx_ni);
> 
> -static inline struct net_device *skb_bond(struct sk_buff *skb)
> -{
> -       struct net_device *dev = skb->dev;
> -
> -       if (dev->master) {
> -               /*
> -                * On bonding slaves other than the currently active
> -                * slave, suppress duplicates except for 802.3ad
> -                * ETH_P_SLOW and alb non-mcast/bcast.
> -                */
> -               if (dev->priv_flags & IFF_SLAVE_INACTIVE) {
> -                       if (dev->master->priv_flags & IFF_MASTER_ALB) {
> -                               if (skb->pkt_type != PACKET_BROADCAST &&
> -                                   skb->pkt_type != PACKET_MULTICAST)
> -                                       goto keep;
> -                       }
> -
> -                       if (dev->master->priv_flags & IFF_MASTER_8023AD &&
> -                           skb->protocol == __constant_htons(ETH_P_SLOW))
> -                               goto keep;
> -
> -                       kfree_skb(skb);
> -                       return NULL;
> -               }
> -keep:
> -               skb->dev = dev->master;
> -       }
> -
> -       return dev;
> -}
> -
>  static void net_tx_action(struct softirq_action *h)
>  {
>         struct softnet_data *sd = &__get_cpu_var(softnet_data);
> 


-- 
Ben Greear <greearb@candelatech.com>
Candela Technologies Inc  http://www.candelatech.com

Re: PATCH Fix bonding active-backup behavior for VLAN interfaces

[Christophe Devriese]

[-- Attachment #1: Type: text/plain, Size: 2792 bytes --]

I basically move the skb_bond method into if_bonding.h, include that file
in if_vlan ( and call it from the vlan forwarding path, and the netif_rx
routine ). 

Somehow this patch is very incomplete. Let me try again.

sorry for the trouble. (I'm new at this)

Regards,

Christophe 

> Christophe Devriese wrote:
> >diff -rU3 linux-2.6.17.7/net/core/dev.c 
> >linux-2.6.17.7-wapper/net/core/dev.c
> >--- linux-2.6.17.7/net/core/dev.c       2006-07-25 05:36:01.000000000 +0200
> >+++ linux-2.6.17.7-wapper/net/core/dev.c        2006-07-27 
> >20:16:36.000000000 +0200
> >@@ -88,6 +88,7 @@
> > #include <linux/sockios.h>
> > #include <linux/errno.h>
> > #include <linux/interrupt.h>
> >+#include <linux/if_bonding.h>
> > #include <linux/if_ether.h>
> > #include <linux/netdevice.h>
> > #include <linux/etherdevice.h>
> >@@ -1518,37 +1519,6 @@
> >
> > EXPORT_SYMBOL(netif_rx_ni);
> >
> >-static inline struct net_device *skb_bond(struct sk_buff *skb)
> >-{
> >-       struct net_device *dev = skb->dev;
> >-
> >-       if (dev->master) {
> >-               /*
> >-                * On bonding slaves other than the currently active
> >-                * slave, suppress duplicates except for 802.3ad
> >-                * ETH_P_SLOW and alb non-mcast/bcast.
> >-                */
> >-               if (dev->priv_flags & IFF_SLAVE_INACTIVE) {
> >-                       if (dev->master->priv_flags & IFF_MASTER_ALB) {
> >-                               if (skb->pkt_type != PACKET_BROADCAST &&
> >-                                   skb->pkt_type != PACKET_MULTICAST)
> >-                                       goto keep;
> >-                       }
> >-
> >-                       if (dev->master->priv_flags & IFF_MASTER_8023AD &&
> >-                           skb->protocol == __constant_htons(ETH_P_SLOW))
> >-                               goto keep;
> >-
> >-                       kfree_skb(skb);
> >-                       return NULL;
> >-               }
> >-keep:
> >-               skb->dev = dev->master;
> >-       }
> >-
> >-       return dev;
> >-}
> >-
> > static void net_tx_action(struct softirq_action *h)
> > {
> >        struct softnet_data *sd = &__get_cpu_var(softnet_data);
> >
> 
> 
> -- 
> Ben Greear <greearb@candelatech.com>
> Candela Technologies Inc  http://www.candelatech.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

-- 
-----------------------------------------------------------------------
Christophe Devriese                                               EURiD
Network Adminstrator / Developer
christophe.devriese@eth1.org
---------------------------- http://www.eth1.org ----------------------


[-- Attachment #2: skbbondvlan.patch --]
[-- Type: text/plain, Size: 3474 bytes --]

diff -rU3 linux-2.6.17.7/include/linux/if_bonding.h linux-2.6.17.7-wapper/include/linux/if_bonding.h
--- linux-2.6.17.7/include/linux/if_bonding.h	2006-07-25 05:36:01.000000000 +0200
+++ linux-2.6.17.7-wapper/include/linux/if_bonding.h	2006-07-27 21:17:25.000000000 +0200
@@ -46,6 +46,7 @@
 #include <linux/if.h>
 #include <linux/types.h>
 #include <linux/if_ether.h>
+#include <linux/netdevice.h>
 
 /* userland - kernel ABI version (2003/05/08) */
 #define BOND_ABI_VERSION 2
@@ -110,6 +111,37 @@
 	__u8 partner_system[ETH_ALEN];
 };
 
+static inline struct net_device *skb_bond(struct sk_buff *skb)
+{
+	struct net_device *dev = skb->dev;
+
+	if (dev->master) {
+		/*
+		 * On bonding slaves other than the currently active
+		 * slave, suppress duplicates except for 802.3ad
+		 * ETH_P_SLOW and alb non-mcast/bcast.
+		 */
+		if (dev->priv_flags & IFF_SLAVE_INACTIVE) {
+			if (dev->master->priv_flags & IFF_MASTER_ALB) {
+				if (skb->pkt_type != PACKET_BROADCAST &&
+				    skb->pkt_type != PACKET_MULTICAST)
+					goto keep;
+			}
+
+			if (dev->master->priv_flags & IFF_MASTER_8023AD &&
+			    skb->protocol == __constant_htons(ETH_P_SLOW))
+				goto keep;
+		
+			kfree_skb(skb);
+			return NULL;
+		}
+keep:
+		skb->dev = dev->master;
+	}
+
+	return dev;
+}
+
 #endif /* _LINUX_IF_BONDING_H */
 
 /*
diff -rU3 linux-2.6.17.7/include/linux/if_vlan.h linux-2.6.17.7-wapper/include/linux/if_vlan.h
--- linux-2.6.17.7/include/linux/if_vlan.h	2006-07-25 05:36:01.000000000 +0200
+++ linux-2.6.17.7-wapper/include/linux/if_vlan.h	2006-07-27 20:16:36.000000000 +0200
@@ -24,6 +24,7 @@
 struct hlist_node;
 
 #include <linux/proc_fs.h> /* for proc_dir_entry */
+#include <linux/if_bonding.h> /* for skb_bond */
 #include <linux/netdevice.h>
 
 #define VLAN_HLEN	4		/* The additional bytes (on top of the Ethernet header)
@@ -154,6 +155,12 @@
 				    unsigned short vlan_tag, int polling)
 {
 	struct net_device_stats *stats;
+	struct net_device *orig_dev;
+
+	orig_dev = skb_bond(skb);
+
+	if (!orig_dev) 
+		return NET_RX_DROP;
 
 	skb->dev = grp->vlan_devices[vlan_tag & VLAN_VID_MASK];
 	if (skb->dev == NULL) {
diff -rU3 linux-2.6.17.7/net/core/dev.c linux-2.6.17.7-wapper/net/core/dev.c
--- linux-2.6.17.7/net/core/dev.c	2006-07-25 05:36:01.000000000 +0200
+++ linux-2.6.17.7-wapper/net/core/dev.c	2006-07-27 20:16:36.000000000 +0200
@@ -88,6 +88,7 @@
 #include <linux/sockios.h>
 #include <linux/errno.h>
 #include <linux/interrupt.h>
+#include <linux/if_bonding.h>
 #include <linux/if_ether.h>
 #include <linux/netdevice.h>
 #include <linux/etherdevice.h>
@@ -1518,37 +1519,6 @@
 
 EXPORT_SYMBOL(netif_rx_ni);
 
-static inline struct net_device *skb_bond(struct sk_buff *skb)
-{
-	struct net_device *dev = skb->dev;
-
-	if (dev->master) {
-		/*
-		 * On bonding slaves other than the currently active
-		 * slave, suppress duplicates except for 802.3ad
-		 * ETH_P_SLOW and alb non-mcast/bcast.
-		 */
-		if (dev->priv_flags & IFF_SLAVE_INACTIVE) {
-			if (dev->master->priv_flags & IFF_MASTER_ALB) {
-				if (skb->pkt_type != PACKET_BROADCAST &&
-				    skb->pkt_type != PACKET_MULTICAST)
-					goto keep;
-			}
-
-			if (dev->master->priv_flags & IFF_MASTER_8023AD &&
-			    skb->protocol == __constant_htons(ETH_P_SLOW))
-				goto keep;
-		
-			kfree_skb(skb);
-			return NULL;
-		}
-keep:
-		skb->dev = dev->master;
-	}
-
-	return dev;
-}
-
 static void net_tx_action(struct softirq_action *h)
 {
 	struct softnet_data *sd = &__get_cpu_var(softnet_data);

Re: PATCH Fix bonding active-backup behavior for VLAN interfaces

[Ben Greear]

Christophe Devriese wrote:
> I basically move the skb_bond method into if_bonding.h, include that file
> in if_vlan ( and call it from the vlan forwarding path, and the netif_rx
> routine ). 
> 
> Somehow this patch is very incomplete. Let me try again.

The patch looks sane this time.

The skb_bond method assigns skb->dev when it does the 'keep',
but the VLAN code immediately over-writes the skb->dev when
searching for the vlan device.

What is the purpose of assinging skb->dev to the master device?

Ben

-- 
Ben Greear <greearb@candelatech.com>
Candela Technologies Inc  http://www.candelatech.com

Re: PATCH Fix bonding active-backup behavior for VLAN interfaces

[Christophe Devriese]

On Fri, Jul 28, 2006 at 02:55:17PM -0700, Ben Greear wrote:
> Christophe Devriese wrote:
> >I basically move the skb_bond method into if_bonding.h, include that file
> >in if_vlan ( and call it from the vlan forwarding path, and the netif_rx
> >routine ). 
> >
> >Somehow this patch is very incomplete. Let me try again.
> 
> The patch looks sane this time.
> 
> The skb_bond method assigns skb->dev when it does the 'keep',
> but the VLAN code immediately over-writes the skb->dev when
> searching for the vlan device.
> 
> What is the purpose of assinging skb->dev to the master device?

I don't know. The method was only moved by this patch, not changed. The
contents of the method are exactly what they are in
linux-2.6.17.7/net/core/dev.c 

I assume it has something to do with the other bonding methods.

-- 
-----------------------------------------------------------------------
Christophe Devriese                                               EURiD
Network Adminstrator / Developer
christophe.devriese@eth1.org
---------------------------- http://www.eth1.org ----------------------

Re: PATCH Fix bonding active-backup behavior for VLAN interfaces

[Christophe Devriese]

On Fri, Jul 28, 2006 at 03:08:49PM -0700, Ben Greear wrote:
> Christophe Devriese wrote:
> >On Fri, Jul 28, 2006 at 02:55:17PM -0700, Ben Greear wrote:
> >
> >>Christophe Devriese wrote:
> >>
> >>>I basically move the skb_bond method into if_bonding.h, include that file
> >>>in if_vlan ( and call it from the vlan forwarding path, and the netif_rx
> >>>routine ). 
> >>>
> >>>Somehow this patch is very incomplete. Let me try again.
> >>
> >>The patch looks sane this time.
> >>
> >>The skb_bond method assigns skb->dev when it does the 'keep',
> >>but the VLAN code immediately over-writes the skb->dev when
> >>searching for the vlan device.
> >>
> >>What is the purpose of assinging skb->dev to the master device?
> >
> >
> >I don't know. The method was only moved by this patch, not changed. The
> >contents of the method are exactly what they are in
> >linux-2.6.17.7/net/core/dev.c 
> >
> >I assume it has something to do with the other bonding methods.
> 
> Ok, I don't know much about the bonding logic.  Looks OK to me.

Will you sign-off on it then ? Or how do I get this applied ?

-- 
-----------------------------------------------------------------------
Christophe Devriese                                               EURiD
Network Adminstrator / Developer
christophe.devriese@eth1.org
---------------------------- http://www.eth1.org ----------------------

Re: PATCH Fix bonding active-backup behavior for VLAN interfaces

[David Miller]

From: Christophe Devriese <christophe.devriese@eth1.org>
Date: Sat, 29 Jul 2006 00:58:59 +0200

> On Fri, Jul 28, 2006 at 03:08:49PM -0700, Ben Greear wrote:
> > Christophe Devriese wrote:
> > >On Fri, Jul 28, 2006 at 02:55:17PM -0700, Ben Greear wrote:
> > >
> > >>Christophe Devriese wrote:
> > >>
> > >>>I basically move the skb_bond method into if_bonding.h, include that file
> > >>>in if_vlan ( and call it from the vlan forwarding path, and the netif_rx
> > >>>routine ). 
> > >>>
> > >>>Somehow this patch is very incomplete. Let me try again.
> > >>
> > >>The patch looks sane this time.
> > >>
> > >>The skb_bond method assigns skb->dev when it does the 'keep',
> > >>but the VLAN code immediately over-writes the skb->dev when
> > >>searching for the vlan device.
> > >>
> > >>What is the purpose of assinging skb->dev to the master device?
> > >
> > >
> > >I don't know. The method was only moved by this patch, not changed. The
> > >contents of the method are exactly what they are in
> > >linux-2.6.17.7/net/core/dev.c 
> > >
> > >I assume it has something to do with the other bonding methods.
> > 
> > Ok, I don't know much about the bonding logic.  Looks OK to me.
> 
> Will you sign-off on it then ? Or how do I get this applied ?

I'll apply this over the weekend unless I spot some problem
with it, thanks.

A sign off from Ben would be nice too :)

Re: PATCH Fix bonding active-backup behavior for VLAN interfaces

[Ben Greear]

David Miller wrote:
> From: Christophe Devriese <christophe.devriese@eth1.org>
> Date: Sat, 29 Jul 2006 00:58:59 +0200
> 
> 
>>On Fri, Jul 28, 2006 at 03:08:49PM -0700, Ben Greear wrote:
>>
>>>Christophe Devriese wrote:
>>>
>>>>On Fri, Jul 28, 2006 at 02:55:17PM -0700, Ben Greear wrote:
>>>>
>>>>
>>>>>Christophe Devriese wrote:
>>>>>
>>>>>
>>>>>>I basically move the skb_bond method into if_bonding.h, include that file
>>>>>>in if_vlan ( and call it from the vlan forwarding path, and the netif_rx
>>>>>>routine ). 
>>>>>>
>>>>>>Somehow this patch is very incomplete. Let me try again.
>>>>>
>>>>>The patch looks sane this time.
>>>>>
>>>>>The skb_bond method assigns skb->dev when it does the 'keep',
>>>>>but the VLAN code immediately over-writes the skb->dev when
>>>>>searching for the vlan device.
>>>>>
>>>>>What is the purpose of assinging skb->dev to the master device?
>>>>
>>>>
>>>>I don't know. The method was only moved by this patch, not changed. The
>>>>contents of the method are exactly what they are in
>>>>linux-2.6.17.7/net/core/dev.c 
>>>>
>>>>I assume it has something to do with the other bonding methods.
>>>
>>>Ok, I don't know much about the bonding logic.  Looks OK to me.
>>
>>Will you sign-off on it then ? Or how do I get this applied ?
> 
> 
> I'll apply this over the weekend unless I spot some problem
> with it, thanks.
> 
> A sign off from Ben would be nice too :)

I don't see any problems with the patch.  The skb->dev
assignment is redundant for the VLAN path, but may be useful
elsewhere.  At any rate, it doesn't seem like it would hurt
anything.

Signed-off-by Ben Greear <greearb@candelatech.com>


-- 
Ben Greear <greearb@candelatech.com>
Candela Technologies Inc  http://www.candelatech.com

Re: PATCH Fix bonding active-backup behavior for VLAN interfaces

[David Miller]

From: Ben Greear <greearb@candelatech.com>
Date: Fri, 28 Jul 2006 14:55:17 -0700

> The skb_bond method assigns skb->dev when it does the 'keep',
> but the VLAN code immediately over-writes the skb->dev when
> searching for the vlan device.
> 
> What is the purpose of assinging skb->dev to the master device?

This makes me consider this patch highly dubious, at best.

The whole intention of bonding on input is to make all packets
incoming on the individual bond slaves to look like they come in via
the master device.

Therefore, even when the bond slaves are VLAN devices, in the end the
skb->dev should be the bond master device _not_ the VLAN device.

I'm not applying this patch, it doesn't look correct at all.

Re: PATCH Fix bonding active-backup behavior for VLAN interfaces

[Christophe Devriese]

On Monday 31 July 2006 05:50, you wrote:
> From: Ben Greear <greearb@candelatech.com>
> Date: Fri, 28 Jul 2006 14:55:17 -0700
>
> > The skb_bond method assigns skb->dev when it does the 'keep',
> > but the VLAN code immediately over-writes the skb->dev when
> > searching for the vlan device.
> >
> > What is the purpose of assinging skb->dev to the master device?
>
> This makes me consider this patch highly dubious, at best.
>
> The whole intention of bonding on input is to make all packets
> incoming on the individual bond slaves to look like they come in via
> the master device.
>
> Therefore, even when the bond slaves are VLAN devices, in the end the
> skb->dev should be the bond master device _not_ the VLAN device.
>
> I'm not applying this patch, it doesn't look correct at all.

That code is not introduced by this patch, but is already in the kernel. This 
patch is about having the same behavior for the vlan accelerated input path 
and the normal input path.

If you bond 2 vlan subinterfaces, the patch is not necessary at all. In that 
case also the source device will be changed from eth0.<vlan> to bond<x>. So 
that's correct behavior no ?

In the second case, you create vlan subifs on a bonding device, vlan 
subinterfaces will be created on the slave interfaces. In that case the vlan 
code will reassign the skb->dev node, and because skb_bond needs to know the 
actual input device in order to make an informed drop decision before passing 
this code (skb active-backup mode needs to drop packets from the backup slave 
interface, if you don't do that you get big problems with broadcasts). 

The same struct vlan_group is assigned to all slave devices and so the only 
vlan subinterfaces that exist in this case are the bond<x>.<vlan> 
subinterfaces, and the vlan path for both slaves will assign the 
bond<x>.<vlan> interface to skb->dev, thereby erasing the information about 
where the packet came from.

I have tested the patch, and it works correctly, in both cases on my test 
sytem (where I join vlan subifs on a bonding device into a bridge and have 
xen guests' vifX.Y interfaces connected to those bridges, which is a 
configuration we imho really want to support) (without this patch, as 
explained earlier in this thread, this config does not work)

Regards,

Christophe

Linville's L2 rant... -- Re: PATCH Fix bonding active-backup behavior for VLAN interfaces

[John W. Linville]

On Mon, Jul 31, 2006 at 10:15:40AM +0200, Christophe Devriese wrote:

> If you bond 2 vlan subinterfaces, the patch is not necessary at all. In that 
> case also the source device will be changed from eth0.<vlan> to bond<x>. So 
> that's correct behavior no ?
> 
> In the second case, you create vlan subifs on a bonding device, vlan 
> subinterfaces will be created on the slave interfaces. In that case the vlan 

(This is not directed at Christophe, or anyone in particular...)

<rant>

Am I the only one that thinks that our handling of LAN L2 stuff
is at best a little "too" flexible (and at worst a collection of
nasty hacks)?

I mean, do we really need both the ability to bond multiple vlan
interfaces AND the ability to have vlan interfaces on top of a bond?
How many people really appreciate the subtle(?) differences?

Then throw bridging into the mix!  If I'm using VLANs and bonds in
a bridged environment, do I bridge the bonds, or bond the bridges?
Do the VLANs come before the bonds?  after the bridges?  or somewhere
in-between?  Do all these combinations even work together?  Who has
the definitive answer (besides the code itself)?

I have no doubt that there are plenty of opportunities for cleverness
here (and no doubt dragons too).  I just doubt that most of them
are worth the complexities introduced by our current collection of
"transparently" stackable pseudo-drivers and strategically placed hacks
(e.g. skb_bond).  All that, and it still isn't clear to me how we
can cleanly accomodate 802.1s (which adds VLAN awareness to bridging).

Do we hold the view that our L2 code is on par with the rest of
our code?  Is there an appetite for a clean-up?  Or is it just me?

</rant>

If you made it this far, thanks for listening...I feel better now. :-)

John
-- 
John W. Linville
linville@tuxdriver.com

Re: Linville's L2 rant... -- Re: PATCH Fix bonding active-backup behavior for VLAN interfaces

[Christophe Devriese]

On Monday 31 July 2006 14:30, you wrote:
> (This is not directed at Christophe, or anyone in particular...)
>
> <rant>
>
> Am I the only one that thinks that our handling of LAN L2 stuff
> is at best a little "too" flexible (and at worst a collection of
> nasty hacks)?
>
> I mean, do we really need both the ability to bond multiple vlan
> interfaces AND the ability to have vlan interfaces on top of a bond?
> How many people really appreciate the subtle(?) differences?
>
> Then throw bridging into the mix!  If I'm using VLANs and bonds in
> a bridged environment, do I bridge the bonds, or bond the bridges?

In all honesty, you cannot bond bridges :-p

> Do the VLANs come before the bonds?  after the bridges?  or somewhere
> in-between?  Do all these combinations even work together?  Who has
> the definitive answer (besides the code itself)?
>
> I have no doubt that there are plenty of opportunities for cleverness
> here (and no doubt dragons too).  I just doubt that most of them
> are worth the complexities introduced by our current collection of
> "transparently" stackable pseudo-drivers and strategically placed hacks
> (e.g. skb_bond).  All that, and it still isn't clear to me how we
> can cleanly accomodate 802.1s (which adds VLAN awareness to bridging).
>
> Do we hold the view that our L2 code is on par with the rest of
> our code?  Is there an appetite for a clean-up?  Or is it just me?

A vlan capable bridge with trunk ports and access ports would be nice :-p

I think the current code is nice. You need it to properly support 
virtualization and I find it very useful where I work to have this option.

Regards,

Christophe

Re: Linville's L2 rant... -- Re: PATCH Fix bonding active-backup behavior for VLAN interfaces

[Jamal Hadi Salim]

On Mon, 2006-31-07 at 08:30 -0400, John W. Linville wrote:
> On Mon, Jul 31, 2006 at 10:15:40AM +0200, Christophe Devriese wrote:
> 
> > If you bond 2 vlan subinterfaces, the patch is not necessary at all. In that 
> > case also the source device will be changed from eth0.<vlan> to bond<x>. So 
> > that's correct behavior no ?
> > 
> > In the second case, you create vlan subifs on a bonding device, vlan 
> > subinterfaces will be created on the slave interfaces. In that case the vlan 
> 
> (This is not directed at Christophe, or anyone in particular...)
> 
> <rant>
> 
> Am I the only one that thinks that our handling of LAN L2 stuff
> is at best a little "too" flexible (and at worst a collection of
> nasty hacks)?
> 
> I mean, do we really need both the ability to bond multiple vlan
> interfaces AND the ability to have vlan interfaces on top of a bond?
> How many people really appreciate the subtle(?) differences?
> 
> Then throw bridging into the mix!  If I'm using VLANs and bonds in
> a bridged environment, do I bridge the bonds, or bond the bridges?
> Do the VLANs come before the bonds?  after the bridges?  or somewhere
> in-between?  Do all these combinations even work together?  Who has
> the definitive answer (besides the code itself)?
> 
> I have no doubt that there are plenty of opportunities for cleverness
> here (and no doubt dragons too).  I just doubt that most of them
> are worth the complexities introduced by our current collection of
> "transparently" stackable pseudo-drivers and strategically placed hacks
> (e.g. skb_bond).  All that, and it still isn't clear to me how we
> can cleanly accomodate 802.1s (which adds VLAN awareness to bridging).
> 
> Do we hold the view that our L2 code is on par with the rest of
> our code?  Is there an appetite for a clean-up?  Or is it just me?
> 
> </rant>
> 
> If you made it this far, thanks for listening...I feel better now. :-)

Yes, I made it this far and you do make good arguement (or i may be
over-dosed ;->).
I have seen the following setups that are useful:

1) Vlans with bridges; in which one or more vlans exist per ethernet
port. Broadcast packets within such vlans are restricted to just those
vlans by the bridge.
2) complicate the above a little by having multiple spanning trees. 
3) Add to the above link layer HA (802.1ad or otherwise as presented
today by Bonding).

To answer your question; i think yes we need all 3.
Unfortunately the 3 above are all done by different people with
different intentions altogether. I think BGrears end goal was VLANs for
an end host. I think Lennert wrote the original Bridge code and for a
while had some VLAN code that worked well with bridging (that code died
as far as i know). Then bonding - theres some pre-historic relation to
it since D Becker days and then the good folks from Intel adding about
1M features to it. Yes, the fact all 3 need to work together is a
mess ;-> (but there are good pragmatic reasons for them to work
together)...
Hope that helps ;->

cheers,
jamal

Re: Linville's L2 rant... -- Re: PATCH Fix bonding active-backup behavior for VLAN interfaces

[John W. Linville]

On Mon, Jul 31, 2006 at 09:39:08PM -0400, Jamal Hadi Salim wrote:
> On Mon, 2006-31-07 at 08:30 -0400, John W. Linville wrote:

> > Do we hold the view that our L2 code is on par with the rest of
> > our code?  Is there an appetite for a clean-up?  Or is it just me?
> > 
> > </rant>
> > 
> > If you made it this far, thanks for listening...I feel better now. :-)
> 
> Yes, I made it this far and you do make good arguement (or i may be
> over-dosed ;->).
> I have seen the following setups that are useful:
> 
> 1) Vlans with bridges; in which one or more vlans exist per ethernet
> port. Broadcast packets within such vlans are restricted to just those
> vlans by the bridge.
> 2) complicate the above a little by having multiple spanning trees. 
> 3) Add to the above link layer HA (802.1ad or otherwise as presented
> today by Bonding).
> 
> To answer your question; i think yes we need all 3.

Oh, don't get me wrong -- I definitely think we need all three.

I'm just not sure we need every conceivable combination of a) bonds
of vlan interfaces; b) vlan interfaces on top of bonds; c) bridged
vlan interfaces w/ disparate vlan IDs; d) bonded vlan interfaces w/
disparate vlan IDs; e) bonded bridge interfaces (does this work?) f)
bonded bonds (seen customers trying to do it); g) bridged vlan
interfaces; h) bridged bonds; i) bridged bridges (probably doesn't
work, but someone probably wants it); j) vlan interfaces on top of
bridges; k) vlan interfaces on top of vlans (double vlan tagging);
and, l) what am I leaving out?

Most (actually all afaik) L2 networking equipment enforces some
hierarchy on the relationship between these L2 entities.  I am more
and more convinced we should do the same, although I do acknowledge
that the current situation does allow for some cleverness.

I'm just not sure that cleverness is worth the headache, especially
since the most clever things usually only work by accident...

> Unfortunately the 3 above are all done by different people with
> different intentions altogether. I think BGrears end goal was VLANs for
> an end host. I think Lennert wrote the original Bridge code and for a
> while had some VLAN code that worked well with bridging (that code died
> as far as i know). Then bonding - theres some pre-historic relation to
> it since D Becker days and then the good folks from Intel adding about
> 1M features to it. Yes, the fact all 3 need to work together is a
> mess ;-> (but there are good pragmatic reasons for them to work
> together)...

I'm sure you are correct -- each entity was developed to serve its
purpose, and each does so admirably on its own.  The fact that they
work together is a desirable miracle.

There is no doubt that we need to be able to do all three (vlan,
bridge, bond) at once.  I'm just not convinced we need to support
stacking them in every conceivable order.  And, I think that a
reconsideration of all three functions as a group could lead to
better/cleaner functionality with easier support for extension (e.g.
802.1s).

Well, I'll guit now before I get sent-off to the visionaries list.
I am putting some thought to this, but I'm not yet far enough along
to sound coherent... :-)

John
-- 
John W. Linville
linville@tuxdriver.com

Re: Linville's L2 rant... -- Re: PATCH Fix bonding active-backup behavior for VLAN interfaces

[Jamal Hadi Salim]

On Tue, 2006-01-08 at 08:08 -0400, John W. Linville wrote:
[..]
> There is no doubt that we need to be able to do all three (vlan,
> bridge, bond) at once.  I'm just not convinced we need to support
> stacking them in every conceivable order.  

In theory there should be no issues stacking netdevices in any order
you want. In other words the hooks for doing so exist (albeit in some
limited way[1]). Practically, some of the topologies of interconnected
netdevices dont make a lot of sense. The danger is in restricting how
the stacking happens and overlooking some future creative use.
Safer to let the user own the policy and configure any way they want aka
"shoot themselves in the foot".

> And, I think that a
> reconsideration of all three functions as a group could lead to
> better/cleaner functionality with easier support for extension (e.g.
> 802.1s).

Agreed. I have some very strong opinions on this subject that i could
share with you if you want. For example, IMO, I think it would be a lot
reasonable to assume that a VLAN or VLANS are attributes of a netdevice
(just like IP addresses or MAC addresses are). 

cheers,
jamal

Re: Linville's L2 rant... -- Re: PATCH Fix bonding active-backup behavior for VLAN interfaces

[Ben Greear]

Jamal Hadi Salim wrote:
> On Tue, 2006-01-08 at 08:08 -0400, John W. Linville wrote:
> [..]
> 
>>There is no doubt that we need to be able to do all three (vlan,
>>bridge, bond) at once.  I'm just not convinced we need to support
>>stacking them in every conceivable order.  
> 
> 
> In theory there should be no issues stacking netdevices in any order
> you want. In other words the hooks for doing so exist (albeit in some
> limited way[1]). Practically, some of the topologies of interconnected
> netdevices dont make a lot of sense. The danger is in restricting how
> the stacking happens and overlooking some future creative use.
> Safer to let the user own the policy and configure any way they want aka
> "shoot themselves in the foot".
> 
> 
>>And, I think that a
>>reconsideration of all three functions as a group could lead to
>>better/cleaner functionality with easier support for extension (e.g.
>>802.1s).
> 
> 
> Agreed. I have some very strong opinions on this subject that i could
> share with you if you want. For example, IMO, I think it would be a lot
> reasonable to assume that a VLAN or VLANS are attributes of a netdevice
> (just like IP addresses or MAC addresses are). 

As might be expected, I feel that VLANs are much more
useful as full-featured net devices.  I do not believe it is worth
decreasing functionality to try to 'clean up' the code.  There are people
doing interesting things with the mentioned virtual devices that the original
developers of the individual parts never envisioned, but I see that only
as a resounding success and validation of the architecture.

It is true that there are some interesting issues about where you add
the hooks to slurp up vlan, bridged, bonded and other type of virtual
device packets.

At least for some of my out-of-the-tree virtual lan devices (mac-vlan, in particular),
I thought it would be useful to allow dynamic registration of the layer-2 hooks such
as bridging.  This way, where there is no logical way to determine which virtual interface
should get first chance at a packet, the user can provide the ordering by adjusting
where the hooks sit in the chain.

Last time I mentioned this feature, it was pointed out that the net-filter hooks provide,
or come close to providing, this ability to stack.  If someone wants to work on
virtual device priority, it might be worth investigating this further and create an
API that makes this usable from kernel and user-space.

Thanks,
Ben

-- 
Ben Greear <greearb@candelatech.com>
Candela Technologies Inc  http://www.candelatech.com

Re: Linville's L2 rant... -- Re: PATCH Fix bonding active-backup behavior for VLAN interfaces

[John W. Linville]

On Tue, Aug 01, 2006 at 09:10:06AM -0700, Ben Greear wrote:
> Jamal Hadi Salim wrote:

> >Agreed. I have some very strong opinions on this subject that i could
> >share with you if you want. For example, IMO, I think it would be a lot
> >reasonable to assume that a VLAN or VLANS are attributes of a netdevice
> >(just like IP addresses or MAC addresses are). 
> 
> As might be expected, I feel that VLANs are much more
> useful as full-featured net devices.  I do not believe it is worth
> decreasing functionality to try to 'clean up' the code.

In general, I agree that we shouldn't lose functionality.

I'm curious as to what particularly functionality you fear would be
lost if VLANs were not implemented the way they are now?

Thanks,

John
-- 
John W. Linville
linville@tuxdriver.com

Re: Linville's L2 rant... -- Re: PATCH Fix bonding active-backup behavior for VLAN interfaces

[Ben Greear]

John W. Linville wrote:
> On Tue, Aug 01, 2006 at 09:10:06AM -0700, Ben Greear wrote:
> 
>>Jamal Hadi Salim wrote:
> 
> 
>>>Agreed. I have some very strong opinions on this subject that i could
>>>share with you if you want. For example, IMO, I think it would be a lot
>>>reasonable to assume that a VLAN or VLANS are attributes of a netdevice
>>>(just like IP addresses or MAC addresses are). 
>>
>>As might be expected, I feel that VLANs are much more
>>useful as full-featured net devices.  I do not believe it is worth
>>decreasing functionality to try to 'clean up' the code.
> 
> 
> In general, I agree that we shouldn't lose functionality.
> 
> I'm curious as to what particularly functionality you fear would be
> lost if VLANs were not implemented the way they are now?

Well, Jamal and I and others discussed this in depth in the 2.4.12 time frame
when VLANs where about to go into the kernel.  Basically, my point is that
if VLANs are true devices, they will just work with all of the user-space protocols
and they will easily handle abstractions such as bridges, (multiple) IP addresses, MAC addresses,
net-filter, and all the rest.

Sounds like Jamal still remembers his reasons for wanting it otherwise...so
will let him describe his reasons.

Nothing is set in stone in Linux, and I am certainly not the final arbiter of
what gets into the linux kernel, but in my opinion, the current VLAN architecture
is supperior to the ip-alias model, and I see no reason to make any significant
changes.

Ben

> 
> Thanks,
> 
> John


-- 
Ben Greear <greearb@candelatech.com>
Candela Technologies Inc  http://www.candelatech.com

Re: Linville's L2 rant... -- Re: PATCH Fix bonding active-backup behavior for VLAN interfaces

[Krzysztof Halasa]

Ben Greear <greearb@candelatech.com> writes:

> Basically, my point is that
> if VLANs are true devices, they will just work with all of the
> user-space protocols
> and they will easily handle abstractions such as bridges, (multiple)
> IP addresses, MAC addresses,
> net-filter, and all the rest.

<AOL mode> I think the same.
-- 
Krzysztof Halasa

Re: Linville's L2 rant... -- Re: PATCH Fix bonding active-backup behavior for VLAN interfaces

[John W. Linville]

On Tue, Aug 01, 2006 at 08:33:34AM -0400, Jamal Hadi Salim wrote:
> On Tue, 2006-01-08 at 08:08 -0400, John W. Linville wrote:

> > And, I think that a
> > reconsideration of all three functions as a group could lead to
> > better/cleaner functionality with easier support for extension (e.g.
> > 802.1s).
> 
> Agreed. I have some very strong opinions on this subject that i could
> share with you if you want. For example, IMO, I think it would be a lot
> reasonable to assume that a VLAN or VLANS are attributes of a netdevice
> (just like IP addresses or MAC addresses are). 

I'd love to hear them.  Feel free to send them off list, since I know
how shy you can be... :-)

John
-- 
John W. Linville
linville@tuxdriver.com

Re: Linville's L2 rant... -- Re: PATCH Fix bonding active-backup behavior for VLAN interfaces

[Ben Greear]

John W. Linville wrote:
> On Mon, Jul 31, 2006 at 09:39:08PM -0400, Jamal Hadi Salim wrote:
> 
>>On Mon, 2006-31-07 at 08:30 -0400, John W. Linville wrote:
> 
> 
>>>Do we hold the view that our L2 code is on par with the rest of
>>>our code?  Is there an appetite for a clean-up?  Or is it just me?
>>>
>>></rant>
>>>
>>>If you made it this far, thanks for listening...I feel better now. :-)
>>
>>Yes, I made it this far and you do make good arguement (or i may be
>>over-dosed ;->).
>>I have seen the following setups that are useful:
>>
>>1) Vlans with bridges; in which one or more vlans exist per ethernet
>>port. Broadcast packets within such vlans are restricted to just those
>>vlans by the bridge.
>>2) complicate the above a little by having multiple spanning trees. 
>>3) Add to the above link layer HA (802.1ad or otherwise as presented
>>today by Bonding).
>>
>>To answer your question; i think yes we need all 3.
> 
> 
> Oh, don't get me wrong -- I definitely think we need all three.
> 
> I'm just not sure we need every conceivable combination of a) bonds
> of vlan interfaces; b) vlan interfaces on top of bonds; c) bridged
> vlan interfaces w/ disparate vlan IDs; d) bonded vlan interfaces w/
> disparate vlan IDs; e) bonded bridge interfaces (does this work?) f)
> bonded bonds (seen customers trying to do it); g) bridged vlan
> interfaces; h) bridged bonds; i) bridged bridges (probably doesn't
> work, but someone probably wants it); j) vlan interfaces on top of
> bridges; k) vlan interfaces on top of vlans (double vlan tagging);
> and, l) what am I leaving out?

Well, if it makes you feel better, I can't see a good way to do
vlans-over-vlans cleanly, backwards compatibly, and functional with
bridging, etc.  I would not plan to add such a feature to the kernel
unless from it's moment of inclusion it could handle at least bridging,
either.  So that feature will probably not see the light of day
any time soon :)

> Most (actually all afaik) L2 networking equipment enforces some
> hierarchy on the relationship between these L2 entities.  I am more
> and more convinced we should do the same, although I do acknowledge
> that the current situation does allow for some cleverness.

Very often, the answer to difficult networking issues is to 'get a linux box',
since that very flexibility is often key to interesting problems.

> I'm just not sure that cleverness is worth the headache, especially
> since the most clever things usually only work by accident...

Or, work by solid, modular design and small tweaks!

Thanks,
Ben

-- 
Ben Greear <greearb@candelatech.com>
Candela Technologies Inc  http://www.candelatech.com

Re: Linville's L2 rant... -- Re: PATCH Fix bonding active-backup behavior for VLAN interfaces

[John W. Linville]

On Tue, Aug 01, 2006 at 09:17:15AM -0700, Ben Greear wrote:
> John W. Linville wrote:

> Well, if it makes you feel better, I can't see a good way to do
> vlans-over-vlans cleanly, backwards compatibly, and functional with
> bridging, etc.  I would not plan to add such a feature to the kernel
> unless from it's moment of inclusion it could handle at least bridging,
> either.  So that feature will probably not see the light of day
> any time soon :)

I suspected as much. :-)

> >Most (actually all afaik) L2 networking equipment enforces some
> >hierarchy on the relationship between these L2 entities.  I am more
> >and more convinced we should do the same, although I do acknowledge
> >that the current situation does allow for some cleverness.
> 
> Very often, the answer to difficult networking issues is to 'get a linux 
> box', since that very flexibility is often key to interesting problems.
 
No doubt that is true.  FWIW I think we would still have to be very
flexible about when/how/where vlan tagging gets done, if for no other
reason than that I can't seem to convince myself of any one place it
should go... :-)

Oh, and there may be no reason to ever remove the virtual device vlan
implementation, even if there was a nicer/cleaner/better L2 layer
for the other 90+%.

> >I'm just not sure that cleverness is worth the headache, especially
> >since the most clever things usually only work by accident...
> 
> Or, work by solid, modular design and small tweaks!

Point taken.  But stashing little hacks in the networking core for
specific virtual drivers isn't totally modular either.  And even if
it were, "modular design" probably belongs on the list of "things
that can be taken too far", like "everything in userland", "never
use ioctl", and "microkernels are superior". :-)

I definitely respect your contributions both past and (presumably)
future, and I have no doubt that you have seen any number of
'interesting' vlan scenarios.  Again, my complaint is more about
the overall lack of integration/coherence for L2 functions.  I have
few complaints about individual functions (that I wish to discuss in
this thread).

John
-- 
John W. Linville
linville@tuxdriver.com

Re: Linville's L2 rant... -- Re: PATCH Fix bonding active-backup behavior for VLAN interfaces

[Ben Greear]

John W. Linville wrote:

>>>I'm just not sure that cleverness is worth the headache, especially
>>>since the most clever things usually only work by accident...
>>
>>Or, work by solid, modular design and small tweaks!
> 
> 
> Point taken.  But stashing little hacks in the networking core for
> specific virtual drivers isn't totally modular either.  And even if
> it were, "modular design" probably belongs on the list of "things
> that can be taken too far", like "everything in userland", "never
> use ioctl", and "microkernels are superior". :-)

To be honest, I'm not over-joyed to see bridging hooks included
in the VLAN code..but if that is what it takes to get bridging
and VLANs to play well and be flexible, I think it is a fair price.

It certainly wouldn't hurt to have someone take a holistic view of the
various L2 device interactions.  Just documenting current functionality
on, say, the netdev wiki would be a good first step.

Thanks,
Ben

-- 
Ben Greear <greearb@candelatech.com>
Candela Technologies Inc  http://www.candelatech.com

Re: Linville's L2 rant... -- Re: PATCH Fix bonding active-backup behavior for VLAN interfaces

[Christophe Devriese]

On Tuesday 01 August 2006 19:21, you wrote:
> John W. Linville wrote:
> >>>I'm just not sure that cleverness is worth the headache, especially
> >>>since the most clever things usually only work by accident...
> >>
> >>Or, work by solid, modular design and small tweaks!
> >
> > Point taken.  But stashing little hacks in the networking core for
> > specific virtual drivers isn't totally modular either.  And even if
> > it were, "modular design" probably belongs on the list of "things
> > that can be taken too far", like "everything in userland", "never
> > use ioctl", and "microkernels are superior". :-)
>
> To be honest, I'm not over-joyed to see bridging hooks included
> in the VLAN code..but if that is what it takes to get bridging
> and VLANs to play well and be flexible, I think it is a fair price.
>
> It certainly wouldn't hurt to have someone take a holistic view of the
> various L2 device interactions.  Just documenting current functionality
> on, say, the netdev wiki would be a good first step.

Ultimate flexibility could be provided by making the netif_rx routine (and the 
others, including vlan etc), a "virtual" routine.

That way a list of "filters" could be defined that allow any processing to be 
done on the packet before it is handed of to the linux kernel's higher 
layers, including not delivering it on that interface, or delivering it on 
another interface.

This would allow very complex implementations including stuff like a 
high-level l2 bridge, with vlan support, and a number of protocols like rstp, 
pvst+, ... with relatively simple code, that could be isolated from the main 
kernel.

Would anyone be interested in signing off on such a patch ? (which basically 
creates netif_rx and vlan_acc_netif_rx lists in the net_device structure, and 
then modify bridging and bonding drivers to just use this) 

Regards,

Christophe

Stackable devices.

[Stephen Hemminger]

On Wed, 2 Aug 2006 11:02:20 +0200
Christophe Devriese <Christophe.Devriese@eurid.eu> wrote:

> On Tuesday 01 August 2006 19:21, you wrote:
> > John W. Linville wrote:
> > >>>I'm just not sure that cleverness is worth the headache, especially
> > >>>since the most clever things usually only work by accident...
> > >>
> > >>Or, work by solid, modular design and small tweaks!
> > >
> > > Point taken.  But stashing little hacks in the networking core for
> > > specific virtual drivers isn't totally modular either.  And even if
> > > it were, "modular design" probably belongs on the list of "things
> > > that can be taken too far", like "everything in userland", "never
> > > use ioctl", and "microkernels are superior". :-)
> >
> > To be honest, I'm not over-joyed to see bridging hooks included
> > in the VLAN code..but if that is what it takes to get bridging
> > and VLANs to play well and be flexible, I think it is a fair price.
> >
> > It certainly wouldn't hurt to have someone take a holistic view of the
> > various L2 device interactions.  Just documenting current functionality
> > on, say, the netdev wiki would be a good first step.
> 
> Ultimate flexibility could be provided by making the netif_rx routine (and the 
> others, including vlan etc), a "virtual" routine.
> 
> That way a list of "filters" could be defined that allow any processing to be 
> done on the packet before it is handed of to the linux kernel's higher 
> layers, including not delivering it on that interface, or delivering it on 
> another interface.
> 
> This would allow very complex implementations including stuff like a 
> high-level l2 bridge, with vlan support, and a number of protocols like rstp, 
> pvst+, ... with relatively simple code, that could be isolated from the main 
> kernel.
> 
> Would anyone be interested in signing off on such a patch ? (which basically 
> creates netif_rx and vlan_acc_netif_rx lists in the net_device structure, and 
> then modify bridging and bonding drivers to just use this) 

I have thought about this, but you end up reinventing System V streams.
The problem is for simple up/down call, the stacking model works fine but
once you add flow-control and multiplexing issues the problem becomes complex.
It is hard to think of a good general solution where the performance wouldn't
end up sucking.

-- 
Stephen Hemminger <shemminger@osdl.org>
"And in the Packet there writ down that doome"

Re: Stackable devices.

[Ben Greear]

Stephen Hemminger wrote:
> On Wed, 2 Aug 2006 11:02:20 +0200
> Christophe Devriese <Christophe.Devriese@eurid.eu> wrote:
> 
> 
>>On Tuesday 01 August 2006 19:21, you wrote:
>>
>>>John W. Linville wrote:
>>>
>>>>>>I'm just not sure that cleverness is worth the headache, especially
>>>>>>since the most clever things usually only work by accident...
>>>>>
>>>>>Or, work by solid, modular design and small tweaks!
>>>>
>>>>Point taken.  But stashing little hacks in the networking core for
>>>>specific virtual drivers isn't totally modular either.  And even if
>>>>it were, "modular design" probably belongs on the list of "things
>>>>that can be taken too far", like "everything in userland", "never
>>>>use ioctl", and "microkernels are superior". :-)
>>>
>>>To be honest, I'm not over-joyed to see bridging hooks included
>>>in the VLAN code..but if that is what it takes to get bridging
>>>and VLANs to play well and be flexible, I think it is a fair price.
>>>
>>>It certainly wouldn't hurt to have someone take a holistic view of the
>>>various L2 device interactions.  Just documenting current functionality
>>>on, say, the netdev wiki would be a good first step.
>>
>>Ultimate flexibility could be provided by making the netif_rx routine (and the 
>>others, including vlan etc), a "virtual" routine.
>>
>>That way a list of "filters" could be defined that allow any processing to be 
>>done on the packet before it is handed of to the linux kernel's higher 
>>layers, including not delivering it on that interface, or delivering it on 
>>another interface.
>>
>>This would allow very complex implementations including stuff like a 
>>high-level l2 bridge, with vlan support, and a number of protocols like rstp, 
>>pvst+, ... with relatively simple code, that could be isolated from the main 
>>kernel.
>>
>>Would anyone be interested in signing off on such a patch ? (which basically 
>>creates netif_rx and vlan_acc_netif_rx lists in the net_device structure, and 
>>then modify bridging and bonding drivers to just use this) 
> 
> 
> I have thought about this, but you end up reinventing System V streams.
> The problem is for simple up/down call, the stacking model works fine but
> once you add flow-control and multiplexing issues the problem becomes complex.
> It is hard to think of a good general solution where the performance wouldn't
> end up sucking.

Currently, the bridge hook logic is something like:

if (bridge-consumed-pkt) {
	return
}

// drop through to other layers


There are several other hooks I'd like to see added (pktgen receive processing,
mac-vlans, etc).  Each of these hooks are logically similar to the bridge hook,
ie if it consumes the pkt, return, else, drop through to the next hook untill
we get to the regular protocol processing logic.

I would like to be able to chain layer-2 handlers, such as bridge, mac-vlan,
pktgen such that if one consumed, you break out of the handling, else, you
try the next handler.  The handlers can be dynamically registered and inserted
in any order, controllable by user-space and/or module load/unload.

For many of the handlers, the logic will re-insert the packet by re-calling the
netif-rx logic, so there would need to be some protection to keep loops from
occurring that would recurse too much and overflow the stack.

Ben

-- 
Ben Greear <greearb@candelatech.com>
Candela Technologies Inc  http://www.candelatech.com

Re: Stackable devices.

[Christophe Devriese]

On Wed, Aug 02, 2006 at 10:50:08AM -0700, Ben Greear wrote:
> Currently, the bridge hook logic is something like:
> 
> if (bridge-consumed-pkt) {
> 	return
> }
> 
> // drop through to other layers
> 
> 
> There are several other hooks I'd like to see added (pktgen receive 
> processing,
> mac-vlans, etc).  Each of these hooks are logically similar to the bridge 
> hook,
> ie if it consumes the pkt, return, else, drop through to the next hook 
> untill
> we get to the regular protocol processing logic.
> 
> I would like to be able to chain layer-2 handlers, such as bridge, mac-vlan,
> pktgen such that if one consumed, you break out of the handling, else, you
> try the next handler.  The handlers can be dynamically registered and 
> inserted
> in any order, controllable by user-space and/or module load/unload.
> 
> For many of the handlers, the logic will re-insert the packet by re-calling 
> the
> netif-rx logic, so there would need to be some protection to keep loops from
> occurring that would recurse too much and overflow the stack.

I'm also a big fan of a generalized system like this. It would need to
catch both the vlan accelerated path and the normal path.

-- 
-----------------------------------------------------------------------
Christophe Devriese                                               EURiD
Network Adminstrator / Developer
christophe.devriese@eth1.org
---------------------------- http://www.eth1.org ----------------------

Re: Stackable devices.

[Ben Greear]

Christophe Devriese wrote:
> On Wed, Aug 02, 2006 at 10:50:08AM -0700, Ben Greear wrote:
> 
>>Currently, the bridge hook logic is something like:
>>
>>if (bridge-consumed-pkt) {
>>	return
>>}
>>
>>// drop through to other layers
>>
>>
>>There are several other hooks I'd like to see added (pktgen receive 
>>processing,
>>mac-vlans, etc).  Each of these hooks are logically similar to the bridge 
>>hook,
>>ie if it consumes the pkt, return, else, drop through to the next hook 
>>untill
>>we get to the regular protocol processing logic.
>>
>>I would like to be able to chain layer-2 handlers, such as bridge, mac-vlan,
>>pktgen such that if one consumed, you break out of the handling, else, you
>>try the next handler.  The handlers can be dynamically registered and 
>>inserted
>>in any order, controllable by user-space and/or module load/unload.
>>
>>For many of the handlers, the logic will re-insert the packet by re-calling 
>>the
>>netif-rx logic, so there would need to be some protection to keep loops from
>>occurring that would recurse too much and overflow the stack.
> 
> 
> I'm also a big fan of a generalized system like this. It would need to
> catch both the vlan accelerated path and the normal path.

Well, it isn't actually needed for VLANs since VLAN's hook is a protocol
handler....

Ben

-- 
Ben Greear <greearb@candelatech.com>
Candela Technologies Inc  http://www.candelatech.com

Re: Stackable devices.

[Christophe Devriese]

It would however be considerable effort to do this. Is this going to end up 
unapplied like my last patch, or ?

Regards,

Christophe

On Tuesday 08 August 2006 18:36, you wrote:
> Christophe Devriese wrote:
> > On Wed, Aug 02, 2006 at 10:50:08AM -0700, Ben Greear wrote:
> >>Currently, the bridge hook logic is something like:
> >>
> >>if (bridge-consumed-pkt) {
> >>	return
> >>}
> >>
> >>// drop through to other layers
> >>
> >>
> >>There are several other hooks I'd like to see added (pktgen receive
> >>processing,
> >>mac-vlans, etc).  Each of these hooks are logically similar to the bridge
> >>hook,
> >>ie if it consumes the pkt, return, else, drop through to the next hook
> >>untill
> >>we get to the regular protocol processing logic.
> >>
> >>I would like to be able to chain layer-2 handlers, such as bridge,
> >> mac-vlan, pktgen such that if one consumed, you break out of the
> >> handling, else, you try the next handler.  The handlers can be
> >> dynamically registered and inserted
> >>in any order, controllable by user-space and/or module load/unload.
> >>
> >>For many of the handlers, the logic will re-insert the packet by
> >> re-calling the
> >>netif-rx logic, so there would need to be some protection to keep loops
> >> from occurring that would recurse too much and overflow the stack.
> >
> > I'm also a big fan of a generalized system like this. It would need to
> > catch both the vlan accelerated path and the normal path.
>
> Well, it isn't actually needed for VLANs since VLAN's hook is a protocol
> handler....
>
> Ben

Re: Stackable devices.

[Ben Greear]

Christophe Devriese wrote:
> It would however be considerable effort to do this. Is this going to end up 
> unapplied like my last patch, or ?

I don't get to make this decision..and when I ask such questions...they
are usually ignored unless I also post a working patch....

I think you could start with just supporting bridging without too
much code.

Ben

> 
> Regards,
> 
> Christophe
> 
> On Tuesday 08 August 2006 18:36, you wrote:
> 
>>Christophe Devriese wrote:
>>
>>>On Wed, Aug 02, 2006 at 10:50:08AM -0700, Ben Greear wrote:
>>>
>>>>Currently, the bridge hook logic is something like:
>>>>
>>>>if (bridge-consumed-pkt) {
>>>>	return
>>>>}
>>>>
>>>>// drop through to other layers
>>>>
>>>>
>>>>There are several other hooks I'd like to see added (pktgen receive
>>>>processing,
>>>>mac-vlans, etc).  Each of these hooks are logically similar to the bridge
>>>>hook,
>>>>ie if it consumes the pkt, return, else, drop through to the next hook
>>>>untill
>>>>we get to the regular protocol processing logic.
>>>>
>>>>I would like to be able to chain layer-2 handlers, such as bridge,
>>>>mac-vlan, pktgen such that if one consumed, you break out of the
>>>>handling, else, you try the next handler.  The handlers can be
>>>>dynamically registered and inserted
>>>>in any order, controllable by user-space and/or module load/unload.
>>>>
>>>>For many of the handlers, the logic will re-insert the packet by
>>>>re-calling the
>>>>netif-rx logic, so there would need to be some protection to keep loops
>>>>from occurring that would recurse too much and overflow the stack.
>>>
>>>I'm also a big fan of a generalized system like this. It would need to
>>>catch both the vlan accelerated path and the normal path.
>>
>>Well, it isn't actually needed for VLANs since VLAN's hook is a protocol
>>handler....
>>
>>Ben
> 
> -
> 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
> 

Re: Stackable devices.

[Ben Greear]

Christophe Devriese wrote:
> It would however be considerable effort to do this. Is this going to end up 
> unapplied like my last patch, or ?

I don't get to make this decision..and when I ask such questions...they
are usually ignored unless I also post a working patch....

I think you could start with just supporting bridging without too
much code.

Ben

Re: PATCH Fix bonding active-backup behavior for VLAN interfaces

[David Miller]

From: Christophe Devriese <Christophe.Devriese@eurid.eu>
Date: Mon, 31 Jul 2006 10:15:40 +0200

Thanks for the detailed explanation.

> If you bond 2 vlan subinterfaces, the patch is not necessary at all. In that 
> case also the source device will be changed from eth0.<vlan> to bond<x>. So 
> that's correct behavior no ?
> 
> In the second case, you create vlan subifs on a bonding device, vlan 
> subinterfaces will be created on the slave interfaces. In that case the vlan 
> code will reassign the skb->dev node, and because skb_bond needs to know the 
> actual input device in order to make an informed drop decision before passing 
> this code (skb active-backup mode needs to drop packets from the backup slave 
> interface, if you don't do that you get big problems with broadcasts). 
> 
> The same struct vlan_group is assigned to all slave devices and so the only 
> vlan subinterfaces that exist in this case are the bond<x>.<vlan> 
> subinterfaces, and the vlan path for both slaves will assign the 
> bond<x>.<vlan> interface to skb->dev, thereby erasing the information about 
> where the packet came from.

Assuming it is correct to do the skb_bond() here in the VLAN hwaccel
RX path, then there is still one piece missing from what I can see.

Notice that in the netif_receive_skb() path, the return value from
skb_bond() is used as the third argument to the deliver_skb() routine
and friends which in turn gets passed to the packet_type functions.

Bonding, in particular, makes use of this third argument, see:

bond_3ad_lacpdu_recv()
rlb_arp_recv()

So if the new "orig_dev" you are computing in the VLAN hwaccel RX path
is the correct one, somehow this has to propagate down to the third
argument of the packet type ->func() invocations, right?

Finally, I'm still a little stumped about why this change is necessary
still, to be honest.  When you configure the bond, the slaves should
be the VLAN devices as far as I can tell.  Therefore it should be the
"vlan_device->masster" that we are interested in not the top-level
"dev->master".

If the ethernet is on a VLAN, and the administrator configures the
underlying ethernet device as the slaves of the bond, this to me seems
like a misconfiguration rather than something we should put hacks in
to support.

The fact that you do not propagate the "orig_dev" returned from
skb_bond() down to the packet type functions seems to support this.
>From my perspective, this looks like a hack for a bonding
misconfiguration.

Re: PATCH Fix bonding active-backup behavior for VLAN interfaces

[Krzysztof Oledzki]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 735 bytes --]



On Wed, 2 Aug 2006, David Miller wrote:
<CUT>
> Finally, I'm still a little stumped about why this change is necessary
> still, to be honest.

If I understand it correctly this patch fixes the "[PATCH] bonding: 
suppress duplicate packets" patch:

http://www.kernel.org/git/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commitdiff;h=8f903c708fcc2b579ebf16542bf6109bad593a1d;hp=ebe19a4ed78d4a11a7e01cdeda25f91b7f2fcb5a

It seems that the original patch does not work properly in vlan 
accelerated environment, which I reported 31 Mar 2006
  http://marc.theaimsgroup.com/?l=bonding-devel&m=114381240718113&w=2

Anyway, I didn't test this patch yet but I'm going to di it ASAP.

Best regards,

 					Krzysztof Olędzki

Re: PATCH Fix bonding active-backup behavior for VLAN interfaces

[Krzysztof Oledzki]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 929 bytes --]



On Thu, 3 Aug 2006, Krzysztof Oledzki wrote:

>
>
> On Wed, 2 Aug 2006, David Miller wrote:
> <CUT>
>> Finally, I'm still a little stumped about why this change is necessary
>> still, to be honest.
>
> If I understand it correctly this patch fixes the "[PATCH] bonding: suppress 
> duplicate packets" patch:
>
> http://www.kernel.org/git/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commitdiff;h=8f903c708fcc2b579ebf16542bf6109bad593a1d;hp=ebe19a4ed78d4a11a7e01cdeda25f91b7f2fcb5a
>
> It seems that the original patch does not work properly in vlan accelerated 
> environment, which I reported 31 Mar 2006
> http://marc.theaimsgroup.com/?l=bonding-devel&m=114381240718113&w=2
>
> Anyway, I didn't test this patch yet but I'm going to di it ASAP.

OK, this patch really solves the bug from my report. Are there any chances 
for similar fix in the net-2.6.19.git?

Best regards,

 				Krzysztof Olędzki

Re: PATCH Fix bonding active-backup behavior for VLAN interfaces

[David Miller]

From: Krzysztof Oledzki <olel@ans.pl>
Date: Thu, 10 Aug 2006 20:18:23 +0200 (CEST)

> OK, this patch really solves the bug from my report. Are there any chances 
> for similar fix in the net-2.6.19.git?

I'm still thinking about this patch and what various people have
explained about the situation.

Re: PATCH Fix bonding active-backup behavior for VLAN interfaces

[Christophe Devriese]

On Friday 11 August 2006 08:45, David Miller wrote:
> From: Krzysztof Oledzki <olel@ans.pl>
> Date: Thu, 10 Aug 2006 20:18:23 +0200 (CEST)
>
> > OK, this patch really solves the bug from my report. Are there any
> > chances for similar fix in the net-2.6.19.git?
>
> I'm still thinking about this patch and what various people have
> explained about the situation.

What can I do to get you to apply this then ? This patch is about fixing a bug 
which is bothering me a lot.

Regards,

Christophe Devriese

Re: PATCH Fix bonding active-backup behavior for VLAN interfaces

[David Miller]

From: Christophe Devriese <Christophe.Devriese@eurid.eu>
Date: Fri, 11 Aug 2006 10:50:44 +0200

> What can I do to get you to apply this then ? This patch is about
> fixing a bug which is bothering me a lot.

You need to be patient while I review the problem.

Nothing you say will allow my brain to operate any faster, sorry to
say.

Re: PATCH Fix bonding active-backup behavior for VLAN interfaces

[Christophe Devriese]

On Friday 11 August 2006 08:45, you wrote:
> From: Krzysztof Oledzki <olel@ans.pl>
> Date: Thu, 10 Aug 2006 20:18:23 +0200 (CEST)
>
> > OK, this patch really solves the bug from my report. Are there any
> > chances for similar fix in the net-2.6.19.git?
>
> I'm still thinking about this patch and what various people have
> explained about the situation.

Anything I can do ? I need this bug fixed.

Regards,

Christophe

Re: PATCH Fix bonding active-backup behavior for VLAN interfaces

[David Miller]

From: Christophe Devriese <Christophe.Devriese@eurid.eu>
Date: Mon, 14 Aug 2006 10:16:36 +0200

> On Friday 11 August 2006 08:45, you wrote:
> > From: Krzysztof Oledzki <olel@ans.pl>
> > Date: Thu, 10 Aug 2006 20:18:23 +0200 (CEST)
> >
> > > OK, this patch really solves the bug from my report. Are there any
> > > chances for similar fix in the net-2.6.19.git?
> >
> > I'm still thinking about this patch and what various people have
> > explained about the situation.
> 
> Anything I can do ? I need this bug fixed.

Then patch your kernel, you don't need my permission for that.

Re: PATCH Fix bonding active-backup behavior for VLAN interfaces

[David Miller]

BTW, I'll be honest with you, by continuing to bug me about this, it
makes me want to look at this issue less, not more.

Just be patient ok?

Re: PATCH Fix bonding active-backup behavior for VLAN interfaces

[Christophe Devriese]

On Wednesday 02 August 2006 22:58, you wrote:
> From: Christophe Devriese <Christophe.Devriese@eurid.eu>
> Date: Mon, 31 Jul 2006 10:15:40 +0200
>
> Thanks for the detailed explanation.
>
> > If you bond 2 vlan subinterfaces, the patch is not necessary at all. In
> > that case also the source device will be changed from eth0.<vlan> to
> > bond<x>. So that's correct behavior no ?
> >
> > In the second case, you create vlan subifs on a bonding device, vlan
> > subinterfaces will be created on the slave interfaces. In that case the
> > vlan code will reassign the skb->dev node, and because skb_bond needs to
> > know the actual input device in order to make an informed drop decision
> > before passing this code (skb active-backup mode needs to drop packets
> > from the backup slave interface, if you don't do that you get big
> > problems with broadcasts).
> >
> > The same struct vlan_group is assigned to all slave devices and so the
> > only vlan subinterfaces that exist in this case are the bond<x>.<vlan>
> > subinterfaces, and the vlan path for both slaves will assign the
> > bond<x>.<vlan> interface to skb->dev, thereby erasing the information
> > about where the packet came from.
>
> Assuming it is correct to do the skb_bond() here in the VLAN hwaccel
> RX path, then there is still one piece missing from what I can see.

The problem is that the vlan access path changes the skb->dev pointer around, 
after which it can no longer be determined what the source device was. (It 
defineately is not the parent of the vlan subinterface in the bond case)

> Notice that in the netif_receive_skb() path, the return value from
> skb_bond() is used as the third argument to the deliver_skb() routine
> and friends which in turn gets passed to the packet_type functions.
>
> Bonding, in particular, makes use of this third argument, see:
>
> bond_3ad_lacpdu_recv()
> rlb_arp_recv()
>
> So if the new "orig_dev" you are computing in the VLAN hwaccel RX path
> is the correct one, somehow this has to propagate down to the third
> argument of the packet type ->func() invocations, right?
>
> Finally, I'm still a little stumped about why this change is necessary
> still, to be honest.  When you configure the bond, the slaves should
> be the VLAN devices as far as I can tell.  Therefore it should be the
> "vlan_device->masster" that we are interested in not the top-level
> "dev->master".

Indeed but vlan_device->master is "bond0" instead of eth0 or eth1 (assuming 
you bond eth0 and eth1) so you cannot determine where the packet came from.

> If the ethernet is on a VLAN, and the administrator configures the
> underlying ethernet device as the slaves of the bond, this to me seems
> like a misconfiguration rather than something we should put hacks in
> to support.

In this configuration you have lot's more interfaces to administer and keep 
track of. Besides, what happened to "there's more than one way to do it ?". 
If you configure vlan subifs on a bonding device the correct behavior would 
be that the bonding logic is applied _first_ and then after the vlan gets 
considered, no ?

> The fact that you do not propagate the "orig_dev" returned from
> skb_bond() down to the packet type functions seems to support this.
> From my perspective, this looks like a hack for a bonding
> misconfiguration.
>
> -
> 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

Re: PATCH Fix bonding active-backup behavior for VLAN interfaces

[Jay Vosburgh]

David Miller <davem@davemloft.net> wrote:

>> The same struct vlan_group is assigned to all slave devices and so the only 
>> vlan subinterfaces that exist in this case are the bond<x>.<vlan> 
>> subinterfaces, and the vlan path for both slaves will assign the 
>> bond<x>.<vlan> interface to skb->dev, thereby erasing the information about 
>> where the packet came from.
>
>Assuming it is correct to do the skb_bond() here in the VLAN hwaccel
>RX path, then there is still one piece missing from what I can see.

	I'm not sure it's correct to do everything that skb_bond() does
(I've been away on family business, and I'm a bit rusty coming back to
this), but it is correct to do the "should we drop this packet because
it's a duplicate because bonding is involved" logic (because the VLAN
acceleration processing is indavertently bypassing that step).

>Notice that in the netif_receive_skb() path, the return value from
>skb_bond() is used as the third argument to the deliver_skb() routine
>and friends which in turn gets passed to the packet_type functions.
>
>Bonding, in particular, makes use of this third argument, see:
>
>bond_3ad_lacpdu_recv()

	This function expects the third argument to be the the device
that is actually enslaved to the bond and is acting as the endpoint for
the 802.3ad protocol negotiations.  If it's a VLAN interface under
there, that's what it needs.

>rlb_arp_recv()

	This function doesn't actually use the third argument that I'm
seeing.

>So if the new "orig_dev" you are computing in the VLAN hwaccel RX path
>is the correct one, somehow this has to propagate down to the third
>argument of the packet type ->func() invocations, right?
>
>Finally, I'm still a little stumped about why this change is necessary
>still, to be honest.  When you configure the bond, the slaves should
>be the VLAN devices as far as I can tell.  Therefore it should be the
>"vlan_device->masster" that we are interested in not the top-level
>"dev->master".
>
>If the ethernet is on a VLAN, and the administrator configures the
>underlying ethernet device as the slaves of the bond, this to me seems
>like a misconfiguration rather than something we should put hacks in
>to support.

	Not necessarily.  For a configuration with redundant paths
(either multiple ports or multiple switches) or multiple VLANs over the
same physical ports, it's also plausible to configure the vlan
interfaces above bonding, and make the ethernet devices direct slaves of
bonding.  In that configuration, the vlan interfaces are on top,
followed by the bonding device, and the ethernet devices at the bottom.

	In this case (bond0.555 above bond0 above eth0,eth1,etc),
skb_bond doesn't suppress duplicates because skb_bond is called with the
skb->dev set to the bond0.555 dev, not the ethX dev.  Non-accelerated
VLAN devices don't do this; they'll come in with skb->dev set to ethX
and will go through skb_bond as expected.

	-J

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

Re: PATCH Fix bonding active-backup behavior for VLAN interfaces

[David Miller]

From: Jay Vosburgh <fubar@us.ibm.com>
Date: Thu, 03 Aug 2006 18:01:35 -0700

> 	In this case (bond0.555 above bond0 above eth0,eth1,etc),
> skb_bond doesn't suppress duplicates because skb_bond is called with the
> skb->dev set to the bond0.555 dev, not the ethX dev.  Non-accelerated
> VLAN devices don't do this; they'll come in with skb->dev set to ethX
> and will go through skb_bond as expected.

Ok, since __vlan_hwaccel_rx() bypasses the netif_receive_skb()
that would normally occur, we have to duplicate the bonding
drop checks.

The submitted patch put skb_bond() into if_vlan.h which is
definitely the wrong thing to do.  This is a generic operation
and therefore belongs in linux/netdevice.h at best.

Furthermore, we're only interested in the packet drop check,
so that's the only part of the logic we need to export,
the rest can stay private to skb_bond() in net/core/dev.c

Can the folks who can reproduce this try this patch?

Thanks.

commit 8a9583e08d0524e3bfe43a51af7c66ca21adf9f3
Author: David S. Miller <davem@sunset.davemloft.net>
Date:   Mon Aug 14 17:08:36 2006 -0700

    [VLAN]: Make sure bonding packet drop checks get done in hwaccel RX path.
    
    Since __vlan_hwaccel_rx() is essentially bypassing the
    netif_receive_skb() call that would have occurred if we did the VLAN
    decapsulation in software, we are missing the skb_bond() call and the
    assosciated checks it does.
    
    Export those checks via an inline function, skb_bond_should_drop(),
    and use this in __vlan_hwaccel_rx().
    
    Signed-off-by: David S. Miller <davem@davemloft.net>

diff --git a/include/linux/if_vlan.h b/include/linux/if_vlan.h
index 383627a..ab27408 100644
--- a/include/linux/if_vlan.h
+++ b/include/linux/if_vlan.h
@@ -155,6 +155,11 @@ static inline int __vlan_hwaccel_rx(stru
 {
 	struct net_device_stats *stats;
 
+	if (skb_bond_should_drop(skb)) {
+		dev_kfree_skb_any(skb);
+		return NET_RX_DROP;
+	}
+
 	skb->dev = grp->vlan_devices[vlan_tag & VLAN_VID_MASK];
 	if (skb->dev == NULL) {
 		dev_kfree_skb_any(skb);
diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
index 75f02d8..c0c2b46 100644
--- a/include/linux/netdevice.h
+++ b/include/linux/netdevice.h
@@ -1012,6 +1012,30 @@ static inline int netif_needs_gso(struct
 		unlikely(skb->ip_summed != CHECKSUM_HW));
 }
 
+/* On bonding slaves other than the currently active slave, suppress
+ * duplicates except for 802.3ad ETH_P_SLOW and alb non-mcast/bcast.
+ */
+static inline int skb_bond_should_drop(struct sk_buff *skb)
+{
+	struct net_device *dev = skb->dev;
+	struct net_device *master = dev->master;
+
+	if (master &&
+	    (dev->priv_flags & IFF_SLAVE_INACTIVE)) {
+		if (master->priv_flags & IFF_MASTER_ALB) {
+			if (skb->pkt_type != PACKET_BROADCAST &&
+			    skb->pkt_type != PACKET_MULTICAST)
+				return 0;
+		}
+		if (master->priv_flags & IFF_MASTER_8023AD &&
+		    skb->protocol == __constant_htons(ETH_P_SLOW))
+			return 0;
+
+		return 1;
+	}
+	return 0;
+}
+
 #endif /* __KERNEL__ */
 
 #endif	/* _LINUX_DEV_H */
diff --git a/net/core/dev.c b/net/core/dev.c
index d95e262..9fe96cd 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -1619,26 +1619,10 @@ static inline struct net_device *skb_bon
 	struct net_device *dev = skb->dev;
 
 	if (dev->master) {
-		/*
-		 * On bonding slaves other than the currently active
-		 * slave, suppress duplicates except for 802.3ad
-		 * ETH_P_SLOW and alb non-mcast/bcast.
-		 */
-		if (dev->priv_flags & IFF_SLAVE_INACTIVE) {
-			if (dev->master->priv_flags & IFF_MASTER_ALB) {
-				if (skb->pkt_type != PACKET_BROADCAST &&
-				    skb->pkt_type != PACKET_MULTICAST)
-					goto keep;
-			}
-
-			if (dev->master->priv_flags & IFF_MASTER_8023AD &&
-			    skb->protocol == __constant_htons(ETH_P_SLOW))
-				goto keep;
-		
+		if (skb_bond_should_drop(skb)) {
 			kfree_skb(skb);
 			return NULL;
 		}
-keep:
 		skb->dev = dev->master;
 	}
 

Re: PATCH Fix bonding active-backup behavior for VLAN interfaces

[Jay Vosburgh]

David Miller <davem@davemloft.net> wrote:
[...]
>Ok, since __vlan_hwaccel_rx() bypasses the netif_receive_skb()
>that would normally occur, we have to duplicate the bonding
>drop checks.
>
>The submitted patch put skb_bond() into if_vlan.h which is
>definitely the wrong thing to do.  This is a generic operation
>and therefore belongs in linux/netdevice.h at best.
>
>Furthermore, we're only interested in the packet drop check,
>so that's the only part of the logic we need to export,
>the rest can stay private to skb_bond() in net/core/dev.c
>
>Can the folks who can reproduce this try this patch?

	I have successfully reproduced the problem and subsequently
validated this patch against 2.6.17.6.  I'm building netdev-2.6#upstream
right now, but I expect it will work as well (and will report back only
if it does not).

Acked-by: Jay Vosburgh <fubar@us.ibm.com>

	-J

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

Re: PATCH Fix bonding active-backup behavior for VLAN interfaces

[David Miller]

From: Jay Vosburgh <fubar@us.ibm.com>
Date: Tue, 15 Aug 2006 15:18:13 -0700

> 	I have successfully reproduced the problem and subsequently
> validated this patch against 2.6.17.6.  I'm building netdev-2.6#upstream
> right now, but I expect it will work as well (and will report back only
> if it does not).
> 
> Acked-by: Jay Vosburgh <fubar@us.ibm.com>

Thanks a lot for testing Jay.

Re: PATCH Fix bonding active-backup behavior for VLAN interfaces

[Krzysztof Oledzki]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 1166 bytes --]



On Mon, 14 Aug 2006, David Miller wrote:

> From: Jay Vosburgh <fubar@us.ibm.com>
> Date: Thu, 03 Aug 2006 18:01:35 -0700
>
>> 	In this case (bond0.555 above bond0 above eth0,eth1,etc),
>> skb_bond doesn't suppress duplicates because skb_bond is called with the
>> skb->dev set to the bond0.555 dev, not the ethX dev.  Non-accelerated
>> VLAN devices don't do this; they'll come in with skb->dev set to ethX
>> and will go through skb_bond as expected.
>
> Ok, since __vlan_hwaccel_rx() bypasses the netif_receive_skb()
> that would normally occur, we have to duplicate the bonding
> drop checks.
>
> The submitted patch put skb_bond() into if_vlan.h which is
> definitely the wrong thing to do.  This is a generic operation
> and therefore belongs in linux/netdevice.h at best.
>
> Furthermore, we're only interested in the packet drop check,
> so that's the only part of the logic we need to export,
> the rest can stay private to skb_bond() in net/core/dev.c
>
> Can the folks who can reproduce this try this patch?

Works for me, thank you.

Acked-by: Krzysztof Piotr Oledzki <ole@ans.pl>

Best regards,

 				Krzysztof Olędzki

Re: PATCH Fix bonding active-backup behavior for VLAN interfaces

[Christophe Devriese]

This fixes my issue. Thanks.

On Tuesday 15 August 2006 02:09, you wrote:
> From: Jay Vosburgh <fubar@us.ibm.com>
> Date: Thu, 03 Aug 2006 18:01:35 -0700
>
> > 	In this case (bond0.555 above bond0 above eth0,eth1,etc),
> > skb_bond doesn't suppress duplicates because skb_bond is called with the
> > skb->dev set to the bond0.555 dev, not the ethX dev.  Non-accelerated
> > VLAN devices don't do this; they'll come in with skb->dev set to ethX
> > and will go through skb_bond as expected.
>
> Ok, since __vlan_hwaccel_rx() bypasses the netif_receive_skb()
> that would normally occur, we have to duplicate the bonding
> drop checks.
>
> The submitted patch put skb_bond() into if_vlan.h which is
> definitely the wrong thing to do.  This is a generic operation
> and therefore belongs in linux/netdevice.h at best.
>
> Furthermore, we're only interested in the packet drop check,
> so that's the only part of the logic we need to export,
> the rest can stay private to skb_bond() in net/core/dev.c
>
> Can the folks who can reproduce this try this patch?
>
> Thanks.
>
> commit 8a9583e08d0524e3bfe43a51af7c66ca21adf9f3
> Author: David S. Miller <davem@sunset.davemloft.net>
> Date:   Mon Aug 14 17:08:36 2006 -0700
>
>     [VLAN]: Make sure bonding packet drop checks get done in hwaccel RX
> path.
>
>     Since __vlan_hwaccel_rx() is essentially bypassing the
>     netif_receive_skb() call that would have occurred if we did the VLAN
>     decapsulation in software, we are missing the skb_bond() call and the
>     assosciated checks it does.
>
>     Export those checks via an inline function, skb_bond_should_drop(),
>     and use this in __vlan_hwaccel_rx().
>
>     Signed-off-by: David S. Miller <davem@davemloft.net>
>
> diff --git a/include/linux/if_vlan.h b/include/linux/if_vlan.h
> index 383627a..ab27408 100644
> --- a/include/linux/if_vlan.h
> +++ b/include/linux/if_vlan.h
> @@ -155,6 +155,11 @@ static inline int __vlan_hwaccel_rx(stru
>  {
>  	struct net_device_stats *stats;
>
> +	if (skb_bond_should_drop(skb)) {
> +		dev_kfree_skb_any(skb);
> +		return NET_RX_DROP;
> +	}
> +
>  	skb->dev = grp->vlan_devices[vlan_tag & VLAN_VID_MASK];
>  	if (skb->dev == NULL) {
>  		dev_kfree_skb_any(skb);
> diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
> index 75f02d8..c0c2b46 100644
> --- a/include/linux/netdevice.h
> +++ b/include/linux/netdevice.h
> @@ -1012,6 +1012,30 @@ static inline int netif_needs_gso(struct
>  		unlikely(skb->ip_summed != CHECKSUM_HW));
>  }
>
> +/* On bonding slaves other than the currently active slave, suppress
> + * duplicates except for 802.3ad ETH_P_SLOW and alb non-mcast/bcast.
> + */
> +static inline int skb_bond_should_drop(struct sk_buff *skb)
> +{
> +	struct net_device *dev = skb->dev;
> +	struct net_device *master = dev->master;
> +
> +	if (master &&
> +	    (dev->priv_flags & IFF_SLAVE_INACTIVE)) {
> +		if (master->priv_flags & IFF_MASTER_ALB) {
> +			if (skb->pkt_type != PACKET_BROADCAST &&
> +			    skb->pkt_type != PACKET_MULTICAST)
> +				return 0;
> +		}
> +		if (master->priv_flags & IFF_MASTER_8023AD &&
> +		    skb->protocol == __constant_htons(ETH_P_SLOW))
> +			return 0;
> +
> +		return 1;
> +	}
> +	return 0;
> +}
> +
>  #endif /* __KERNEL__ */
>
>  #endif	/* _LINUX_DEV_H */
> diff --git a/net/core/dev.c b/net/core/dev.c
> index d95e262..9fe96cd 100644
> --- a/net/core/dev.c
> +++ b/net/core/dev.c
> @@ -1619,26 +1619,10 @@ static inline struct net_device *skb_bon
>  	struct net_device *dev = skb->dev;
>
>  	if (dev->master) {
> -		/*
> -		 * On bonding slaves other than the currently active
> -		 * slave, suppress duplicates except for 802.3ad
> -		 * ETH_P_SLOW and alb non-mcast/bcast.
> -		 */
> -		if (dev->priv_flags & IFF_SLAVE_INACTIVE) {
> -			if (dev->master->priv_flags & IFF_MASTER_ALB) {
> -				if (skb->pkt_type != PACKET_BROADCAST &&
> -				    skb->pkt_type != PACKET_MULTICAST)
> -					goto keep;
> -			}
> -
> -			if (dev->master->priv_flags & IFF_MASTER_8023AD &&
> -			    skb->protocol == __constant_htons(ETH_P_SLOW))
> -				goto keep;
> -
> +		if (skb_bond_should_drop(skb)) {
>  			kfree_skb(skb);
>  			return NULL;
>  		}
> -keep:
>  		skb->dev = dev->master;
>  	}