| Message ID | 20200420075426.31462-4-maorg@mellanox.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Add support to get xmit slave | expand |
Mon, Apr 20, 2020 at 09:54:19AM CEST, maorg@mellanox.com 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. > >Signed-off-by: Maor Gottlieb <maorg@mellanox.com> >--- > 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? 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 >
Jiri Pirko <jiri@resnulli.us> wrote: >Mon, Apr 20, 2020 at 09:54:19AM CEST, maorg@mellanox.com 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 > >> >>Signed-off-by: Maor Gottlieb <maorg@mellanox.com> >>--- >> 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? > >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@canonical.com
On 4/20/2020 9:46 PM, Jay Vosburgh wrote: > Jiri Pirko <jiri@resnulli.us> wrote: > >> Mon, Apr 20, 2020 at 09:54:19AM CEST, maorg@mellanox.com 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@mellanox.com> >>> --- >>> 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@canonical.com
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); - 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; +} + /*-------------------------- 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);
This helpers will be used by both the xmit function and the get xmit slave ndo. Signed-off-by: Maor Gottlieb <maorg@mellanox.com> --- 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(-)