[PATCH net-next 0/2] bonding: remove rlb_enabled and alb_set_mac_address()

[PATCH net-next 0/2] bonding: remove rlb_enabled and alb_set_mac_address()

[Veaceslav Falico]

Hi,

Currently rlb_enabled works as
rlb_enabled == (BOND_MODE(bond) == BOND_MODE_ALB)
so we can always use this verification and drop maintenance of this
variable.

alb_set_mac_address(), OTOH, isn't used at all (as the mac changing logic
is used in other parts), and isn't ever called, so it's safe to remove it
as well.

CC: Jay Vosburgh <j.vosburgh@gmail.com>
CC: Andy Gospodarek <andy@greyhouse.net>
CC: netdev@vger.kernel.org
Signed-off-by: Veaceslav Falico <vfalico@gmail.com>

---
 drivers/net/bonding/bond_alb.c  | 95 ++++++-----------------------------------
 drivers/net/bonding/bond_alb.h  |  3 +-
 drivers/net/bonding/bond_main.c |  2 +-
 3 files changed, 16 insertions(+), 84 deletions(-)

[PATCH net-next 1/2] bonding: remove rlb_enabled and use mode == ALB instead

[Veaceslav Falico]

Currently rlb_enabled is set only when bonding mode is ALB, and 0
otherwise, so just use the mode == ALB verification instead of maintaining
additional variable.

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  | 36 +++++++++++++++---------------------
 drivers/net/bonding/bond_alb.h  |  3 +--
 drivers/net/bonding/bond_main.c |  2 +-
 3 files changed, 17 insertions(+), 24 deletions(-)

