* [dpdk-dev] [PATCH] net/bonding: fix slave tx burst for mode 4
@ 2019-02-11 8:01 wangyunjian
2019-02-11 15:34 ` Chas Williams
0 siblings, 1 reply; 5+ messages in thread
From: wangyunjian @ 2019-02-11 8:01 UTC (permalink / raw)
To: dev; +Cc: chas3, xudingke, Yunjian Wang, stable
From: Yunjian Wang <wangyunjian@huawei.com>
Now sending 0 packet it doesn't consider for LACPDUs,
but the LACPDUs should be checked and sended.
Fixes: 09150784a776 ("net/bonding: burst mode hash calculation")
Cc: stable@dpdk.org
Reported-by: Hui Zhao <zhaohui8@huawei.com>
Signed-off-by: Yunjian Wang <wangyunjian@huawei.com>
---
drivers/net/bonding/rte_eth_bond_pmd.c | 7 ++++---
1 file changed, 4 insertions(+), 3 deletions(-)
diff --git a/drivers/net/bonding/rte_eth_bond_pmd.c b/drivers/net/bonding/rte_eth_bond_pmd.c
index 44deaf1..a77af19 100644
--- a/drivers/net/bonding/rte_eth_bond_pmd.c
+++ b/drivers/net/bonding/rte_eth_bond_pmd.c
@@ -1298,9 +1298,6 @@ struct bwg_slave {
uint16_t i;
- if (unlikely(nb_bufs == 0))
- return 0;
-
/* Copy slave list to protect against slave up/down changes during tx
* bursting */
slave_count = internals->active_slave_count;
@@ -1310,6 +1307,9 @@ struct bwg_slave {
memcpy(slave_port_ids, internals->active_slaves,
sizeof(slave_port_ids[0]) * slave_count);
+ if (unlikely(nb_bufs == 0))
+ goto lacp_send;
+
dist_slave_count = 0;
for (i = 0; i < slave_count; i++) {
struct port *port = &bond_mode_8023ad_ports[slave_port_ids[i]];
@@ -1365,6 +1365,7 @@ struct bwg_slave {
}
}
+lacp_send:
/* Check for LACP control packets and send if available */
for (i = 0; i < slave_count; i++) {
struct port *port = &bond_mode_8023ad_ports[slave_port_ids[i]];
--
1.8.3.1
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [dpdk-dev] [PATCH] net/bonding: fix slave tx burst for mode 4
2019-02-11 8:01 [dpdk-dev] [PATCH] net/bonding: fix slave tx burst for mode 4 wangyunjian
@ 2019-02-11 15:34 ` Chas Williams
2019-02-12 2:04 ` wangyunjian
0 siblings, 1 reply; 5+ messages in thread
From: Chas Williams @ 2019-02-11 15:34 UTC (permalink / raw)
To: wangyunjian, dev; +Cc: chas3, xudingke, stable
How strange. I was just looking at this issue this weekend. I think
we can just move the control packet handling before the data packet
handling. That avoids the goto and is a little more sensible -- we should
try to prioritize control traffic (even if we can't guarantee there will
be space it's better than nothing). Something like this:
diff --git a/drivers/net/bonding/rte_eth_bond_pmd.c
b/drivers/net/bonding/rte_eth_bond_pmd.c
index 44deaf119..89ad67266 100644
--- a/drivers/net/bonding/rte_eth_bond_pmd.c
+++ b/drivers/net/bonding/rte_eth_bond_pmd.c
@@ -1298,9 +1298,6 @@ bond_ethdev_tx_burst_8023ad(void *queue, struct
rte_mbuf **bufs,
uint16_t i;
- if (unlikely(nb_bufs == 0))
- return 0;
-
/* Copy slave list to protect against slave up/down changes during tx
* bursting */
slave_count = internals->active_slave_count;
@@ -1310,6 +1307,30 @@ bond_ethdev_tx_burst_8023ad(void *queue, struct
rte_mbuf **bufs,
memcpy(slave_port_ids, internals->active_slaves,
sizeof(slave_port_ids[0]) * slave_count);
+ /* Check for LACP control packets and send if available */
+ for (i = 0; i < slave_count; i++) {
+ struct port *port = &bond_mode_8023ad_ports[slave_port_ids[i]];
+ struct rte_mbuf *ctrl_pkt = NULL;
+
+ if (likely(rte_ring_empty(port->tx_ring)))
+ continue;
+
+ if (rte_ring_dequeue(port->tx_ring,
+ (void **)&ctrl_pkt) != -ENOENT) {
+ slave_tx_count = rte_eth_tx_burst(slave_port_ids[i],
+ bd_tx_q->queue_id, &ctrl_pkt, 1);
+ /*
+ * re-enqueue LAG control plane packets to buffering
+ * ring if transmission fails so the packet isn't lost.
+ */
+ if (slave_tx_count != 1)
+ rte_ring_enqueue(port->tx_ring, ctrl_pkt);
+ }
+ }
+
+ if (unlikely(nb_bufs == 0))
+ return 0;
+
dist_slave_count = 0;
for (i = 0; i < slave_count; i++) {
struct port *port = &bond_mode_8023ad_ports[slave_port_ids[i]];
@@ -1365,27 +1386,6 @@ bond_ethdev_tx_burst_8023ad(void *queue, struct
rte_mbuf **bufs,
}
}
- /* Check for LACP control packets and send if available */
- for (i = 0; i < slave_count; i++) {
- struct port *port = &bond_mode_8023ad_ports[slave_port_ids[i]];
- struct rte_mbuf *ctrl_pkt = NULL;
-
- if (likely(rte_ring_empty(port->tx_ring)))
- continue;
-
- if (rte_ring_dequeue(port->tx_ring,
- (void **)&ctrl_pkt) != -ENOENT) {
- slave_tx_count = rte_eth_tx_burst(slave_port_ids[i],
- bd_tx_q->queue_id, &ctrl_pkt, 1);
- /*
- * re-enqueue LAG control plane packets to buffering
- * ring if transmission fails so the packet isn't lost.
- */
- if (slave_tx_count != 1)
- rte_ring_enqueue(port->tx_ring, ctrl_pkt);
- }
- }
-
return total_tx_count;
}
On 2/11/19 3:01 AM, wangyunjian wrote:
> From: Yunjian Wang <wangyunjian@huawei.com>
>
> Now sending 0 packet it doesn't consider for LACPDUs,
> but the LACPDUs should be checked and sended.
>
> Fixes: 09150784a776 ("net/bonding: burst mode hash calculation")
> Cc: stable@dpdk.org
>
> Reported-by: Hui Zhao <zhaohui8@huawei.com>
> Signed-off-by: Yunjian Wang <wangyunjian@huawei.com>
> ---
> drivers/net/bonding/rte_eth_bond_pmd.c | 7 ++++---
> 1 file changed, 4 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/net/bonding/rte_eth_bond_pmd.c b/drivers/net/bonding/rte_eth_bond_pmd.c
> index 44deaf1..a77af19 100644
> --- a/drivers/net/bonding/rte_eth_bond_pmd.c
> +++ b/drivers/net/bonding/rte_eth_bond_pmd.c
> @@ -1298,9 +1298,6 @@ struct bwg_slave {
>
> uint16_t i;
>
> - if (unlikely(nb_bufs == 0))
> - return 0;
> -
> /* Copy slave list to protect against slave up/down changes during tx
> * bursting */
> slave_count = internals->active_slave_count;
> @@ -1310,6 +1307,9 @@ struct bwg_slave {
> memcpy(slave_port_ids, internals->active_slaves,
> sizeof(slave_port_ids[0]) * slave_count);
>
> + if (unlikely(nb_bufs == 0))
> + goto lacp_send;
> +
> dist_slave_count = 0;
> for (i = 0; i < slave_count; i++) {
> struct port *port = &bond_mode_8023ad_ports[slave_port_ids[i]];
> @@ -1365,6 +1365,7 @@ struct bwg_slave {
> }
> }
>
> +lacp_send:
> /* Check for LACP control packets and send if available */
> for (i = 0; i < slave_count; i++) {
> struct port *port = &bond_mode_8023ad_ports[slave_port_ids[i]];
>
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [dpdk-dev] [PATCH] net/bonding: fix slave tx burst for mode 4
2019-02-11 15:34 ` Chas Williams
@ 2019-02-12 2:04 ` wangyunjian
2019-02-14 19:09 ` [dpdk-dev] [PATCH v2] " Chas Williams
0 siblings, 1 reply; 5+ messages in thread
From: wangyunjian @ 2019-02-12 2:04 UTC (permalink / raw)
To: Chas Williams, dev; +Cc: chas3, xudingke, stable, Zhaohui (zhaohui, Polestar)
I agree with you. Can you fix it?
Thanks
Yunjian
> -----Original Message-----
> From: Chas Williams [mailto:3chas3@gmail.com]
> Sent: Monday, February 11, 2019 11:35 PM
> To: wangyunjian <wangyunjian@huawei.com>; dev@dpdk.org
> Cc: chas3@att.com; xudingke <xudingke@huawei.com>; stable@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH] net/bonding: fix slave tx burst for mode 4
>
> How strange. I was just looking at this issue this weekend. I think we can just
> move the control packet handling before the data packet handling. That
> avoids the goto and is a little more sensible -- we should try to prioritize
> control traffic (even if we can't guarantee there will be space it's better than
> nothing). Something like this:
>
> diff --git a/drivers/net/bonding/rte_eth_bond_pmd.c
> b/drivers/net/bonding/rte_eth_bond_pmd.c
> index 44deaf119..89ad67266 100644
> --- a/drivers/net/bonding/rte_eth_bond_pmd.c
> +++ b/drivers/net/bonding/rte_eth_bond_pmd.c
> @@ -1298,9 +1298,6 @@ bond_ethdev_tx_burst_8023ad(void *queue,
> struct rte_mbuf **bufs,
>
> uint16_t i;
>
> - if (unlikely(nb_bufs == 0))
> - return 0;
> -
> /* Copy slave list to protect against slave up/down changes during tx
> * bursting */
> slave_count = internals->active_slave_count; @@ -1310,6 +1307,30
> @@ bond_ethdev_tx_burst_8023ad(void *queue, struct rte_mbuf **bufs,
> memcpy(slave_port_ids, internals->active_slaves,
> sizeof(slave_port_ids[0]) * slave_count);
>
> + /* Check for LACP control packets and send if available */
> + for (i = 0; i < slave_count; i++) {
> + struct port *port =
> &bond_mode_8023ad_ports[slave_port_ids[i]];
> + struct rte_mbuf *ctrl_pkt = NULL;
> +
> + if (likely(rte_ring_empty(port->tx_ring)))
> + continue;
> +
> + if (rte_ring_dequeue(port->tx_ring,
> + (void **)&ctrl_pkt) != -ENOENT) {
> + slave_tx_count = rte_eth_tx_burst(slave_port_ids[i],
> + bd_tx_q->queue_id, &ctrl_pkt, 1);
> + /*
> + * re-enqueue LAG control plane packets to buffering
> + * ring if transmission fails so the packet isn't lost.
> + */
> + if (slave_tx_count != 1)
> + rte_ring_enqueue(port->tx_ring,
> ctrl_pkt);
> + }
> + }
> +
> + if (unlikely(nb_bufs == 0))
> + return 0;
> +
> dist_slave_count = 0;
> for (i = 0; i < slave_count; i++) {
> struct port *port =
> &bond_mode_8023ad_ports[slave_port_ids[i]];
> @@ -1365,27 +1386,6 @@ bond_ethdev_tx_burst_8023ad(void *queue,
> struct rte_mbuf **bufs,
> }
> }
>
> - /* Check for LACP control packets and send if available */
> - for (i = 0; i < slave_count; i++) {
> - struct port *port =
> &bond_mode_8023ad_ports[slave_port_ids[i]];
> - struct rte_mbuf *ctrl_pkt = NULL;
> -
> - if (likely(rte_ring_empty(port->tx_ring)))
> - continue;
> -
> - if (rte_ring_dequeue(port->tx_ring,
> - (void **)&ctrl_pkt) != -ENOENT) {
> - slave_tx_count = rte_eth_tx_burst(slave_port_ids[i],
> - bd_tx_q->queue_id, &ctrl_pkt, 1);
> - /*
> - * re-enqueue LAG control plane packets to buffering
> - * ring if transmission fails so the packet isn't lost.
> - */
> - if (slave_tx_count != 1)
> - rte_ring_enqueue(port->tx_ring,
> ctrl_pkt);
> - }
> - }
> -
> return total_tx_count;
> }
>
>
> On 2/11/19 3:01 AM, wangyunjian wrote:
> > From: Yunjian Wang <wangyunjian@huawei.com>
> >
> > Now sending 0 packet it doesn't consider for LACPDUs, but the LACPDUs
> > should be checked and sended.
> >
> > Fixes: 09150784a776 ("net/bonding: burst mode hash calculation")
> > Cc: stable@dpdk.org
> >
> > Reported-by: Hui Zhao <zhaohui8@huawei.com>
> > Signed-off-by: Yunjian Wang <wangyunjian@huawei.com>
> > ---
> > drivers/net/bonding/rte_eth_bond_pmd.c | 7 ++++---
> > 1 file changed, 4 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/net/bonding/rte_eth_bond_pmd.c
> > b/drivers/net/bonding/rte_eth_bond_pmd.c
> > index 44deaf1..a77af19 100644
> > --- a/drivers/net/bonding/rte_eth_bond_pmd.c
> > +++ b/drivers/net/bonding/rte_eth_bond_pmd.c
> > @@ -1298,9 +1298,6 @@ struct bwg_slave {
> >
> > uint16_t i;
> >
> > - if (unlikely(nb_bufs == 0))
> > - return 0;
> > -
> > /* Copy slave list to protect against slave up/down changes during tx
> > * bursting */
> > slave_count = internals->active_slave_count; @@ -1310,6 +1307,9
> @@
> > struct bwg_slave {
> > memcpy(slave_port_ids, internals->active_slaves,
> > sizeof(slave_port_ids[0]) * slave_count);
> >
> > + if (unlikely(nb_bufs == 0))
> > + goto lacp_send;
> > +
> > dist_slave_count = 0;
> > for (i = 0; i < slave_count; i++) {
> > struct port *port =
> &bond_mode_8023ad_ports[slave_port_ids[i]];
> > @@ -1365,6 +1365,7 @@ struct bwg_slave {
> > }
> > }
> >
> > +lacp_send:
> > /* Check for LACP control packets and send if available */
> > for (i = 0; i < slave_count; i++) {
> > struct port *port =
> &bond_mode_8023ad_ports[slave_port_ids[i]];
> >
^ permalink raw reply [flat|nested] 5+ messages in thread
* [dpdk-dev] [PATCH v2] net/bonding: fix slave tx burst for mode 4
2019-02-12 2:04 ` wangyunjian
@ 2019-02-14 19:09 ` Chas Williams
2019-02-18 15:52 ` [dpdk-dev] [dpdk-stable] " Ferruh Yigit
0 siblings, 1 reply; 5+ messages in thread
From: Chas Williams @ 2019-02-14 19:09 UTC (permalink / raw)
To: dev; +Cc: Chas Williams, stable, Yunjian Wang
From: Chas Williams <chas3@att.com>
The tx burst routine always needs to check for pending LACPDUs
and send them if available. Do this first to prioritize the
control traffic. We can still early exit, before calculating
the distribution slaves, if there isn't any data packets.
Fixes: 09150784a776 ("net/bonding: burst mode hash calculation")
Cc: stable@dpdk.org
Reported-by: Hui Zhao <zhaohui8@huawei.com>
Cc: Yunjian Wang <wangyunjian@huawei.com>
Signed-off-by: Chas Williams <chas3@att.com>
---
drivers/net/bonding/rte_eth_bond_pmd.c | 48 +++++++++++++-------------
1 file changed, 24 insertions(+), 24 deletions(-)
diff --git a/drivers/net/bonding/rte_eth_bond_pmd.c b/drivers/net/bonding/rte_eth_bond_pmd.c
index 2304172d0..61e731a8f 100644
--- a/drivers/net/bonding/rte_eth_bond_pmd.c
+++ b/drivers/net/bonding/rte_eth_bond_pmd.c
@@ -1298,9 +1298,6 @@ bond_ethdev_tx_burst_8023ad(void *queue, struct rte_mbuf **bufs,
uint16_t i;
- if (unlikely(nb_bufs == 0))
- return 0;
-
/* Copy slave list to protect against slave up/down changes during tx
* bursting */
slave_count = internals->active_slave_count;
@@ -1310,6 +1307,30 @@ bond_ethdev_tx_burst_8023ad(void *queue, struct rte_mbuf **bufs,
memcpy(slave_port_ids, internals->active_slaves,
sizeof(slave_port_ids[0]) * slave_count);
+ /* Check for LACP control packets and send if available */
+ for (i = 0; i < slave_count; i++) {
+ struct port *port = &bond_mode_8023ad_ports[slave_port_ids[i]];
+ struct rte_mbuf *ctrl_pkt = NULL;
+
+ if (likely(rte_ring_empty(port->tx_ring)))
+ continue;
+
+ if (rte_ring_dequeue(port->tx_ring,
+ (void **)&ctrl_pkt) != -ENOENT) {
+ slave_tx_count = rte_eth_tx_burst(slave_port_ids[i],
+ bd_tx_q->queue_id, &ctrl_pkt, 1);
+ /*
+ * re-enqueue LAG control plane packets to buffering
+ * ring if transmission fails so the packet isn't lost.
+ */
+ if (slave_tx_count != 1)
+ rte_ring_enqueue(port->tx_ring, ctrl_pkt);
+ }
+ }
+
+ if (unlikely(nb_bufs == 0))
+ return 0;
+
dist_slave_count = 0;
for (i = 0; i < slave_count; i++) {
struct port *port = &bond_mode_8023ad_ports[slave_port_ids[i]];
@@ -1365,27 +1386,6 @@ bond_ethdev_tx_burst_8023ad(void *queue, struct rte_mbuf **bufs,
}
}
- /* Check for LACP control packets and send if available */
- for (i = 0; i < slave_count; i++) {
- struct port *port = &bond_mode_8023ad_ports[slave_port_ids[i]];
- struct rte_mbuf *ctrl_pkt = NULL;
-
- if (likely(rte_ring_empty(port->tx_ring)))
- continue;
-
- if (rte_ring_dequeue(port->tx_ring,
- (void **)&ctrl_pkt) != -ENOENT) {
- slave_tx_count = rte_eth_tx_burst(slave_port_ids[i],
- bd_tx_q->queue_id, &ctrl_pkt, 1);
- /*
- * re-enqueue LAG control plane packets to buffering
- * ring if transmission fails so the packet isn't lost.
- */
- if (slave_tx_count != 1)
- rte_ring_enqueue(port->tx_ring, ctrl_pkt);
- }
- }
-
return total_tx_count;
}
--
2.17.2
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [dpdk-dev] [dpdk-stable] [PATCH v2] net/bonding: fix slave tx burst for mode 4
2019-02-14 19:09 ` [dpdk-dev] [PATCH v2] " Chas Williams
@ 2019-02-18 15:52 ` Ferruh Yigit
0 siblings, 0 replies; 5+ messages in thread
From: Ferruh Yigit @ 2019-02-18 15:52 UTC (permalink / raw)
To: Chas Williams, dev; +Cc: Chas Williams, stable, Yunjian Wang
On 2/14/2019 7:09 PM, Chas Williams wrote:
> From: Chas Williams <chas3@att.com>
>
> The tx burst routine always needs to check for pending LACPDUs
> and send them if available. Do this first to prioritize the
> control traffic. We can still early exit, before calculating
> the distribution slaves, if there isn't any data packets.
>
> Fixes: 09150784a776 ("net/bonding: burst mode hash calculation")
> Cc: stable@dpdk.org
> Reported-by: Hui Zhao <zhaohui8@huawei.com>
> Cc: Yunjian Wang <wangyunjian@huawei.com>
> Signed-off-by: Chas Williams <chas3@att.com>
Applied to dpdk-next-net/master, thanks.
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2019-02-18 15:52 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-02-11 8:01 [dpdk-dev] [PATCH] net/bonding: fix slave tx burst for mode 4 wangyunjian
2019-02-11 15:34 ` Chas Williams
2019-02-12 2:04 ` wangyunjian
2019-02-14 19:09 ` [dpdk-dev] [PATCH v2] " Chas Williams
2019-02-18 15:52 ` [dpdk-dev] [dpdk-stable] " Ferruh Yigit
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).