| Message ID | 10757.1334772163@death.nxdomain |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | David Miller |
| Headers | show |
On Wed, 2012-04-18 at 11:02 -0700, Jay Vosburgh wrote: > With bonding's fail_over_mac=active, during failover the MAC > address of the bond itself changes to match that of the slave. > > This patch adds a notifier call to cause VLANs stacked atop the > bonding to also change their MAC addresses to the new address when a > failover occurs. > > While it is legal for a VLAN to have a MAC address that differs > from the underlying device, at least one device (qeth) that requires the > use of fail_over_mac for bonding cannot handle the VLAN's MAC differing > from that of the bond; thus, it needs the MAC change to propagate up > to any VLANs when fail_over_mac is set to active. [...] This doesn't make sense to me. You're applying the behaviour to all VLANs on top of a bond, whether or not the underlying device is driven by qeth, and ignoring any MAC address changes that don't involve the bonding driver. I think either of these would be better fixes: 1. Make VLAN devices follow changes to the parent device's MAC address unless they are assigned an address of their own. 2. Add a configuration flag for VLAN devices to follow changes to the parent device's MAC address. Ben.
Ben Hutchings <bhutchings@solarflare.com> wrote: >On Wed, 2012-04-18 at 11:02 -0700, Jay Vosburgh wrote: >> With bonding's fail_over_mac=active, during failover the MAC >> address of the bond itself changes to match that of the slave. >> >> This patch adds a notifier call to cause VLANs stacked atop the >> bonding to also change their MAC addresses to the new address when a >> failover occurs. >> >> While it is legal for a VLAN to have a MAC address that differs >> from the underlying device, at least one device (qeth) that requires the >> use of fail_over_mac for bonding cannot handle the VLAN's MAC differing >> from that of the bond; thus, it needs the MAC change to propagate up >> to any VLANs when fail_over_mac is set to active. >[...] > >This doesn't make sense to me. You're applying the behaviour to all >VLANs on top of a bond, whether or not the underlying device is driven >by qeth, and ignoring any MAC address changes that don't involve the >bonding driver. With the patch, the PROPAGATE event is only generated if bonding is set for fail_over_mac=active, which is normally only enabled on those devices that require it (some devices for IBM's pseries and zseries architectures and Infiniband, which doesn't have VLANs). Devices that do not use bonding's fail_over_mac will not have VLANs following MAC changes. >I think either of these would be better fixes: >1. Make VLAN devices follow changes to the parent device's MAC address >unless they are assigned an address of their own. >2. Add a configuration flag for VLAN devices to follow changes to the >parent device's MAC address. #1 would be a behavior change for all VLAN devices, which I sought to avoid. #2 would be an additional configuration option that would have to be enabled just for this case (unless VLANs following MAC changes of the parent device is a generally desirable feature). The patch requires no additional option settings beyond what are currently in use. -J --- -Jay Vosburgh, IBM Linux Technology Center, fubar@us.ibm.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
On Wed, 2012-04-18 at 11:49 -0700, Jay Vosburgh wrote: > Ben Hutchings <bhutchings@solarflare.com> wrote: > > >On Wed, 2012-04-18 at 11:02 -0700, Jay Vosburgh wrote: > >> With bonding's fail_over_mac=active, during failover the MAC > >> address of the bond itself changes to match that of the slave. > >> > >> This patch adds a notifier call to cause VLANs stacked atop the > >> bonding to also change their MAC addresses to the new address when a > >> failover occurs. > >> > >> While it is legal for a VLAN to have a MAC address that differs > >> from the underlying device, at least one device (qeth) that requires the > >> use of fail_over_mac for bonding cannot handle the VLAN's MAC differing > >> from that of the bond; thus, it needs the MAC change to propagate up > >> to any VLANs when fail_over_mac is set to active. > >[...] > > > >This doesn't make sense to me. You're applying the behaviour to all > >VLANs on top of a bond, whether or not the underlying device is driven > >by qeth, and ignoring any MAC address changes that don't involve the > >bonding driver. > > With the patch, the PROPAGATE event is only generated if bonding > is set for fail_over_mac=active, which is normally only enabled on those > devices that require it (some devices for IBM's pseries and zseries > architectures and Infiniband, which doesn't have VLANs). Yeah, OK, that makes sense. > Devices that do not use bonding's fail_over_mac will not have > VLANs following MAC changes. I take it that the devices with this limitation on source MAC address have an essentially unchangeable MAC address? If they are limited to single address but it's changeable then they should be emitting this notification too. > >I think either of these would be better fixes: > >1. Make VLAN devices follow changes to the parent device's MAC address > >unless they are assigned an address of their own. > >2. Add a configuration flag for VLAN devices to follow changes to the > >parent device's MAC address. > > #1 would be a behavior change for all VLAN devices, which I > sought to avoid. > > #2 would be an additional configuration option that would have > to be enabled just for this case (unless VLANs following MAC changes of > the parent device is a generally desirable feature). I don't know whether it is generally desirable. My guess would be that unless a VLAN device is explicitly configured to use its own address then it is desirable. > The patch requires > no additional option settings beyond what are currently in use. Right, I understand that this ought to Just Work, if possible. Ben.
Please do not apply this patch; we've found an alternate solution that doesn't require this change. Ben Hutchings <bhutchings@solarflare.com> wrote: >On Wed, 2012-04-18 at 11:49 -0700, Jay Vosburgh wrote: >> Ben Hutchings <bhutchings@solarflare.com> wrote: >> >> >On Wed, 2012-04-18 at 11:02 -0700, Jay Vosburgh wrote: >> >> With bonding's fail_over_mac=active, during failover the MAC >> >> address of the bond itself changes to match that of the slave. >> >> >> >> This patch adds a notifier call to cause VLANs stacked atop the >> >> bonding to also change their MAC addresses to the new address when a >> >> failover occurs. >> >> >> >> While it is legal for a VLAN to have a MAC address that differs >> >> from the underlying device, at least one device (qeth) that requires the >> >> use of fail_over_mac for bonding cannot handle the VLAN's MAC differing >> >> from that of the bond; thus, it needs the MAC change to propagate up >> >> to any VLANs when fail_over_mac is set to active. >> >[...] >> > >> >This doesn't make sense to me. You're applying the behaviour to all >> >VLANs on top of a bond, whether or not the underlying device is driven >> >by qeth, and ignoring any MAC address changes that don't involve the >> >bonding driver. >> >> With the patch, the PROPAGATE event is only generated if bonding >> is set for fail_over_mac=active, which is normally only enabled on those >> devices that require it (some devices for IBM's pseries and zseries >> architectures and Infiniband, which doesn't have VLANs). > >Yeah, OK, that makes sense. > >> Devices that do not use bonding's fail_over_mac will not have >> VLANs following MAC changes. > >I take it that the devices with this limitation on source MAC address >have an essentially unchangeable MAC address? If they are limited to >single address but it's changeable then they should be emitting this >notification too. It's not that it can't change the MAC, the issue has to do with the packet forwarding logic on the actual device that services the various virtual devices configured on the LPARs. This being s390, it's not quite that simple, but that's the short version. >> >I think either of these would be better fixes: >> >1. Make VLAN devices follow changes to the parent device's MAC address >> >unless they are assigned an address of their own. >> >2. Add a configuration flag for VLAN devices to follow changes to the >> >parent device's MAC address. >> >> #1 would be a behavior change for all VLAN devices, which I >> sought to avoid. >> >> #2 would be an additional configuration option that would have >> to be enabled just for this case (unless VLANs following MAC changes of >> the parent device is a generally desirable feature). > >I don't know whether it is generally desirable. My guess would be that >unless a VLAN device is explicitly configured to use its own address >then it is desirable. > >> The patch requires >> no additional option settings beyond what are currently in use. > >Right, I understand that this ought to Just Work, if possible. > >Ben. -J --- -Jay Vosburgh, IBM Linux Technology Center, fubar@us.ibm.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
diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c index 44e6a64..7a92e35 100644 --- a/drivers/net/bonding/bond_main.c +++ b/drivers/net/bonding/bond_main.c @@ -896,6 +896,8 @@ static void bond_do_fail_over_mac(struct bonding *bond, new_active->dev->addr_len); write_unlock_bh(&bond->curr_slave_lock); read_unlock(&bond->lock); + call_netdevice_notifiers(NETDEV_PROPAGATE_ADDR, + bond->dev); call_netdevice_notifiers(NETDEV_CHANGEADDR, bond->dev); read_lock(&bond->lock); write_lock_bh(&bond->curr_slave_lock); diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h index e0b70e9..2fe9697 100644 --- a/include/linux/netdevice.h +++ b/include/linux/netdevice.h @@ -1559,6 +1559,7 @@ struct packet_type { #define NETDEV_RELEASE 0x0012 #define NETDEV_NOTIFY_PEERS 0x0013 #define NETDEV_JOIN 0x0014 +#define NETDEV_PROPAGATE_ADDR 0x0015 extern int register_netdevice_notifier(struct notifier_block *nb); extern int unregister_netdevice_notifier(struct notifier_block *nb); diff --git a/net/8021q/vlan.c b/net/8021q/vlan.c index efea35b..29da25f 100644 --- a/net/8021q/vlan.c +++ b/net/8021q/vlan.c @@ -303,6 +303,8 @@ static void vlan_transfer_features(struct net_device *dev, static void __vlan_device_event(struct net_device *dev, unsigned long event) { + struct net_device *realdev; + switch (event) { case NETDEV_CHANGENAME: vlan_proc_rem_dev(dev); @@ -317,6 +319,10 @@ static void __vlan_device_event(struct net_device *dev, unsigned long event) case NETDEV_UNREGISTER: vlan_proc_rem_dev(dev); break; + case NETDEV_PROPAGATE_ADDR: + realdev = vlan_dev_priv(dev)->real_dev; + memcpy(dev->dev_addr, realdev->dev_addr, ETH_ALEN); + break; } } @@ -464,6 +470,7 @@ static int vlan_device_event(struct notifier_block *unused, unsigned long event, case NETDEV_NOTIFY_PEERS: case NETDEV_BONDING_FAILOVER: + case NETDEV_PROPAGATE_ADDR: /* Propagate to vlan devices */ for (i = 0; i < VLAN_N_VID; i++) { vlandev = vlan_group_get_device(grp, i);
With bonding's fail_over_mac=active, during failover the MAC address of the bond itself changes to match that of the slave. This patch adds a notifier call to cause VLANs stacked atop the bonding to also change their MAC addresses to the new address when a failover occurs. While it is legal for a VLAN to have a MAC address that differs from the underlying device, at least one device (qeth) that requires the use of fail_over_mac for bonding cannot handle the VLAN's MAC differing from that of the bond; thus, it needs the MAC change to propagate up to any VLANs when fail_over_mac is set to active. Signed-off-by: Jay Vosburgh <fubar@us.ibm.com> --- drivers/net/bonding/bond_main.c | 2 ++ include/linux/netdevice.h | 1 + net/8021q/vlan.c | 7 +++++++ 3 files changed, 10 insertions(+), 0 deletions(-)