From mboxrd@z Thu Jan 1 00:00:00 1970 From: Chas Williams <3chas3@gmail.com> Subject: Re: [PATCH] net/bonding: fix slave tx burst for mode 4 Date: Mon, 11 Feb 2019 10:34:44 -0500 Message-ID: <6f80e7f3-6d6c-e2e6-861d-d57795c0289c@gmail.com> References: <1549872096-16116-1-git-send-email-wangyunjian@huawei.com> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 7bit Cc: chas3@att.com, xudingke@huawei.com, stable@dpdk.org To: wangyunjian , dev@dpdk.org Return-path: In-Reply-To: <1549872096-16116-1-git-send-email-wangyunjian@huawei.com> Content-Language: en-US List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" 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 > > 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 > Signed-off-by: Yunjian Wang > --- > 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]]; >