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

[wangyunjian <wangyunjian@huawei.com>]

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