diff mbox series

[V3,mlx5-next,08/15] bonding: Add array of all salves

Message ID 20200421102844.23640-9-maorg@mellanox.com (mailing list archive)
State Superseded
Headers show
Series Add support to get xmit slave | expand

Commit Message

Maor Gottlieb April 21, 2020, 10:28 a.m. UTC 
Keep all slaves in array so it could be used to get the xmit slave
assume all the slaves are active.
The logic to add slave to the array is like the usable slaves, except
that we also add slaves that currently can't transmit - not up or active.

Signed-off-by: Maor Gottlieb <maorg@mellanox.com>
---
 drivers/net/bonding/bond_main.c | 80 +++++++++++++++++++++++++--------
 include/net/bonding.h           |  1 +
 2 files changed, 62 insertions(+), 19 deletions(-)

Comments

Jiri Pirko April 21, 2020, 7:47 p.m. UTC | #1 
Tue, Apr 21, 2020 at 12:28:37PM CEST, maorg@mellanox.com wrote:
>Keep all slaves in array so it could be used to get the xmit slave
>assume all the slaves are active.
>The logic to add slave to the array is like the usable slaves, except
>that we also add slaves that currently can't transmit - not up or active.
>
>Signed-off-by: Maor Gottlieb <maorg@mellanox.com>

Reviewed-by: Jiri Pirko <jiri@mellanox.com>
Jay Vosburgh April 21, 2020, 8:33 p.m. UTC | #2 
Maor Gottlieb <maorg@mellanox.com> wrote:

>Keep all slaves in array so it could be used to get the xmit slave
>assume all the slaves are active.
>The logic to add slave to the array is like the usable slaves, except
>that we also add slaves that currently can't transmit - not up or active.

	Typo: in the Subject, slaves is misspelled "salves."

