| Message ID | 1400764324-23221-3-git-send-email-vfalico@gmail.com |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | David Miller |
| Headers | show |
On Thu, May 22, 2014 at 4:12 PM, Veaceslav Falico <vfalico@gmail.com> wrote: > > Currently it's called only from bond_alb_set_mac_address(), which is called > only for ALB mode, and it does nothing in case the mode is ALB. So, > basically, it's a no-op. All the needed functionality (modifying the active > slave's mac address, per example) is handled by the > bond_alb_set_mac_address() itself. So with this patch @ hand, no more touching of the underlying device dev_addr field directly -- or we still have more accesses of this such, e.g in bond_alb_set_mac_address() and/or friends? Or. -- 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 Thu, May 22, 2014 at 05:36:19PM +0300, Or Gerlitz wrote: >On Thu, May 22, 2014 at 4:12 PM, Veaceslav Falico <vfalico@gmail.com> wrote: >> >> Currently it's called only from bond_alb_set_mac_address(), which is called >> only for ALB mode, and it does nothing in case the mode is ALB. So, >> basically, it's a no-op. All the needed functionality (modifying the active >> slave's mac address, per example) is handled by the >> bond_alb_set_mac_address() itself. > >So with this patch @ hand, no more touching of the underlying device >dev_addr field directly -- or we still have more accesses of this >such, e.g in bond_alb_set_mac_address() and/or friends? Sorry, but no :(. I'm still working on it, and already sent several patchsets approaching it :). Hopefully it will be ready next week. If you need it urgently - patches welcome :). > >Or. -- 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 05/22/2014 09:12 AM, Veaceslav Falico wrote: > Currently it's called only from bond_alb_set_mac_address(), which is called > only for ALB mode, and it does nothing in case the mode is ALB. So, > basically, it's a no-op. All the needed functionality (modifying the active > slave's mac address, per example) is handled by the > bond_alb_set_mac_address() itself. > > So remove it, as it's not needed. From the comments and code, it seems like it should be called for TLB mode as well as it does that weird half-setting of the address that TLB seems to want. Is that not needed any more? -vlad > > CC: Jay Vosburgh <j.vosburgh@gmail.com> > CC: Andy Gospodarek <andy@greyhouse.net> > Signed-off-by: Veaceslav Falico <vfalico@gmail.com> > --- > drivers/net/bonding/bond_alb.c | 61 ------------------------------------------ > 1 file changed, 61 deletions(-) > > diff --git a/drivers/net/bonding/bond_alb.c b/drivers/net/bonding/bond_alb.c > index 965518b..1d772b5 100644 > --- a/drivers/net/bonding/bond_alb.c > +++ b/drivers/net/bonding/bond_alb.c > @@ -1256,62 +1256,6 @@ static int alb_handle_addr_collision_on_attach(struct bonding *bond, struct slav > return 0; > } > > -/** > - * alb_set_mac_address > - * @bond: > - * @addr: > - * > - * In TLB mode all slaves are configured to the bond's hw address, but set > - * their dev_addr field to different addresses (based on their permanent hw > - * addresses). > - * > - * For each slave, this function sets the interface to the new address and then > - * changes its dev_addr field to its previous value. > - * > - * Unwinding assumes bond's mac address has not yet changed. > - */ > -static int alb_set_mac_address(struct bonding *bond, void *addr) > -{ > - struct slave *slave, *rollback_slave; > - struct list_head *iter; > - struct sockaddr sa; > - char tmp_addr[ETH_ALEN]; > - int res; > - > - if (BOND_MODE(bond) == BOND_MODE_ALB) > - return 0; > - > - bond_for_each_slave(bond, slave, iter) { > - /* save net_device's current hw address */ > - ether_addr_copy(tmp_addr, slave->dev->dev_addr); > - > - res = dev_set_mac_address(slave->dev, addr); > - > - /* restore net_device's hw address */ > - ether_addr_copy(slave->dev->dev_addr, tmp_addr); > - > - if (res) > - goto unwind; > - } > - > - return 0; > - > -unwind: > - memcpy(sa.sa_data, bond->dev->dev_addr, bond->dev->addr_len); > - sa.sa_family = bond->dev->type; > - > - /* unwind from head to the slave that failed */ > - bond_for_each_slave(bond, rollback_slave, iter) { > - if (rollback_slave == slave) > - break; > - ether_addr_copy(tmp_addr, rollback_slave->dev->dev_addr); > - dev_set_mac_address(rollback_slave->dev, &sa); > - ether_addr_copy(rollback_slave->dev->dev_addr, tmp_addr); > - } > - > - return res; > -} > - > /************************ exported alb funcions ************************/ > > int bond_alb_initialize(struct bonding *bond) > @@ -1777,15 +1721,10 @@ int bond_alb_set_mac_address(struct net_device *bond_dev, void *addr) > struct bonding *bond = netdev_priv(bond_dev); > struct sockaddr *sa = addr; > struct slave *swap_slave; > - int res; > > if (!is_valid_ether_addr(sa->sa_data)) > return -EADDRNOTAVAIL; > > - res = alb_set_mac_address(bond, addr); > - if (res) > - return res; > - > memcpy(bond_dev->dev_addr, sa->sa_data, bond_dev->addr_len); > > /* If there is no curr_active_slave there is nothing else to do. > -- 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 Thu, May 22, 2014 at 11:28:37AM -0400, Vlad Yasevich wrote: >On 05/22/2014 09:12 AM, Veaceslav Falico wrote: >> Currently it's called only from bond_alb_set_mac_address(), which is called >> only for ALB mode, and it does nothing in case the mode is ALB. So, >> basically, it's a no-op. All the needed functionality (modifying the active >> slave's mac address, per example) is handled by the >> bond_alb_set_mac_address() itself. >> >> So remove it, as it's not needed. > >From the comments and code, it seems like it should be called for TLB >mode as well as it does that weird half-setting of the address that >TLB seems to want. Is that not needed any more? It's still needed, but neither before my patch, nor after it works. The whole "save src mac address in dev->dev_addr" approach is wrong, and should be fixed - I'm working on it, and in the meanwhile cleaning the codebase. Basically, I see no point in fixing this code that never (at least till the beginning of this git tree) worked considering that we'll go away from it completely in the nearest future. OTOH, it'll be a basic fix (and then, anyway, removed ;) ), so I can send a v2 with it. > >-vlad > >> >> CC: Jay Vosburgh <j.vosburgh@gmail.com> >> CC: Andy Gospodarek <andy@greyhouse.net> >> Signed-off-by: Veaceslav Falico <vfalico@gmail.com> >> --- >> drivers/net/bonding/bond_alb.c | 61 ------------------------------------------ >> 1 file changed, 61 deletions(-) >> >> diff --git a/drivers/net/bonding/bond_alb.c b/drivers/net/bonding/bond_alb.c >> index 965518b..1d772b5 100644 >> --- a/drivers/net/bonding/bond_alb.c >> +++ b/drivers/net/bonding/bond_alb.c >> @@ -1256,62 +1256,6 @@ static int alb_handle_addr_collision_on_attach(struct bonding *bond, struct slav >> return 0; >> } >> >> -/** >> - * alb_set_mac_address >> - * @bond: >> - * @addr: >> - * >> - * In TLB mode all slaves are configured to the bond's hw address, but set >> - * their dev_addr field to different addresses (based on their permanent hw >> - * addresses). >> - * >> - * For each slave, this function sets the interface to the new address and then >> - * changes its dev_addr field to its previous value. >> - * >> - * Unwinding assumes bond's mac address has not yet changed. >> - */ >> -static int alb_set_mac_address(struct bonding *bond, void *addr) >> -{ >> - struct slave *slave, *rollback_slave; >> - struct list_head *iter; >> - struct sockaddr sa; >> - char tmp_addr[ETH_ALEN]; >> - int res; >> - >> - if (BOND_MODE(bond) == BOND_MODE_ALB) >> - return 0; >> - >> - bond_for_each_slave(bond, slave, iter) { >> - /* save net_device's current hw address */ >> - ether_addr_copy(tmp_addr, slave->dev->dev_addr); >> - >> - res = dev_set_mac_address(slave->dev, addr); >> - >> - /* restore net_device's hw address */ >> - ether_addr_copy(slave->dev->dev_addr, tmp_addr); >> - >> - if (res) >> - goto unwind; >> - } >> - >> - return 0; >> - >> -unwind: >> - memcpy(sa.sa_data, bond->dev->dev_addr, bond->dev->addr_len); >> - sa.sa_family = bond->dev->type; >> - >> - /* unwind from head to the slave that failed */ >> - bond_for_each_slave(bond, rollback_slave, iter) { >> - if (rollback_slave == slave) >> - break; >> - ether_addr_copy(tmp_addr, rollback_slave->dev->dev_addr); >> - dev_set_mac_address(rollback_slave->dev, &sa); >> - ether_addr_copy(rollback_slave->dev->dev_addr, tmp_addr); >> - } >> - >> - return res; >> -} >> - >> /************************ exported alb funcions ************************/ >> >> int bond_alb_initialize(struct bonding *bond) >> @@ -1777,15 +1721,10 @@ int bond_alb_set_mac_address(struct net_device *bond_dev, void *addr) >> struct bonding *bond = netdev_priv(bond_dev); >> struct sockaddr *sa = addr; >> struct slave *swap_slave; >> - int res; >> >> if (!is_valid_ether_addr(sa->sa_data)) >> return -EADDRNOTAVAIL; >> >> - res = alb_set_mac_address(bond, addr); >> - if (res) >> - return res; >> - >> memcpy(bond_dev->dev_addr, sa->sa_data, bond_dev->addr_len); >> >> /* If there is no curr_active_slave there is nothing else to do. >> > -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Veaceslav Falico <vfalico@gmail.com> wrote: >Currently it's called only from bond_alb_set_mac_address(), which is called >only for ALB mode, and it does nothing in case the mode is ALB. So, >basically, it's a no-op. All the needed functionality (modifying the active >slave's mac address, per example) is handled by the >bond_alb_set_mac_address() itself. > >So remove it, as it's not needed. I thought this seemed odd, and I did some looking. In the days of yore, before ndo_*, bond_alb_set_mac_address() was used for both alb and tlb modes, and alb_set_mac_address was not a no-op. That changed in this commit: commit eb7cc59a038b4e1914ae991d313f35904924759f Author: Stephen Hemminger <shemminger@vyatta.com> Date: Wed Nov 19 21:56:05 2008 -0800 bonding: convert to net_device_ops Convert to net_device_ops table. Note: for some operations move error checking into generic networking layer (rather than looking at pointers in bonding). [...] Before this, dev->set_mac_address was bond_alb_set_mac_address for both alb and tlb modes; after this commit, the now-unified bond_set_mac_address calls bond_alb_set_mac_address only for alb mode, and not for tlb mode. Looking at alb_set_mac_address, the comment block says that: * In TLB mode all slaves are configured to the bond's hw address, but set * their dev_addr field to different addresses (based on their permanent hw * addresses). * * For each slave, this function sets the interface to the new address and then * changes its dev_addr field to its previous value. And, sure enough, that's what the code does. So, again, in the days of yore, when a manual change of the bond's MAC took place, for tlb mode only, it would run through and change all of the slaves to the new bond MAC, and, importantly, preserve the dev->dev_addr tomfoolery that the tlb mode does. This preservation step appears to not happen currently (and, given the date of the commit above, hasn't for some years now). This makes me wonder if some of the occasional oddball tlb problems that crop up are due to this omission, since it looks to me like any time the bond's MAC is manually changed in tlb mode, the special stashing of MAC addresses in the slave dev->dev_addr won't be preserved, and all of the slaves would end up using the same MAC, which would not work so well. Separately, I'm not really sure how well this would have worked, given that the bond would be down to change the MAC, but the slaves would not necessarily also be down, and some of those slaves may not be able to change MAC while up. So, anyway, in the end, I think this is ok to delete, but only because the dev_addr MAC stash is about to be replaced with a New & Shiny somewhere-else MAC stash. It seems counterproductive to first fix this, and then almost immediately throw it all away. Since there seems to be an actual bug here (and not just useless dead code removal), I'm also inclined to prefer that this removal should occur in conjunction with the introduction of the somewhere-else stash. Also, since there is (theoretically) a bug here, it may be necessary to consider the new somewhere-else stash for stable backporting. -J >CC: Jay Vosburgh <j.vosburgh@gmail.com> >CC: Andy Gospodarek <andy@greyhouse.net> >Signed-off-by: Veaceslav Falico <vfalico@gmail.com> >--- > drivers/net/bonding/bond_alb.c | 61 ------------------------------------------ > 1 file changed, 61 deletions(-) > >diff --git a/drivers/net/bonding/bond_alb.c b/drivers/net/bonding/bond_alb.c >index 965518b..1d772b5 100644 >--- a/drivers/net/bonding/bond_alb.c >+++ b/drivers/net/bonding/bond_alb.c >@@ -1256,62 +1256,6 @@ static int alb_handle_addr_collision_on_attach(struct bonding *bond, struct slav > return 0; > } > >-/** >- * alb_set_mac_address >- * @bond: >- * @addr: >- * >- * In TLB mode all slaves are configured to the bond's hw address, but set >- * their dev_addr field to different addresses (based on their permanent hw >- * addresses). >- * >- * For each slave, this function sets the interface to the new address and then >- * changes its dev_addr field to its previous value. >- * >- * Unwinding assumes bond's mac address has not yet changed. >- */ >-static int alb_set_mac_address(struct bonding *bond, void *addr) >-{ >- struct slave *slave, *rollback_slave; >- struct list_head *iter; >- struct sockaddr sa; >- char tmp_addr[ETH_ALEN]; >- int res; >- >- if (BOND_MODE(bond) == BOND_MODE_ALB) >- return 0; >- >- bond_for_each_slave(bond, slave, iter) { >- /* save net_device's current hw address */ >- ether_addr_copy(tmp_addr, slave->dev->dev_addr); >- >- res = dev_set_mac_address(slave->dev, addr); >- >- /* restore net_device's hw address */ >- ether_addr_copy(slave->dev->dev_addr, tmp_addr); >- >- if (res) >- goto unwind; >- } >- >- return 0; >- >-unwind: >- memcpy(sa.sa_data, bond->dev->dev_addr, bond->dev->addr_len); >- sa.sa_family = bond->dev->type; >- >- /* unwind from head to the slave that failed */ >- bond_for_each_slave(bond, rollback_slave, iter) { >- if (rollback_slave == slave) >- break; >- ether_addr_copy(tmp_addr, rollback_slave->dev->dev_addr); >- dev_set_mac_address(rollback_slave->dev, &sa); >- ether_addr_copy(rollback_slave->dev->dev_addr, tmp_addr); >- } >- >- return res; >-} >- > /************************ exported alb funcions ************************/ > > int bond_alb_initialize(struct bonding *bond) >@@ -1777,15 +1721,10 @@ int bond_alb_set_mac_address(struct net_device *bond_dev, void *addr) > struct bonding *bond = netdev_priv(bond_dev); > struct sockaddr *sa = addr; > struct slave *swap_slave; >- int res; > > if (!is_valid_ether_addr(sa->sa_data)) > return -EADDRNOTAVAIL; > >- res = alb_set_mac_address(bond, addr); >- if (res) >- return res; >- > memcpy(bond_dev->dev_addr, sa->sa_data, bond_dev->addr_len); > > /* If there is no curr_active_slave there is nothing else to do. >-- >1.8.4 --- -Jay Vosburgh, jay.vosburgh@canonical.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 Thu, May 22, 2014 at 5:57 PM, Veaceslav Falico <vfalico@gmail.com> wrote: > > On Thu, May 22, 2014 at 05:36:19PM +0300, Or Gerlitz wrote: >> >> >> So with this patch @ hand, no more touching of the underlying device >> dev_addr field directly -- or we still have more accesses of this >> such, e.g in bond_alb_set_mac_address() and/or friends? > > > Sorry, but no :(. I'm still working on it, and already sent several > patchsets approaching it :). > > Hopefully it will be ready next week. If you need it urgently - patches welcome :) Hey -- no rush, I just asked as heads up since we have some proposed mlx4 driver change which was done also under the assumption that bonding touches directly the underlying dev_addr directly and I wanted to see where this stands, so we can re-evaluate the driver patch Or. -- 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_alb.c b/drivers/net/bonding/bond_alb.c index 965518b..1d772b5 100644 --- a/drivers/net/bonding/bond_alb.c +++ b/drivers/net/bonding/bond_alb.c @@ -1256,62 +1256,6 @@ static int alb_handle_addr_collision_on_attach(struct bonding *bond, struct slav return 0; } -/** - * alb_set_mac_address - * @bond: - * @addr: - * - * In TLB mode all slaves are configured to the bond's hw address, but set - * their dev_addr field to different addresses (based on their permanent hw - * addresses). - * - * For each slave, this function sets the interface to the new address and then - * changes its dev_addr field to its previous value. - * - * Unwinding assumes bond's mac address has not yet changed. - */ -static int alb_set_mac_address(struct bonding *bond, void *addr) -{ - struct slave *slave, *rollback_slave; - struct list_head *iter; - struct sockaddr sa; - char tmp_addr[ETH_ALEN]; - int res; - - if (BOND_MODE(bond) == BOND_MODE_ALB) - return 0; - - bond_for_each_slave(bond, slave, iter) { - /* save net_device's current hw address */ - ether_addr_copy(tmp_addr, slave->dev->dev_addr); - - res = dev_set_mac_address(slave->dev, addr); - - /* restore net_device's hw address */ - ether_addr_copy(slave->dev->dev_addr, tmp_addr); - - if (res) - goto unwind; - } - - return 0; - -unwind: - memcpy(sa.sa_data, bond->dev->dev_addr, bond->dev->addr_len); - sa.sa_family = bond->dev->type; - - /* unwind from head to the slave that failed */ - bond_for_each_slave(bond, rollback_slave, iter) { - if (rollback_slave == slave) - break; - ether_addr_copy(tmp_addr, rollback_slave->dev->dev_addr); - dev_set_mac_address(rollback_slave->dev, &sa); - ether_addr_copy(rollback_slave->dev->dev_addr, tmp_addr); - } - - return res; -} - /************************ exported alb funcions ************************/ int bond_alb_initialize(struct bonding *bond) @@ -1777,15 +1721,10 @@ int bond_alb_set_mac_address(struct net_device *bond_dev, void *addr) struct bonding *bond = netdev_priv(bond_dev); struct sockaddr *sa = addr; struct slave *swap_slave; - int res; if (!is_valid_ether_addr(sa->sa_data)) return -EADDRNOTAVAIL; - res = alb_set_mac_address(bond, addr); - if (res) - return res; - memcpy(bond_dev->dev_addr, sa->sa_data, bond_dev->addr_len); /* If there is no curr_active_slave there is nothing else to do.
Currently it's called only from bond_alb_set_mac_address(), which is called only for ALB mode, and it does nothing in case the mode is ALB. So, basically, it's a no-op. All the needed functionality (modifying the active slave's mac address, per example) is handled by the bond_alb_set_mac_address() itself. So remove it, as it's not needed. CC: Jay Vosburgh <j.vosburgh@gmail.com> CC: Andy Gospodarek <andy@greyhouse.net> Signed-off-by: Veaceslav Falico <vfalico@gmail.com> --- drivers/net/bonding/bond_alb.c | 61 ------------------------------------------ 1 file changed, 61 deletions(-)