diff --git a/drivers/net/bonding/bond_alb.c b/drivers/net/bonding/bond_alb.c
index 03e0bca..965518b 100644
--- a/drivers/net/bonding/bond_alb.c
+++ b/drivers/net/bonding/bond_alb.c
@@ -1108,7 +1108,7 @@ static void alb_fasten_mac_swap(struct bonding *bond, struct slave *slave1,
 	/* fasten the change in the switch */
 	if (bond_slave_can_tx(slave1)) {
 		alb_send_learning_packets(slave1, slave1->dev->dev_addr);
-		if (bond->alb_info.rlb_enabled) {
+		if (BOND_MODE(bond) == BOND_MODE_ALB) {
 			/* inform the clients that the mac address
 			 * has changed
 			 */
@@ -1120,7 +1120,7 @@ static void alb_fasten_mac_swap(struct bonding *bond, struct slave *slave1,
 
 	if (bond_slave_can_tx(slave2)) {
 		alb_send_learning_packets(slave2, slave2->dev->dev_addr);
-		if (bond->alb_info.rlb_enabled) {
+		if (BOND_MODE(bond) == BOND_MODE_ALB) {
 			/* inform the clients that the mac address
 			 * has changed
 			 */
@@ -1130,7 +1130,7 @@ static void alb_fasten_mac_swap(struct bonding *bond, struct slave *slave1,
 		disabled_slave = slave2;
 	}
 
-	if (bond->alb_info.rlb_enabled && slaves_state_differ) {
+	if (BOND_MODE(bond) == BOND_MODE_ALB && slaves_state_differ) {
 		/* A disabled slave was assigned an active mac addr */
 		rlb_teach_disabled_mac_on_primary(bond,
 						  disabled_slave->dev->dev_addr);
@@ -1278,7 +1278,7 @@ static int alb_set_mac_address(struct bonding *bond, void *addr)
 	char tmp_addr[ETH_ALEN];
 	int res;
 
-	if (bond->alb_info.rlb_enabled)
+	if (BOND_MODE(bond) == BOND_MODE_ALB)
 		return 0;
 
 	bond_for_each_slave(bond, slave, iter) {
@@ -1314,7 +1314,7 @@ unwind:
 
 /************************ exported alb funcions ************************/
 
-int bond_alb_initialize(struct bonding *bond, int rlb_enabled)
+int bond_alb_initialize(struct bonding *bond)
 {
 	int res;
 
@@ -1322,16 +1322,13 @@ int bond_alb_initialize(struct bonding *bond, int rlb_enabled)
 	if (res)
 		return res;
 
-	if (rlb_enabled) {
-		bond->alb_info.rlb_enabled = 1;
+	if (BOND_MODE(bond) == BOND_MODE_ALB) {
 		/* initialize rlb */
 		res = rlb_initialize(bond);
 		if (res) {
 			tlb_deinitialize(bond);
 			return res;
 		}
-	} else {
-		bond->alb_info.rlb_enabled = 0;
 	}
 
 	return 0;
@@ -1339,11 +1336,9 @@ int bond_alb_initialize(struct bonding *bond, int rlb_enabled)
 
 void bond_alb_deinitialize(struct bonding *bond)
 {
-	struct alb_bond_info *bond_info = &(BOND_ALB_INFO(bond));
-
 	tlb_deinitialize(bond);
 
-	if (bond_info->rlb_enabled)
+	if (BOND_MODE(bond) == BOND_MODE_ALB)
 		rlb_deinitialize(bond);
 }
 
@@ -1422,7 +1417,6 @@ int bond_alb_xmit(struct sk_buff *skb, struct net_device *bond_dev)
 {
 	struct bonding *bond = netdev_priv(bond_dev);
 	struct ethhdr *eth_data;
-	struct alb_bond_info *bond_info = &(BOND_ALB_INFO(bond));
 	struct slave *tx_slave = NULL;
 	static const __be32 ip_bcast = htonl(0xffffffff);
 	int hash_size = 0;
@@ -1499,7 +1493,7 @@ int bond_alb_xmit(struct sk_buff *skb, struct net_device *bond_dev)
 		break;
 	case ETH_P_ARP:
 		do_tx_balance = false;
-		if (bond_info->rlb_enabled)
+		if (BOND_MODE(bond) == BOND_MODE_ALB)
 			tx_slave = rlb_arp_xmit(skb, bond);
 		break;
 	default:
@@ -1572,7 +1566,7 @@ void bond_alb_monitor(struct work_struct *work)
 	}
 
 	/* handle rlb stuff */
-	if (bond_info->rlb_enabled) {
+	if (BOND_MODE(bond) == BOND_MODE_ALB) {
 		if (bond_info->primary_is_promisc &&
 		    (++bond_info->rlb_promisc_timeout_counter >= RLB_PROMISC_TIMEOUT)) {
 
@@ -1640,7 +1634,7 @@ int bond_alb_init_slave(struct bonding *bond, struct slave *slave)
 	/* order a rebalance ASAP */
 	bond->alb_info.tx_rebalance_counter = BOND_TLB_REBALANCE_TICKS;
 
-	if (bond->alb_info.rlb_enabled)
+	if (BOND_MODE(bond) == BOND_MODE_ALB)
 		bond->alb_info.rlb_rebalance = 1;
 
 	return 0;
@@ -1659,7 +1653,7 @@ void bond_alb_deinit_slave(struct bonding *bond, struct slave *slave)
 
 	tlb_clear_slave(bond, slave, 0);
 
-	if (bond->alb_info.rlb_enabled) {
+	if (BOND_MODE(bond) == BOND_MODE_ALB) {
 		bond->alb_info.rx_slave = NULL;
 		rlb_clear_slave(bond, slave);
 	}
@@ -1672,12 +1666,12 @@ void bond_alb_handle_link_change(struct bonding *bond, struct slave *slave, char
 
 	if (link == BOND_LINK_DOWN) {
 		tlb_clear_slave(bond, slave, 0);
-		if (bond->alb_info.rlb_enabled)
+		if (BOND_MODE(bond) == BOND_MODE_ALB)
 			rlb_clear_slave(bond, slave);
 	} else if (link == BOND_LINK_UP) {
 		/* order a rebalance ASAP */
 		bond_info->tx_rebalance_counter = BOND_TLB_REBALANCE_TICKS;
-		if (bond->alb_info.rlb_enabled) {
+		if (BOND_MODE(bond) == BOND_MODE_ALB) {
 			bond->alb_info.rlb_rebalance = 1;
 			/* If the updelay module parameter is smaller than the
 			 * forwarding delay of the switch the rebalance will
@@ -1811,7 +1805,7 @@ int bond_alb_set_mac_address(struct net_device *bond_dev, void *addr)
 
 		read_lock(&bond->lock);
 		alb_send_learning_packets(bond->curr_active_slave, bond_dev->dev_addr);
-		if (bond->alb_info.rlb_enabled) {
+		if (BOND_MODE(bond) == BOND_MODE_ALB) {
 			/* inform clients mac address has changed */
 			rlb_req_update_slave_clients(bond, bond->curr_active_slave);
 		}
@@ -1823,7 +1817,7 @@ int bond_alb_set_mac_address(struct net_device *bond_dev, void *addr)
 
 void bond_alb_clear_vlan(struct bonding *bond, unsigned short vlan_id)
 {
-	if (bond->alb_info.rlb_enabled)
+	if (BOND_MODE(bond) == BOND_MODE_ALB)
 		rlb_clear_vlan(bond, vlan_id);
 }
 
diff --git a/drivers/net/bonding/bond_alb.h b/drivers/net/bonding/bond_alb.h
index 5fc76c0..fee7a26 100644
--- a/drivers/net/bonding/bond_alb.h
+++ b/drivers/net/bonding/bond_alb.h
@@ -146,7 +146,6 @@ struct alb_bond_info {
 	int			tx_rebalance_counter;
 	int			lp_counter;
 	/* -------- rlb parameters -------- */
-	int rlb_enabled;
 	struct rlb_client_info	*rx_hashtbl;	/* Receive hash table */
 	spinlock_t		rx_hashtbl_lock;
 	u32			rx_hashtbl_used_head;
@@ -168,7 +167,7 @@ struct alb_bond_info {
 						 */
 };
 
-int bond_alb_initialize(struct bonding *bond, int rlb_enabled);
+int bond_alb_initialize(struct bonding *bond);
 void bond_alb_deinitialize(struct bonding *bond);
 int bond_alb_init_slave(struct bonding *bond, struct slave *slave);
 void bond_alb_deinit_slave(struct bonding *bond, struct slave *slave);
diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
index 7123205..1c7ae78 100644
--- a/drivers/net/bonding/bond_main.c
+++ b/drivers/net/bonding/bond_main.c
@@ -3124,7 +3124,7 @@ static int bond_open(struct net_device *bond_dev)
 		/* bond_alb_initialize must be called before the timer
 		 * is started.
 		 */
-		if (bond_alb_initialize(bond, (BOND_MODE(bond) == BOND_MODE_ALB)))
+		if (bond_alb_initialize(bond))
 			return -ENOMEM;
 		if (bond->params.tlb_dynamic_lb)
 			queue_delayed_work(bond->wq, &bond->alb_work, 0);
-- 
1.8.4

[PATCH net-next 2/2] bonding: remove alb_set_mac_address()

[Veaceslav Falico]

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(-)

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

Re: [PATCH net-next 2/2] bonding: remove alb_set_mac_address()

[Or Gerlitz]

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.

Re: [PATCH net-next 2/2] bonding: remove alb_set_mac_address()

[Veaceslav Falico]

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.

Re: [PATCH net-next 2/2] bonding: remove alb_set_mac_address()

[Vlad Yasevich]

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.
> 

Re: [PATCH net-next 2/2] bonding: remove alb_set_mac_address()

[Veaceslav Falico]

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.
>>
>

Re: [PATCH net-next 2/2] bonding: remove alb_set_mac_address()

[Jay Vosburgh]

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

Re: [PATCH net-next 0/2] bonding: remove rlb_enabled and alb_set_mac_address()

[David Miller]

From: Veaceslav Falico <vfalico@gmail.com>
Date: Thu, 22 May 2014 15:12:02 +0200

> Currently rlb_enabled works as
> rlb_enabled == (BOND_MODE(bond) == BOND_MODE_ALB)
> so we can always use this verification and drop maintenance of this
> variable.
> 
> alb_set_mac_address(), OTOH, isn't used at all (as the mac changing logic
> is used in other parts), and isn't ever called, so it's safe to remove it
> as well.
> 
> CC: Jay Vosburgh <j.vosburgh@gmail.com>
> CC: Andy Gospodarek <andy@greyhouse.net>
> CC: netdev@vger.kernel.org
> Signed-off-by: Veaceslav Falico <vfalico@gmail.com>

I agree with Jay's feedback and that you should:

1) Just go to the new MAC stash scheme in net-next instead of trying
   to fix something you intend to immediately remove.

2) Something to fix this as minimally as possible for 'net' and
   -stable should be done as well.

I'm therefore not applying this series to the tree.

Thanks.

Re: [PATCH net-next 2/2] bonding: remove alb_set_mac_address()

[Or Gerlitz]

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.

Re: [PATCH net-next 0/2] bonding: remove rlb_enabled and alb_set_mac_address()

[Or Gerlitz]

On Fri, May 23, 2014 at 12:22 AM, David Miller <davem@davemloft.net> wrote:
>
> From: Veaceslav Falico <vfalico@gmail.com>
> Date: Thu, 22 May 2014 15:12:02 +0200
>
> > Currently rlb_enabled works as
> > rlb_enabled == (BOND_MODE(bond) == BOND_MODE_ALB)
> > so we can always use this verification and drop maintenance of this
> > variable.
> >
> > alb_set_mac_address(), OTOH, isn't used at all (as the mac changing logic
> > is used in other parts), and isn't ever called, so it's safe to remove it
> > as well.
> >
> > CC: Jay Vosburgh <j.vosburgh@gmail.com>
> > CC: Andy Gospodarek <andy@greyhouse.net>
> > CC: netdev@vger.kernel.org
> > Signed-off-by: Veaceslav Falico <vfalico@gmail.com>
>
> I agree with Jay's feedback and that you should:
>
> 1) Just go to the new MAC stash scheme in net-next instead of trying
>    to fix something you intend to immediately remove.
>
> 2) Something to fix this as minimally as possible for 'net' and
>    -stable should be done as well.
>
> I'm therefore not applying this series to the tree.

Hi Veaceslav ,

Just a reminder, any progress on this matter?

Or.