[dpdk-dev] [PATCH] net/bonding: fix slave tx burst for mode 4

[dpdk-dev] [PATCH] net/bonding: fix slave tx burst for mode 4

[wangyunjian]

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

Re: [dpdk-dev] [PATCH] net/bonding: fix slave tx burst for mode 4

[Chas Williams]

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

Re: [dpdk-dev] [PATCH] net/bonding: fix slave tx burst for mode 4

[wangyunjian]

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

[dpdk-dev] [PATCH v2] net/bonding: fix slave tx burst for mode 4

[Chas Williams]

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

Re: [dpdk-dev] [dpdk-stable] [PATCH v2] net/bonding: fix slave tx burst for mode 4

[Ferruh Yigit]

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.