Re: [PATCH net-next 3/4] bonding: allow use of tx hashing in balance-alb

[Jay Vosburgh <jay.vosburgh@canonical.com>]

Debabrata Banerjee <dbanerje@akamai.com> wrote:

>The rx load balancing provided by balance-alb is not mutually
>exclusive with using hashing for tx selection, and should provide a decent
>speed increase because this eliminates spinlocks and cache contention.
>
>Signed-off-by: Debabrata Banerjee <dbanerje@akamai.com>
>---
> drivers/net/bonding/bond_alb.c     | 20 ++++++++++++++++++--
> drivers/net/bonding/bond_main.c    | 25 +++++++++++++++----------
> drivers/net/bonding/bond_options.c |  2 +-
> include/net/bonding.h              | 10 +++++++++-
> 4 files changed, 43 insertions(+), 14 deletions(-)
>
>diff --git a/drivers/net/bonding/bond_alb.c b/drivers/net/bonding/bond_alb.c
>index 180e50f7806f..6228635880d5 100644
>--- a/drivers/net/bonding/bond_alb.c
>+++ b/drivers/net/bonding/bond_alb.c
>@@ -1478,8 +1478,24 @@ int bond_alb_xmit(struct sk_buff *skb, struct net_device *bond_dev)
> 	}
> 
> 	if (do_tx_balance) {
>-		hash_index = _simple_hash(hash_start, hash_size);
>-		tx_slave = tlb_choose_channel(bond, hash_index, skb->len);
>+		if (bond->params.tlb_dynamic_lb) {
>+			hash_index = _simple_hash(hash_start, hash_size);
>+			tx_slave = tlb_choose_channel(bond, hash_index, skb->len);
>+		} else {
>+			/*
>+			 * do_tx_balance means we are free to select the tx_slave
>+			 * So we do exactly what tlb would do for hash selection
>+			 */
>+
>+			struct bond_up_slave *slaves;
>+			unsigned int count;
>+
>+			slaves = rcu_dereference(bond->slave_arr);
>+			count = slaves ? READ_ONCE(slaves->count) : 0;
>+			if (likely(count))
>+				tx_slave = slaves->arr[bond_xmit_hash(bond, skb) %
>+						       count];
>+		}
> 	}
> 
> 	return bond_do_alb_xmit(skb, bond, tx_slave);
>diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
>index 1f1e97b26f95..f7f8a49cb32b 100644
>--- a/drivers/net/bonding/bond_main.c
>+++ b/drivers/net/bonding/bond_main.c
>@@ -159,7 +159,7 @@ module_param(min_links, int, 0);
> MODULE_PARM_DESC(min_links, "Minimum number of available links before turning on carrier");
> 
> module_param(xmit_hash_policy, charp, 0);
>-MODULE_PARM_DESC(xmit_hash_policy, "balance-xor and 802.3ad hashing method; "
>+MODULE_PARM_DESC(xmit_hash_policy, "balance-alb, balance-tlb, balance-xor, 802.3ad hashing method; "
> 				   "0 for layer 2 (default), 1 for layer 3+4, "
> 				   "2 for layer 2+3, 3 for encap layer 2+3, "
> 				   "4 for encap layer 3+4");
>@@ -1735,7 +1735,7 @@ int bond_enslave(struct net_device *bond_dev, struct net_device *slave_dev,
> 		unblock_netpoll_tx();
> 	}
> 
>-	if (bond_mode_uses_xmit_hash(bond))
>+	if (bond_mode_can_use_xmit_hash(bond))
> 		bond_update_slave_arr(bond, NULL);
> 
> 	bond->nest_level = dev_get_nest_level(bond_dev);
>@@ -1870,7 +1870,7 @@ static int __bond_release_one(struct net_device *bond_dev,
> 	if (BOND_MODE(bond) == BOND_MODE_8023AD)
> 		bond_3ad_unbind_slave(slave);
> 
>-	if (bond_mode_uses_xmit_hash(bond))
>+	if (bond_mode_can_use_xmit_hash(bond))
> 		bond_update_slave_arr(bond, slave);
> 
> 	netdev_info(bond_dev, "Releasing %s interface %s\n",
>@@ -3102,7 +3102,7 @@ static int bond_slave_netdev_event(unsigned long event,
> 		 * events. If these (miimon/arpmon) parameters are configured
> 		 * then array gets refreshed twice and that should be fine!
> 		 */
>-		if (bond_mode_uses_xmit_hash(bond))
>+		if (bond_mode_can_use_xmit_hash(bond))
> 			bond_update_slave_arr(bond, NULL);
> 		break;
> 	case NETDEV_CHANGEMTU:
>@@ -3322,7 +3322,7 @@ static int bond_open(struct net_device *bond_dev)
> 		 */
> 		if (bond_alb_initialize(bond, (BOND_MODE(bond) == BOND_MODE_ALB)))
> 			return -ENOMEM;
>-		if (bond->params.tlb_dynamic_lb)
>+		if (bond->params.tlb_dynamic_lb || BOND_MODE(bond) == BOND_MODE_ALB)
> 			queue_delayed_work(bond->wq, &bond->alb_work, 0);
> 	}
> 
>@@ -3341,7 +3341,7 @@ static int bond_open(struct net_device *bond_dev)
> 		bond_3ad_initiate_agg_selection(bond, 1);
> 	}
> 
>-	if (bond_mode_uses_xmit_hash(bond))
>+	if (bond_mode_can_use_xmit_hash(bond))
> 		bond_update_slave_arr(bond, NULL);
> 
> 	return 0;
>@@ -3892,7 +3892,7 @@ static void bond_slave_arr_handler(struct work_struct *work)
>  * to determine the slave interface -
>  * (a) BOND_MODE_8023AD
>  * (b) BOND_MODE_XOR
>- * (c) BOND_MODE_TLB && tlb_dynamic_lb == 0
>+ * (c) (BOND_MODE_TLB || BOND_MODE_ALB) && tlb_dynamic_lb == 0
>  *
>  * The caller is expected to hold RTNL only and NO other lock!
>  */
>@@ -3945,6 +3945,11 @@ int bond_update_slave_arr(struct bonding *bond, struct slave *skipslave)
> 			continue;
> 		if (skipslave == slave)
> 			continue;
>+
>+		netdev_dbg(bond->dev,
>+			   "Adding slave dev %s to tx hash array[%d]\n",
>+			   slave->dev->name, new_arr->count);
>+
> 		new_arr->arr[new_arr->count++] = slave;
> 	}
> 
>@@ -4320,9 +4325,9 @@ static int bond_check_params(struct bond_params *params)
> 	}
> 
> 	if (xmit_hash_policy) {
>-		if ((bond_mode != BOND_MODE_XOR) &&
>-		    (bond_mode != BOND_MODE_8023AD) &&
>-		    (bond_mode != BOND_MODE_TLB)) {
>+		if (bond_mode == BOND_MODE_ROUNDROBIN ||
>+		    bond_mode == BOND_MODE_ACTIVEBACKUP ||
>+		    bond_mode == BOND_MODE_BROADCAST) {
> 			pr_info("xmit_hash_policy param is irrelevant in mode %s\n",
> 				bond_mode_name(bond_mode));
> 		} else {
>diff --git a/drivers/net/bonding/bond_options.c b/drivers/net/bonding/bond_options.c
>index 58c705f24f96..8a945c9341d6 100644
>--- a/drivers/net/bonding/bond_options.c
>+++ b/drivers/net/bonding/bond_options.c
>@@ -395,7 +395,7 @@ static const struct bond_option bond_opts[BOND_OPT_LAST] = {
> 		.id = BOND_OPT_TLB_DYNAMIC_LB,
> 		.name = "tlb_dynamic_lb",
> 		.desc = "Enable dynamic flow shuffling",
>-		.unsuppmodes = BOND_MODE_ALL_EX(BIT(BOND_MODE_TLB)),
>+		.unsuppmodes = BOND_MODE_ALL_EX(BIT(BOND_MODE_TLB) | BIT(BOND_MODE_ALB)),
> 		.values = bond_tlb_dynamic_lb_tbl,
> 		.flags = BOND_OPTFLAG_IFDOWN,
> 		.set = bond_option_tlb_dynamic_lb_set,
>diff --git a/include/net/bonding.h b/include/net/bonding.h
>index b52235158836..9a41a50b0bd2 100644
>--- a/include/net/bonding.h
>+++ b/include/net/bonding.h
>@@ -285,10 +285,18 @@ static inline bool bond_needs_speed_duplex(const struct bonding *bond)
> 
> static inline bool bond_is_nondyn_tlb(const struct bonding *bond)
> {
>-	return (BOND_MODE(bond) == BOND_MODE_TLB)  &&
>+	return (BOND_MODE(bond) == BOND_MODE_TLB || BOND_MODE(bond) == BOND_MODE_ALB) &&

	I believe this could use bond_is_lb(bond) instead.

	-J
	
> 	       (bond->params.tlb_dynamic_lb == 0);
> }
> 
>+static inline bool bond_mode_can_use_xmit_hash(const struct bonding *bond)
>+{
>+	return (BOND_MODE(bond) == BOND_MODE_8023AD ||
>+		BOND_MODE(bond) == BOND_MODE_XOR ||
>+		BOND_MODE(bond) == BOND_MODE_TLB ||
>+		BOND_MODE(bond) == BOND_MODE_ALB);
>+}
>+
> static inline bool bond_mode_uses_xmit_hash(const struct bonding *bond)
> {
> 	return (BOND_MODE(bond) == BOND_MODE_8023AD ||
>-- 
>2.17.0
>