Re: [PATCH V2 mlx5-next 04/10] bonding: Implement ndo_xmit_slave_get

David Ahern
2020-04-20T15:36:47Z

On 4/20/20 1:54 AM, Maor Gottlieb wrote:
> Add implementation of ndo_xmit_slave_get.
> When user sets the LAG_FLAGS_HASH_ALL_SLAVES bit and the xmit slave
> result is based on the hash, then the slave will be selected from the
> array of all the slaves.
>
> Signed-off-by: Maor Gottlieb <maorg@mellanox.com>
> ---
> drivers/net/bonding/bond_main.c | 123 +++++++++++++++++++++++++++-----
> include/net/bonding.h | 1 +
> 2 files changed, 105 insertions(+), 19 deletions(-)
>
> diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
> index 7e04be86fda8..320bcb1394fd 100644
> --- a/drivers/net/bonding/bond_main.c
> +++ b/drivers/net/bonding/bond_main.c
> @@ -4137,6 +4137,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
> @@ -4147,7 +4181,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;
> @@ -4159,7 +4193,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;
> }
> @@ -4168,20 +4204,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;
>
> @@ -4191,8 +4226,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);
> @@ -4200,14 +4233,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;
> }

none of the above code has anything to do directly with looking up the
bond slave in bond_xmit_get_slave; all of that and the bond_uninit
changes should be done in refactoring patch(es) that prepare existing
code to be called from the new ndo.