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
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]];
> >
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
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.