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

login
register
mail settings
Submitter Chas Williams
Date Feb. 11, 2019, 3:34 p.m.
Message ID <6f80e7f3-6d6c-e2e6-861d-d57795c0289c@gmail.com>
Download mbox | patch
Permalink /patch/723059/
State New
Headers show

Comments

Chas Williams - Feb. 11, 2019, 3:34 p.m.
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:

  	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]];
>
Yunjian Wang - Feb. 12, 2019, 2:04 a.m.
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]];

> >

Patch

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 */