Re: [PATCH v5 net-next 11/27] bonding: rework bond_3ad_xmit_xor() to use bond_for_each_slave() only

[Ding Tianhong <dingtianhong@huawei.com>]

On 2013/9/25 15:20, Veaceslav Falico wrote:
> Currently, there are two loops - first we find the first slave in an
> aggregator after the xmit_hash_policy() returned number, and after that we
> loop from that slave, over bonding head, and till that slave to find any
> suitable slave to send the packet through.
> 
> Replace it by just one bond_for_each_slave() loop, which first loops
> through the requested number of slaves, saving the first suitable one, and
> after that we've hit the requested number of slaves to skip - search for
> any up slave to send the packet through. If we don't find such kind of
> slave - then just send the packet through the first suitable slave found.
> 
> Logic remains unchainged, and we skip two loops. Also, refactor it a bit
> for readability.
> 
> CC: Jay Vosburgh <fubar@us.ibm.com>
> CC: Andy Gospodarek <andy@greyhouse.net>
> Signed-off-by: Veaceslav Falico <vfalico@redhat.com>
> ---
> 
> Notes:
>     v4  -> v5
>     No change.
>     
>     v3  -> v4:
>     No change.
>     
>     v2  -> v3:
>     No change.
>     
>     v1  -> v2:
>     No changes.
>     
>     RFC -> v1:
>     New patch.
> 
>  drivers/net/bonding/bond_3ad.c | 46 ++++++++++++++++++++----------------------
>  1 file changed, 22 insertions(+), 24 deletions(-)
> 
> diff --git a/drivers/net/bonding/bond_3ad.c b/drivers/net/bonding/bond_3ad.c
> index 3847aee..c861ee7 100644
> --- a/drivers/net/bonding/bond_3ad.c
> +++ b/drivers/net/bonding/bond_3ad.c
> @@ -2417,15 +2417,15 @@ int bond_3ad_get_active_agg_info(struct bonding *bond, struct ad_info *ad_info)
>  
>  int bond_3ad_xmit_xor(struct sk_buff *skb, struct net_device *dev)
>  {
> -	struct slave *slave, *start_at;
>  	struct bonding *bond = netdev_priv(dev);
> +	struct slave *slave, *first_ok_slave;
> +	struct aggregator *agg;
> +	struct ad_info ad_info;
>  	struct list_head *iter;
> -	int slave_agg_no;
>  	int slaves_in_agg;
> -	int agg_id;
> -	int i;
> -	struct ad_info ad_info;
> +	int slave_agg_no;
>  	int res = 1;
> +	int agg_id;
>  
>  	read_lock(&bond->lock);
>  	if (__bond_3ad_get_active_agg_info(bond, &ad_info)) {
> @@ -2438,20 +2438,28 @@ int bond_3ad_xmit_xor(struct sk_buff *skb, struct net_device *dev)
>  	agg_id = ad_info.aggregator_id;
>  
>  	if (slaves_in_agg == 0) {
> -		/*the aggregator is empty*/
>  		pr_debug("%s: Error: active aggregator is empty\n", dev->name);
>  		goto out;
>  	}
>  
>  	slave_agg_no = bond->xmit_hash_policy(skb, slaves_in_agg);
> +	first_ok_slave = NULL;
>  
>  	bond_for_each_slave(bond, slave, iter) {
> -		struct aggregator *agg = SLAVE_AD_INFO(slave).port.aggregator;
> +		agg = SLAVE_AD_INFO(slave).port.aggregator;
> +		if (!agg || agg->aggregator_identifier != agg_id)
> +			continue;
>  
> -		if (agg && (agg->aggregator_identifier == agg_id)) {
> +		if (slave_agg_no >= 0) {
> +			if (!first_ok_slave && SLAVE_IS_OK(slave))
> +				first_ok_slave = slave;
>  			slave_agg_no--;
> -			if (slave_agg_no < 0)
> -				break;
> +			continue;
> +		}
> +
> +		if (SLAVE_IS_OK(slave)) {
> +			res = bond_dev_queue_xmit(bond, skb, slave->dev);
> +			goto out;
>  		}

I think you miss something, you will send skb always by the first suitable port,
it could not support load balance.
pls consult my function.
 		if (agg && (agg->aggregator_identifier == agg_id)) {
-			slave_agg_no--;
-			if (slave_agg_no < 0)
-				break;
+			if (--slave_agg_no < 0) {
+				if (SLAVE_IS_OK(slave)) {
+					res = bond_dev_queue_xmit(bond,
+						skb, slave->dev);
+					goto out;
+				}
+			}
 		}
 	}

>  	}
>  
> @@ -2461,20 +2469,10 @@ int bond_3ad_xmit_xor(struct sk_buff *skb, struct net_device *dev)
>  		goto out;
>  	}
>  
> -	start_at = slave;
> -
> -	bond_for_each_slave_from(bond, slave, i, start_at) {
> -		int slave_agg_id = 0;
> -		struct aggregator *agg = SLAVE_AD_INFO(slave).port.aggregator;
> -
> -		if (agg)
> -			slave_agg_id = agg->aggregator_identifier;
> -
> -		if (SLAVE_IS_OK(slave) && agg && (slave_agg_id == agg_id)) {
> -			res = bond_dev_queue_xmit(bond, skb, slave->dev);
> -			break;
> -		}
> -	}
> +	/* we couldn't find any suitable slave after the agg_no, so use the
> +	 * first suitable found, if found. */
> +	if (first_ok_slave)
> +		res = bond_dev_queue_xmit(bond, skb, first_ok_slave->dev);
>  
>  out:
>  	read_unlock(&bond->lock);
>