>Signed-off-by: Maor Gottlieb <maorg@mellanox.com>
>---
> drivers/net/bonding/bond_main.c | 80 +++++++++++++++++++++++++--------
> include/net/bonding.h           |  1 +
> 2 files changed, 62 insertions(+), 19 deletions(-)
>
>diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
>index 1b0ae750d732..c37fd57bfcd4 100644
>--- a/drivers/net/bonding/bond_main.c
>+++ b/drivers/net/bonding/bond_main.c
>@@ -4120,6 +4120,40 @@ static void bond_skip_slave(struct bond_up_slave *slaves,
> 	}
> }
> 
>+static void bond_set_slave_arr(struct bonding *bond,
>+			       struct bond_up_slave *usable_slaves,
>+			       struct bond_up_slave *all_slaves)
>+{
>+	struct bond_up_slave *usable, *all;
>+
>+	usable = rtnl_dereference(bond->usable_slaves);
>+	rcu_assign_pointer(bond->usable_slaves, usable_slaves);
>+	if (usable)
>+		kfree_rcu(usable, rcu);
>+
>+	all = rtnl_dereference(bond->all_slaves);
>+	rcu_assign_pointer(bond->all_slaves, all_slaves);
>+	if (all)
>+		kfree_rcu(all, rcu);

	Minor nit: kfree_rcu can accept a NULL pointer, so testing
beforehand is unnecessary.


>+}
>+
>+static void bond_reset_slave_arr(struct bonding *bond)
>+{
>+	struct bond_up_slave *usable, *all;
>+
>+	usable = rtnl_dereference(bond->usable_slaves);
>+	if (usable) {
>+		RCU_INIT_POINTER(bond->usable_slaves, NULL);
>+		kfree_rcu(usable, rcu);
>+	}
>+
>+	all = rtnl_dereference(bond->all_slaves);
>+	if (all) {
>+		RCU_INIT_POINTER(bond->all_slaves, NULL);
>+		kfree_rcu(all, rcu);
>+	}
>+}
>+
> /* Build the usable slaves array in control path for modes that use xmit-hash
>  * to determine the slave interface -
>  * (a) BOND_MODE_8023AD
>@@ -4130,7 +4164,7 @@ static void bond_skip_slave(struct bond_up_slave *slaves,
>  */
> int bond_update_slave_arr(struct bonding *bond, struct slave *skipslave)
> {
>-	struct bond_up_slave *usable_slaves, *old_usable_slaves;
>+	struct bond_up_slave *usable_slaves = NULL, *all_slaves = NULL;
> 	struct slave *slave;
> 	struct list_head *iter;
> 	int agg_id = 0;
>@@ -4142,7 +4176,9 @@ int bond_update_slave_arr(struct bonding *bond, struct slave *skipslave)
> 
> 	usable_slaves = kzalloc(struct_size(usable_slaves, arr,
> 					    bond->slave_cnt), GFP_KERNEL);
>-	if (!usable_slaves) {
>+	all_slaves = kzalloc(struct_size(all_slaves, arr,
>+					 bond->slave_cnt), GFP_KERNEL);
>+	if (!usable_slaves || !all_slaves) {
> 		ret = -ENOMEM;
> 		goto out;
> 	}
>@@ -4151,20 +4187,19 @@ int bond_update_slave_arr(struct bonding *bond, struct slave *skipslave)
> 
> 		if (bond_3ad_get_active_agg_info(bond, &ad_info)) {
> 			pr_debug("bond_3ad_get_active_agg_info failed\n");
>-			kfree_rcu(usable_slaves, rcu);
> 			/* No active aggragator means it's not safe to use
> 			 * the previous array.
> 			 */
>-			old_usable_slaves = rtnl_dereference(bond->usable_slaves);
>-			if (old_usable_slaves) {
>-				RCU_INIT_POINTER(bond->usable_slaves, NULL);
>-				kfree_rcu(old_usable_slaves, rcu);
>-			}
>+			bond_reset_slave_arr(bond);
> 			goto out;
> 		}
> 		agg_id = ad_info.aggregator_id;
> 	}
> 	bond_for_each_slave(bond, slave, iter) {
>+		if (skipslave == slave)
>+			continue;
>+
>+		all_slaves->arr[all_slaves->count++] = slave;
> 		if (BOND_MODE(bond) == BOND_MODE_8023AD) {
> 			struct aggregator *agg;
> 
>@@ -4174,8 +4209,6 @@ int bond_update_slave_arr(struct bonding *bond, struct slave *skipslave)
> 		}
> 		if (!bond_slave_can_tx(slave))
> 			continue;
>-		if (skipslave == slave)
>-			continue;
> 
> 		slave_dbg(bond->dev, slave->dev, "Adding slave to tx hash array[%d]\n",
> 			  usable_slaves->count);
>@@ -4183,14 +4216,17 @@ int bond_update_slave_arr(struct bonding *bond, struct slave *skipslave)
> 		usable_slaves->arr[usable_slaves->count++] = slave;
> 	}
> 
>-	old_usable_slaves = rtnl_dereference(bond->usable_slaves);
>-	rcu_assign_pointer(bond->usable_slaves, usable_slaves);
>-	if (old_usable_slaves)
>-		kfree_rcu(old_usable_slaves, rcu);
>+	bond_set_slave_arr(bond, usable_slaves, all_slaves);
>+	return ret;
> out:
>-	if (ret != 0 && skipslave)
>+	if (ret != 0 && skipslave) {
>+		bond_skip_slave(rtnl_dereference(bond->all_slaves),
>+				skipslave);
> 		bond_skip_slave(rtnl_dereference(bond->usable_slaves),
> 				skipslave);
>+	}
>+	kfree_rcu(all_slaves, rcu);
>+	kfree_rcu(usable_slaves, rcu);
> 
> 	return ret;
> }
>@@ -4501,9 +4537,9 @@ void bond_setup(struct net_device *bond_dev)
> static void bond_uninit(struct net_device *bond_dev)
> {
> 	struct bonding *bond = netdev_priv(bond_dev);
>+	struct bond_up_slave *usable, *all;
> 	struct list_head *iter;
> 	struct slave *slave;
>-	struct bond_up_slave *arr;
> 
> 	bond_netpoll_cleanup(bond_dev);
> 
>@@ -4512,10 +4548,16 @@ static void bond_uninit(struct net_device *bond_dev)
> 		__bond_release_one(bond_dev, slave->dev, true, true);
> 	netdev_info(bond_dev, "Released all slaves\n");
> 
>-	arr = rtnl_dereference(bond->usable_slaves);
>-	if (arr) {
>+	usable = rtnl_dereference(bond->usable_slaves);
>+	if (usable) {
> 		RCU_INIT_POINTER(bond->usable_slaves, NULL);
>-		kfree_rcu(arr, rcu);
>+		kfree_rcu(usable, rcu);
>+	}
>+
>+	all = rtnl_dereference(bond->all_slaves);
>+	if (all) {
>+		RCU_INIT_POINTER(bond->all_slaves, NULL);
>+		kfree_rcu(all, rcu);
> 	}
> 
> 	list_del(&bond->bond_list);
>diff --git a/include/net/bonding.h b/include/net/bonding.h
>index 33bdb6d5182d..a2a7f461fa63 100644
>--- a/include/net/bonding.h
>+++ b/include/net/bonding.h
>@@ -201,6 +201,7 @@ struct bonding {
> 	struct   slave __rcu *current_arp_slave;
> 	struct   slave __rcu *primary_slave;
> 	struct   bond_up_slave __rcu *usable_slaves; /* Array of usable slaves */
>+	struct   bond_up_slave __rcu *all_slaves; /* Array of all slaves */

	Another nit: these comments don't really add much now, given the
new names of the arrays.  I don't know if the nits are worth respinning
the patch set, but the Subject ought to get fixed.

	I've looked at the other bonding patches in the series and don't
have any other comments, so for the series:

Reviewed-by: Jay Vosburgh <jay.vosburgh@canonical.com>

	-J

> 	bool     force_primary;
> 	s32      slave_cnt; /* never change this value outside the attach/detach wrappers */
> 	int     (*recv_probe)(const struct sk_buff *, struct bonding *,
>-- 
>2.17.2
>

---
	-Jay Vosburgh, jay.vosburgh@canonical.com
diff mbox series

Patch

diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
index 1b0ae750d732..c37fd57bfcd4 100644
--- a/drivers/net/bonding/bond_main.c
+++ b/drivers/net/bonding/bond_main.c
@@ -4120,6 +4120,40 @@  static void bond_skip_slave(struct bond_up_slave *slaves,
 	}
 }
 
+static void bond_set_slave_arr(struct bonding *bond,
+			       struct bond_up_slave *usable_slaves,
+			       struct bond_up_slave *all_slaves)
+{
+	struct bond_up_slave *usable, *all;
+
+	usable = rtnl_dereference(bond->usable_slaves);
+	rcu_assign_pointer(bond->usable_slaves, usable_slaves);
+	if (usable)
+		kfree_rcu(usable, rcu);
+
+	all = rtnl_dereference(bond->all_slaves);
+	rcu_assign_pointer(bond->all_slaves, all_slaves);
+	if (all)
+		kfree_rcu(all, rcu);
+}
+
+static void bond_reset_slave_arr(struct bonding *bond)
+{
+	struct bond_up_slave *usable, *all;
+
+	usable = rtnl_dereference(bond->usable_slaves);
+	if (usable) {
+		RCU_INIT_POINTER(bond->usable_slaves, NULL);
+		kfree_rcu(usable, rcu);
+	}
+
+	all = rtnl_dereference(bond->all_slaves);
+	if (all) {
+		RCU_INIT_POINTER(bond->all_slaves, NULL);
+		kfree_rcu(all, rcu);
+	}
+}
+
 /* Build the usable slaves array in control path for modes that use xmit-hash
  * to determine the slave interface -
  * (a) BOND_MODE_8023AD
@@ -4130,7 +4164,7 @@  static void bond_skip_slave(struct bond_up_slave *slaves,
  */
 int bond_update_slave_arr(struct bonding *bond, struct slave *skipslave)
 {
-	struct bond_up_slave *usable_slaves, *old_usable_slaves;
+	struct bond_up_slave *usable_slaves = NULL, *all_slaves = NULL;
 	struct slave *slave;
 	struct list_head *iter;
 	int agg_id = 0;
@@ -4142,7 +4176,9 @@  int bond_update_slave_arr(struct bonding *bond, struct slave *skipslave)
 
 	usable_slaves = kzalloc(struct_size(usable_slaves, arr,
 					    bond->slave_cnt), GFP_KERNEL);
-	if (!usable_slaves) {
+	all_slaves = kzalloc(struct_size(all_slaves, arr,
+					 bond->slave_cnt), GFP_KERNEL);
+	if (!usable_slaves || !all_slaves) {
 		ret = -ENOMEM;
 		goto out;
 	}
@@ -4151,20 +4187,19 @@  int bond_update_slave_arr(struct bonding *bond, struct slave *skipslave)
 
 		if (bond_3ad_get_active_agg_info(bond, &ad_info)) {
 			pr_debug("bond_3ad_get_active_agg_info failed\n");
-			kfree_rcu(usable_slaves, rcu);
 			/* No active aggragator means it's not safe to use
 			 * the previous array.
 			 */
-			old_usable_slaves = rtnl_dereference(bond->usable_slaves);
-			if (old_usable_slaves) {
-				RCU_INIT_POINTER(bond->usable_slaves, NULL);
-				kfree_rcu(old_usable_slaves, rcu);
-			}
+			bond_reset_slave_arr(bond);
 			goto out;
 		}
 		agg_id = ad_info.aggregator_id;
 	}
 	bond_for_each_slave(bond, slave, iter) {
+		if (skipslave == slave)
+			continue;
+
+		all_slaves->arr[all_slaves->count++] = slave;
 		if (BOND_MODE(bond) == BOND_MODE_8023AD) {
 			struct aggregator *agg;
 
@@ -4174,8 +4209,6 @@  int bond_update_slave_arr(struct bonding *bond, struct slave *skipslave)
 		}
 		if (!bond_slave_can_tx(slave))
 			continue;
-		if (skipslave == slave)
-			continue;
 
 		slave_dbg(bond->dev, slave->dev, "Adding slave to tx hash array[%d]\n",
 			  usable_slaves->count);
@@ -4183,14 +4216,17 @@  int bond_update_slave_arr(struct bonding *bond, struct slave *skipslave)
 		usable_slaves->arr[usable_slaves->count++] = slave;
 	}
 
-	old_usable_slaves = rtnl_dereference(bond->usable_slaves);
-	rcu_assign_pointer(bond->usable_slaves, usable_slaves);
-	if (old_usable_slaves)
-		kfree_rcu(old_usable_slaves, rcu);
+	bond_set_slave_arr(bond, usable_slaves, all_slaves);
+	return ret;
 out:
-	if (ret != 0 && skipslave)
+	if (ret != 0 && skipslave) {
+		bond_skip_slave(rtnl_dereference(bond->all_slaves),
+				skipslave);
 		bond_skip_slave(rtnl_dereference(bond->usable_slaves),
 				skipslave);
+	}
+	kfree_rcu(all_slaves, rcu);
+	kfree_rcu(usable_slaves, rcu);
 
 	return ret;
 }
@@ -4501,9 +4537,9 @@  void bond_setup(struct net_device *bond_dev)
 static void bond_uninit(struct net_device *bond_dev)
 {
 	struct bonding *bond = netdev_priv(bond_dev);
+	struct bond_up_slave *usable, *all;
 	struct list_head *iter;
 	struct slave *slave;
-	struct bond_up_slave *arr;
 
 	bond_netpoll_cleanup(bond_dev);
 
@@ -4512,10 +4548,16 @@  static void bond_uninit(struct net_device *bond_dev)
 		__bond_release_one(bond_dev, slave->dev, true, true);
 	netdev_info(bond_dev, "Released all slaves\n");
 
-	arr = rtnl_dereference(bond->usable_slaves);
-	if (arr) {
+	usable = rtnl_dereference(bond->usable_slaves);
+	if (usable) {
 		RCU_INIT_POINTER(bond->usable_slaves, NULL);
-		kfree_rcu(arr, rcu);
+		kfree_rcu(usable, rcu);
+	}
+
+	all = rtnl_dereference(bond->all_slaves);
+	if (all) {
+		RCU_INIT_POINTER(bond->all_slaves, NULL);
+		kfree_rcu(all, rcu);
 	}
 
 	list_del(&bond->bond_list);
diff --git a/include/net/bonding.h b/include/net/bonding.h
index 33bdb6d5182d..a2a7f461fa63 100644
--- a/include/net/bonding.h
+++ b/include/net/bonding.h
@@ -201,6 +201,7 @@  struct bonding {
 	struct   slave __rcu *current_arp_slave;
 	struct   slave __rcu *primary_slave;
 	struct   bond_up_slave __rcu *usable_slaves; /* Array of usable slaves */
+	struct   bond_up_slave __rcu *all_slaves; /* Array of all slaves */
 	bool     force_primary;
 	s32      slave_cnt; /* never change this value outside the attach/detach wrappers */
 	int     (*recv_probe)(const struct sk_buff *, struct bonding *,