net/bonding: fix slave tx burst for mode 4

  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(-)
  

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