net/bonding: fix slave tx burst for mode 4
Checks
Commit Message
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(-)
Comments
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]];
>
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]];
> >
@@ -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]];