From: Jiri Pirko <jiri@resnulli.us>
To: Maor Gottlieb <maorg@mellanox.com>
Cc: davem@davemloft.net, jgg@mellanox.com, dledford@redhat.com,
j.vosburgh@gmail.com, vfalico@gmail.com, andy@greyhouse.net,
kuba@kernel.org, leonro@mellanox.com, saeedm@mellanox.com,
jiri@mellanox.com, linux-rdma@vger.kernel.org,
netdev@vger.kernel.org, alexr@mellanox.com
Subject: Re: [PATCH V2 mlx5-next 03/10] bonding: Add helpers to get xmit slave
Date: Mon, 20 Apr 2020 16:27:26 +0200 [thread overview]
Message-ID: <20200420142726.GM6581@nanopsycho.orion> (raw)
In-Reply-To: <20200420075426.31462-4-maorg@mellanox.com>
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
>
next prev parent reply other threads:[~2020-04-20 14:27 UTC|newest]
Thread overview: 31+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-04-20 7:54 [PATCH V2 mlx5-next 00/10] Add support to get xmit slave Maor Gottlieb
2020-04-20 7:54 ` [PATCH V2 mlx5-next 01/10] net/core: Introduce master_xmit_slave_get Maor Gottlieb
2020-04-20 14:01 ` Jiri Pirko
2020-04-20 17:29 ` David Ahern
2020-04-20 17:41 ` Jiri Pirko
2020-04-20 17:43 ` David Ahern
2020-04-20 17:53 ` Jiri Pirko
2020-04-20 17:54 ` Jiri Pirko
2020-04-20 17:56 ` David Ahern
2020-04-20 18:01 ` Jiri Pirko
2020-04-20 18:04 ` David Ahern
2020-04-20 18:48 ` Jiri Pirko
2020-04-20 18:56 ` Maor Gottlieb
2020-04-20 19:02 ` David Ahern
2020-04-21 5:37 ` Jiri Pirko
2020-04-21 5:43 ` Maor Gottlieb
2020-04-20 7:54 ` [PATCH V2 mlx5-next 02/10] bonding: Rename slave_arr to usable_slaves Maor Gottlieb
2020-04-20 14:17 ` Jiri Pirko
2020-04-20 7:54 ` [PATCH V2 mlx5-next 03/10] bonding: Add helpers to get xmit slave Maor Gottlieb
2020-04-20 14:27 ` Jiri Pirko [this message]
2020-04-20 18:46 ` Jay Vosburgh
2020-04-20 18:59 ` Maor Gottlieb
2020-04-20 7:54 ` [PATCH V2 mlx5-next 04/10] bonding: Implement ndo_xmit_slave_get Maor Gottlieb
2020-04-20 15:04 ` Jiri Pirko
2020-04-20 15:36 ` David Ahern
2020-04-20 7:54 ` [PATCH V2 mlx5-next 05/10] RDMA/core: Add LAG functionality Maor Gottlieb
2020-04-20 7:54 ` [PATCH V2 mlx5-next 06/10] RDMA/core: Get xmit slave for LAG Maor Gottlieb
2020-04-20 7:54 ` [PATCH V2 mlx5-next 07/10] net/mlx5: Change lag mutex lock to spin lock Maor Gottlieb
2020-04-20 7:54 ` [PATCH V2 mlx5-next 08/10] net/mlx5: Add support to get lag physical port Maor Gottlieb
2020-04-20 7:54 ` [PATCH V2 mlx5-next 09/10] RDMA/mlx5: Refactor affinity related code Maor Gottlieb
2020-04-20 7:54 ` [PATCH V2 mlx5-next 10/10] RDMA/mlx5: Set lag tx affinity according to slave Maor Gottlieb
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20200420142726.GM6581@nanopsycho.orion \
--to=jiri@resnulli.us \
--cc=alexr@mellanox.com \
--cc=andy@greyhouse.net \
--cc=davem@davemloft.net \
--cc=dledford@redhat.com \
--cc=j.vosburgh@gmail.com \
--cc=jgg@mellanox.com \
--cc=jiri@mellanox.com \
--cc=kuba@kernel.org \
--cc=leonro@mellanox.com \
--cc=linux-rdma@vger.kernel.org \
--cc=maorg@mellanox.com \
--cc=netdev@vger.kernel.org \
--cc=saeedm@mellanox.com \
--cc=vfalico@gmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).