Re: [PATCH V2 mlx5-next 03/10] bonding: Add helpers to get xmit slave

[Maor Gottlieb <maorg@xxxxxxxxxxxx>]

On 4/20/2020 9:46 PM, Jay Vosburgh wrote:
> Jiri Pirko <jiri@xxxxxxxxxxx> wrote:
>
> > Mon, Apr 20, 2020 at 09:54:19AM CEST, maorg@xxxxxxxxxxxx wrote:
> > > This helpers will be used by both the xmit function
> > > and the get xmit slave ndo.
> > Be more verbose about what you are doing please. From this I have no
> > clue what is going on.
> 	Agreed, and also with Jiri's comment further down to split this
> into multiple patches.  The current series is difficult to follow.
>
> 	-J

Agree, I am splitting this series to more patches so it will be easier 
to follow.
> > > Signed-off-by: Maor Gottlieb <maorg@xxxxxxxxxxxx>
> > > ---
> > > drivers/net/bonding/bond_alb.c  | 35 ++++++++----
> > > drivers/net/bonding/bond_main.c | 94 +++++++++++++++++++++------------
> > > include/net/bond_alb.h          |  4 ++
> > > 3 files changed, 89 insertions(+), 44 deletions(-)
> > >
> > > diff --git a/drivers/net/bonding/bond_alb.c b/drivers/net/bonding/bond_alb.c
> > > index 7bb49b049dcc..e863c694c309 100644
> > > --- a/drivers/net/bonding/bond_alb.c
> > > +++ b/drivers/net/bonding/bond_alb.c
> > > @@ -1334,11 +1334,11 @@ static netdev_tx_t bond_do_alb_xmit(struct sk_buff *skb, struct bonding *bond,
> > > 	return NETDEV_TX_OK;
> > > }
> > >
> > > -netdev_tx_t bond_tlb_xmit(struct sk_buff *skb, struct net_device *bond_dev)
> > > +struct slave *bond_xmit_tlb_slave_get(struct bonding *bond,
> > > +				      struct sk_buff *skb)
> > > {
> > > -	struct bonding *bond = netdev_priv(bond_dev);
> > > -	struct ethhdr *eth_data;
> > > 	struct slave *tx_slave = NULL;
> > > +	struct ethhdr *eth_data;
> > > 	u32 hash_index;
> > >
> > > 	skb_reset_mac_header(skb);
> > > @@ -1369,20 +1369,29 @@ netdev_tx_t bond_tlb_xmit(struct sk_buff *skb, struct net_device *bond_dev)
> > > 			break;
> > > 		}
> > > 	}
> > > -	return bond_do_alb_xmit(skb, bond, tx_slave);
> > > +	return tx_slave;
> > > }
> > >
> > > -netdev_tx_t bond_alb_xmit(struct sk_buff *skb, struct net_device *bond_dev)
> > > +netdev_tx_t bond_tlb_xmit(struct sk_buff *skb, struct net_device *bond_dev)
> > > {
> > > 	struct bonding *bond = netdev_priv(bond_dev);
> > > -	struct ethhdr *eth_data;
> > > +	struct slave *tx_slave;
> > > +
> > > +	tx_slave = bond_xmit_tlb_slave_get(bond, skb);
> > > +	return bond_do_alb_xmit(skb, bond, tx_slave);
> > > +}
> > > +
> > > +struct slave *bond_xmit_alb_slave_get(struct bonding *bond,
> > > +				      struct sk_buff *skb)
> > > +{
> > > 	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;
> > > +	struct slave *tx_slave = NULL;
> > > +	const u8 *hash_start = NULL;
> > > 	bool do_tx_balance = true;
> > > +	struct ethhdr *eth_data;
> > > 	u32 hash_index = 0;
> > > -	const u8 *hash_start = NULL;
> > > +	int hash_size = 0;
> > >
> > > 	skb_reset_mac_header(skb);
> > > 	eth_data = eth_hdr(skb);
> > > @@ -1501,7 +1510,15 @@ netdev_tx_t bond_alb_xmit(struct sk_buff *skb, struct net_device *bond_dev)
> > > 						       count];
> > > 		}
> > > 	}
> > > +	return tx_slave;
> > > +}
> > > +
> > > +netdev_tx_t bond_alb_xmit(struct sk_buff *skb, struct net_device *bond_dev)
> > > +{
> > > +	struct bonding *bond = netdev_priv(bond_dev);
> > > +	struct slave *tx_slave = NULL;
> > >
> > > +	tx_slave = bond_xmit_alb_slave_get(bond, skb);
> > > 	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 2cb41d480ae2..7e04be86fda8 100644
> > > --- a/drivers/net/bonding/bond_main.c
> > > +++ b/drivers/net/bonding/bond_main.c
> > > @@ -82,6 +82,7 @@
> > > #include <net/bonding.h>
> > > #include <net/bond_3ad.h>
> > > #include <net/bond_alb.h>
> > > +#include <net/lag.h>
> > >
> > > #include "bonding_priv.h"
> > >
> > > @@ -3406,10 +3407,26 @@ u32 bond_xmit_hash(struct bonding *bond, struct sk_buff *skb)
> > > 		(__force u32)flow_get_u32_src(&flow);
> > > 	hash ^= (hash >> 16);
> > > 	hash ^= (hash >> 8);
> > > -
> > Please avoid changes like this one.
> >
> >
> > > 	return hash >> 1;
> > > }
> > >
> > > +static struct slave *bond_xmit_3ad_xor_slave_get(struct bonding *bond,
> > > +						 struct sk_buff *skb,
> > > +						 struct bond_up_slave *slaves)
> > > +{
> > > +	struct slave *slave;
> > > +	unsigned int count;
> > > +	u32 hash;
> > > +
> > > +	hash = bond_xmit_hash(bond, skb);
> > > +	count = slaves ? READ_ONCE(slaves->count) : 0;
> > > +	if (unlikely(!count))
> > > +		return NULL;
> > > +
> > > +	slave = slaves->arr[hash % count];
> > > +	return slave;
> > > +}
> > Why don't you have this helper near bond_3ad_xor_xmit() as you have for
> > round robin for example?

