Message-ID: <22549.1307744758@death>
Date:	Fri, 10 Jun 2011 15:25:58 -0700
From:	Jay Vosburgh <fubar@...ibm.com>
To:	Jiri Bohac <jbohac@...e.cz>
cc:	Andy Gospodarek <andy@...yhouse.net>,
	"David S. Miller" <davem@...emloft.net>, netdev@...r.kernel.org,
	Pedro Garcia <pedro.netdev@...devamos.com>,
	Patrick McHardy <kaber@...sh.net>, mirq-linux@...e.qmqm.pl
Subject: Re: [PATCH 2/2] bonding: restore NETIF_F_VLAN_CHALLENGED properly in bond_del_vlan()

Jiri Bohac <jbohac@...e.cz> wrote:

>Hi,
>
>On Fri, Jun 03, 2011 at 05:26:51PM -0700, Jay Vosburgh wrote:
>> Jiri Bohac <jbohac@...e.cz> wrote:
>> 
>> >Since commit ad1afb00, bond_del_vlan() never restores
>> >NETIF_F_VLAN_CHALLENGED as intended.  bond->vlan_list is never
>> >empty once the 8021q module is loaded, because the special VLAN 0
>> >is always kept registered on the bond interface. Change the
>> >condition to check if bond->vlan_list contains exactly one item
>> >instead of checking for an empty list.
>> >
>> >Signed-off-by: Jiri Bohac <jbohac@...e.cz>
>> >
>> >diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
>> >index 17b4dd9..4d317cd 100644
>> >--- a/drivers/net/bonding/bond_main.c
>> >+++ b/drivers/net/bonding/bond_main.c
>> >@@ -329,9 +329,10 @@ static int bond_del_vlan(struct bonding *bond, unsigned short vlan_id)
>> >
>> > 			kfree(vlan);
>> >
>> >-			if (list_empty(&bond->vlan_list) &&
>> >+			if (bond->vlan_list.next->next == &bond->vlan_list &&
>> > 			    (bond->slave_cnt == 0)) {
>> >-				/* Last VLAN removed and no slaves, so
>> >+				/* Last VLAN removed (the only member of vlan_list
>> >+				 * is the special vid == 0 vlan) and no slaves, so
>> > 				 * restore block on adding VLANs. This will
>> > 				 * be removed once new slaves that are not
>> > 				 * VLAN challenged will be added.
>> 
>> 	Could we do this instead in bond_release, when the last slave is
>> removed?  The CHALLENGED flag just prevents adding new VLANs; existing
>> ones would persist until a new slave was added.
>
>Actually, this has already happenned with commit b2a103e6. Sorry
>I originally checked with an older kernel. This makes the above
>patch obsolete and calls for a little cleanup (below).
>
>b2a103e6 has changed the behaviour a little. Before, if you had a
>vlan configured on a bond and removed the last slave, the
>CHALLENGED flag would stay off, until you deleted the vlan.  Now,
>CHALLENGED is turned on as soon as the last slave is deleted.
>
>BTW, this can lead to a sort of inconsistent state, where CHALLENGED
>is on and the bond has vlans.
>
>> Since CHALLENGED slaves are in the minority these days (looks
>> like just IPoIB, one wimax and one obscure ethernet chipset) we
>> could even invert the logic: only assert CHALLENGED for the
>> master when such a slave is added.
>
>This sounds good. I don't understand why bonding currently tries
>to prevent VLANs on an slave-less bond, and I might not be the
>only one. Citing a comment in bond_fix_features():
>
>	/* Disable adding VLANs to empty bond. But why? --mq */
>
>Another comment, in bond_setup():
>
>        /* At first, we block adding VLANs. That's the only way to
>         * prevent problems that occur when adding VLANs over an
>         * empty bond. The block will be removed once non-challenged
>         * slaves are enslaved.
>         */
>
>Do we need to prevent this at all? What are the potential
>problems?

	The problems have to do with the VLAN pseudo-device not getting
a MAC address if the bond master's MAC is all zeros when the VLAN is
added.

	The VLAN pseudo-device copies the underlying device's MAC at the
time the VLAN is added (in vlan_dev_init via register_netdevice), and
doesn't update it later if the underlying device's MAC address changes.
So we assert CHALLENGED until the bonding master has a MAC address (when
the first slave is added), to guarantee that there's a valid MAC in the
bond's dev_addr when the VLAN is added.

	If we relax this restriction (only enable CHALLENGED if an
actual CHALLENGED slave is added), then we also must add a CHANGEADDR
notifier when the bonding master picks up its MAC from the first slave
(which we do not currently do), and figure out how to get that initial
MAC assignment passed up to a VLAN device (or stack) atop bonding.
Currently, the vlan's event handler for CHANGEADDR, vlan_sync_address,
only tracks the underlying device's "real_dev_addr" but doesn't alter
the VLAN device's dev_addr.

	One obvious suggestion to resolve that is to have 8021q's
CHANGEADDR update the VLAN device's dev_addr to the underlying device's
dev_addr if the VLAN's is all zeroes.

>Anyway a little cleanup:
>
>
>bonding: clean up bond_del_vlan()
>
>1) the setting of NETIF_F_VLAN_CHALLENGED in bond_del_vlan() is
>useless since commit b2a103e6 because bond_fix_features() now
>sets NETIF_F_VLAN_CHALLENGED whenever the last slave is being
>removed.
>
>2) the code never triggers anyway as vlan_list is never empty
>since ad1afb00.
>
>Signed-off-by: Jiri Bohac <jbohac@...e.cz>

	Just to be clear, this patch is unrelated to the discussion
above about changing the CHALLENGED behavior of bonding, and is really
just dead code removal.

Signed-off-by: Jay Vosburgh <fubar@...ibm.com>

	-J

>diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
>--- a/drivers/net/bonding/bond_main.c
>+++ b/drivers/net/bonding/bond_main.c
>@@ -329,16 +329,6 @@ static int bond_del_vlan(struct bonding *bond, unsigned short vlan_id)
>
> 			kfree(vlan);
>
>-			if (list_empty(&bond->vlan_list) &&
>-			    (bond->slave_cnt == 0)) {
>-				/* Last VLAN removed and no slaves, so
>-				 * restore block on adding VLANs. This will
>-				 * be removed once new slaves that are not
>-				 * VLAN challenged will be added.
>-				 */
>-				bond->dev->features |= NETIF_F_VLAN_CHALLENGED;
>-			}
>-
> 			res = 0;
> 			goto out;
> 		}
>
>-- 

---
	-Jay Vosburgh, IBM Linux Technology Center, fubar@...ibm.com
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Powered by blists - more mailing lists