No good reason, will move it near.
> > I think it would make this patch much easier to review if you split to
> > multiple patches, per-mode.
> >
> >
> > > +
> > > /*-------------------------- Device entry points ----------------------------*/
> > >
> > > void bond_work_init_all(struct bonding *bond)
> > > @@ -3923,16 +3940,15 @@ static int bond_set_mac_address(struct net_device *bond_dev, void *addr)
> > > }
> > >
> > > /**
> > > - * bond_xmit_slave_id - transmit skb through slave with slave_id
> > > + * bond_get_slave_by_id - get xmit slave with slave_id
> > >   * @bond: bonding device that is transmitting
> > > - * @skb: buffer to transmit
> > >   * @slave_id: slave id up to slave_cnt-1 through which to transmit
> > >   *
> > > - * This function tries to transmit through slave with slave_id but in case
> > > + * This function tries to get slave with slave_id but in case
> > >   * it fails, it tries to find the first available slave for transmission.
> > > - * The skb is consumed in all cases, thus the function is void.
> > >   */
> > > -static void bond_xmit_slave_id(struct bonding *bond, struct sk_buff *skb, int slave_id)
> > > +static struct slave *bond_get_slave_by_id(struct bonding *bond,
> > > +					  int slave_id)
> > > {
> > > 	struct list_head *iter;
> > > 	struct slave *slave;
> > > @@ -3941,10 +3957,8 @@ static void bond_xmit_slave_id(struct bonding *bond, struct sk_buff *skb, int sl
> > > 	/* Here we start from the slave with slave_id */
> > > 	bond_for_each_slave_rcu(bond, slave, iter) {
> > > 		if (--i < 0) {
> > > -			if (bond_slave_can_tx(slave)) {
> > > -				bond_dev_queue_xmit(bond, skb, slave->dev);
> > > -				return;
> > > -			}
> > > +			if (bond_slave_can_tx(slave))
> > > +				return slave;
> > > 		}
> > > 	}
> > >
> > > @@ -3953,13 +3967,11 @@ static void bond_xmit_slave_id(struct bonding *bond, struct sk_buff *skb, int sl
> > > 	bond_for_each_slave_rcu(bond, slave, iter) {
> > > 		if (--i < 0)
> > > 			break;
> > > -		if (bond_slave_can_tx(slave)) {
> > > -			bond_dev_queue_xmit(bond, skb, slave->dev);
> > > -			return;
> > > -		}
> > > +		if (bond_slave_can_tx(slave))
> > > +			return slave;
> > > 	}
> > > -	/* no slave that can tx has been found */
> > > -	bond_tx_drop(bond->dev, skb);
> > > +
> > > +	return NULL;
> > > }
> > >
> > > /**
> > > @@ -3995,10 +4007,9 @@ static u32 bond_rr_gen_slave_id(struct bonding *bond)
> > > 	return slave_id;
> > > }
> > >
> > > -static netdev_tx_t bond_xmit_roundrobin(struct sk_buff *skb,
> > > -					struct net_device *bond_dev)
> > > +static struct slave *bond_xmit_roundrobin_slave_get(struct bonding *bond,
> > > +						    struct sk_buff *skb)
> > > {
> > > -	struct bonding *bond = netdev_priv(bond_dev);
> > > 	struct slave *slave;
> > > 	int slave_cnt;
> > > 	u32 slave_id;
> > > @@ -4020,24 +4031,40 @@ static netdev_tx_t bond_xmit_roundrobin(struct sk_buff *skb,
> > > 		if (iph->protocol == IPPROTO_IGMP) {
> > > 			slave = rcu_dereference(bond->curr_active_slave);
> > > 			if (slave)
> > > -				bond_dev_queue_xmit(bond, skb, slave->dev);
> > > -			else
> > > -				bond_xmit_slave_id(bond, skb, 0);
> > > -			return NETDEV_TX_OK;
> > > +				return slave;
> > > +			return bond_get_slave_by_id(bond, 0);
> > > 		}
> > > 	}
> > >
> > > non_igmp:
> > > 	slave_cnt = READ_ONCE(bond->slave_cnt);
> > > 	if (likely(slave_cnt)) {
> > > -		slave_id = bond_rr_gen_slave_id(bond);
> > > -		bond_xmit_slave_id(bond, skb, slave_id % slave_cnt);
> > > -	} else {
> > > -		bond_tx_drop(bond_dev, skb);
> > > +		slave_id = bond_rr_gen_slave_id(bond) % slave_cnt;
> > > +		return bond_get_slave_by_id(bond, slave_id);
> > > 	}
> > > +	return NULL;
> > > +}
> > > +
> > > +static netdev_tx_t bond_xmit_roundrobin(struct sk_buff *skb,
> > > +					struct net_device *bond_dev)
> > > +{
> > > +	struct bonding *bond = netdev_priv(bond_dev);
> > > +	struct slave *slave;
> > > +
> > > +	slave = bond_xmit_roundrobin_slave_get(bond, skb);
> > > +	if (slave)
> > > +		bond_dev_queue_xmit(bond, skb, slave->dev);
> > > +	else
> > > +		bond_tx_drop(bond_dev, skb);
> > > 	return NETDEV_TX_OK;
> > > }
> > >
> > > +static struct slave *bond_xmit_activebackup_slave_get(struct bonding *bond,
> > > +						      struct sk_buff *skb)
> > > +{
> > > +	return rcu_dereference(bond->curr_active_slave);
> > > +}
> > > +
> > > /* In active-backup mode, we know that bond->curr_active_slave is always valid if
> > >   * the bond has a usable interface.
> > >   */
> > > @@ -4047,7 +4074,7 @@ static netdev_tx_t bond_xmit_activebackup(struct sk_buff *skb,
> > > 	struct bonding *bond = netdev_priv(bond_dev);
> > > 	struct slave *slave;
> > >
> > > -	slave = rcu_dereference(bond->curr_active_slave);
> > > +	slave = bond_xmit_activebackup_slave_get(bond, skb);
> > > 	if (slave)
> > > 		bond_dev_queue_xmit(bond, skb, slave->dev);
> > > 	else
> > > @@ -4193,18 +4220,15 @@ static netdev_tx_t bond_3ad_xor_xmit(struct sk_buff *skb,
> > > 				     struct net_device *dev)
> > > {
> > > 	struct bonding *bond = netdev_priv(dev);
> > > -	struct slave *slave;
> > > 	struct bond_up_slave *slaves;
> > > -	unsigned int count;
> > > +	struct slave *slave;
> > >
> > > 	slaves = rcu_dereference(bond->usable_slaves);
> > > -	count = slaves ? READ_ONCE(slaves->count) : 0;
> > > -	if (likely(count)) {
> > > -		slave = slaves->arr[bond_xmit_hash(bond, skb) % count];
> > > +	slave = bond_xmit_3ad_xor_slave_get(bond, skb, slaves);
> > > +	if (likely(slave))
> > > 		bond_dev_queue_xmit(bond, skb, slave->dev);
> > > -	} else {
> > > +	else
> > > 		bond_tx_drop(dev, skb);
> > > -	}
> > >
> > > 	return NETDEV_TX_OK;
> > > }
> > > diff --git a/include/net/bond_alb.h b/include/net/bond_alb.h
> > > index b3504fcd773d..f6af76c87a6c 100644
> > > --- a/include/net/bond_alb.h
> > > +++ b/include/net/bond_alb.h
> > > @@ -158,6 +158,10 @@ void bond_alb_handle_link_change(struct bonding *bond, struct slave *slave, char
> > > void bond_alb_handle_active_change(struct bonding *bond, struct slave *new_slave);
> > > int bond_alb_xmit(struct sk_buff *skb, struct net_device *bond_dev);
> > > int bond_tlb_xmit(struct sk_buff *skb, struct net_device *bond_dev);
> > > +struct slave *bond_xmit_alb_slave_get(struct bonding *bond,
> > > +				      struct sk_buff *skb);
> > > +struct slave *bond_xmit_tlb_slave_get(struct bonding *bond,
> > > +				      struct sk_buff *skb);
> > > void bond_alb_monitor(struct work_struct *);
> > > 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);
> > > --
> > > 2.17.2
> ---
> 	-Jay Vosburgh, jay.vosburgh@xxxxxxxxxxxxx