[dpdk-dev] [PATCH 3/7] vmxnet3: add support for mulit-segment transmit

[Yong Wang]

On 12/16/14, 9:13 PM, "Stephen Hemminger" <stephen at networkplumber.org>
wrote:

>From: Stephen Hemminger <shemming at brocade.com>
>
>Change sending loop to support multi-segment mbufs.
>The VMXNET3 api has start-of-packet and end-packet flags, so it
>is not hard to send multi-segment mbuf's.
>
>Also, update descriptor in 32 bit value rather than toggling
>bitfields which is slower and error prone.
>Based on code in earlier driver, and the Linux kernel driver.
>
>Add a compiler barrier to make sure that update of earlier descriptor
>are completed prior to update of generation bit on start of packet.
>
>Signed-off-by: Stephen Hemminger <stephen at networkplumber.org>
>Signed-off-by: Bill Hong <bhong at brocade.com>
>---
> lib/librte_pmd_vmxnet3/vmxnet3_rxtx.c | 126
>+++++++++++++---------------------
> 1 file changed, 48 insertions(+), 78 deletions(-)
>
>diff --git a/lib/librte_pmd_vmxnet3/vmxnet3_rxtx.c
>b/lib/librte_pmd_vmxnet3/vmxnet3_rxtx.c
>index 8e15784..7cb0b93 100644
>--- a/lib/librte_pmd_vmxnet3/vmxnet3_rxtx.c
>+++ b/lib/librte_pmd_vmxnet3/vmxnet3_rxtx.c
>@@ -312,13 +312,9 @@ vmxnet3_tq_tx_complete(vmxnet3_tx_queue_t *txq)
> 		VMXNET3_ASSERT(txq->cmd_ring.base[tcd->txdIdx].txd.eop == 1);
> #endif
> 		mbuf = txq->cmd_ring.buf_info[tcd->txdIdx].m;
>-		if (unlikely(mbuf == NULL))
>-			rte_panic("EOP desc does not point to a valid mbuf");
>-		else
>-			rte_pktmbuf_free(mbuf);
>-
>-
>+		rte_pktmbuf_free_seg(mbuf);
> 		txq->cmd_ring.buf_info[tcd->txdIdx].m = NULL;
>+
> 		/* Mark the txd for which tcd was generated as completed */
> 		vmxnet3_cmd_ring_adv_next2comp(&txq->cmd_ring);
> 
>@@ -336,13 +332,8 @@ vmxnet3_xmit_pkts(void *tx_queue, struct rte_mbuf
>**tx_pkts,
> 		  uint16_t nb_pkts)
> {
> 	uint16_t nb_tx;
>-	Vmxnet3_TxDesc *txd = NULL;
>-	vmxnet3_buf_info_t *tbi = NULL;
>-	struct vmxnet3_hw *hw;
>-	struct rte_mbuf *txm;
> 	vmxnet3_tx_queue_t *txq = tx_queue;
>-
>-	hw = txq->hw;
>+	struct vmxnet3_hw *hw = txq->hw;
> 
> 	if (unlikely(txq->stopped)) {
> 		PMD_TX_LOG(DEBUG, "Tx queue is stopped.");
>@@ -354,75 +345,60 @@ vmxnet3_xmit_pkts(void *tx_queue, struct rte_mbuf
>**tx_pkts,
> 
> 	nb_tx  = 0;
> 	while (nb_tx < nb_pkts) {
>+		Vmxnet3_GenericDesc *gdesc;
>+		vmxnet3_buf_info_t *tbi;
>+		uint32_t first2fill, avail, dw2;
>+		struct rte_mbuf *txm = tx_pkts[nb_tx];
>+		struct rte_mbuf *m_seg = txm;
>+
>+		/* Is command ring full? */
>+		avail = vmxnet3_cmd_ring_desc_avail(&txq->cmd_ring);
>+		if (txm->nb_segs > avail) {
>+			++txq->stats.tx_ring_full;
>+			break;
>+		}

You need to check for VMXNET3_MAX_TXD_PER_PKT as this is the max # of tx
descriptors
allowed for a non-tso pkt.

Food for thoughts: if avail is more than 0, it might be better to continue
if the next pkt could fit into the remaining descriptors, rather than break
here so all remaining pkts will be freed.

> 
>-		if (vmxnet3_cmd_ring_desc_avail(&txq->cmd_ring)) {
>-			int copy_size = 0;
>-
>-			txm = tx_pkts[nb_tx];
>-			/* Don't support scatter packets yet, free them if met */
>-			if (txm->nb_segs != 1) {
>-				PMD_TX_LOG(DEBUG, "Don't support scatter packets yet, drop!");
>-				rte_pktmbuf_free(tx_pkts[nb_tx]);
>-				txq->stats.drop_total++;
>-
>-				nb_tx++;
>-				continue;
>-			}
>-
>-			txd = (Vmxnet3_TxDesc *)(txq->cmd_ring.base +
>txq->cmd_ring.next2fill);
>-			if (rte_pktmbuf_pkt_len(txm) <= VMXNET3_HDR_COPY_SIZE) {
>-				struct Vmxnet3_TxDataDesc *tdd;
>-
>-				tdd = txq->data_ring.base + txq->cmd_ring.next2fill;
>-				copy_size = rte_pktmbuf_pkt_len(txm);
>-				rte_memcpy(tdd->data, rte_pktmbuf_mtod(txm, char *), copy_size);
>-			}

Why do you remove this code block?  It uses a special data ring for small
packets
which results in substantial performance improvement.  Please refer to
commit
2e849373 for details.

Related to this, I wonder what tests have you done as that¹s not clear
from the
commit descriptions?  Do we have any coverage of perf regressions?

>-
>-			/* Fill the tx descriptor */
>+		/* use the previous gen bit for the SOP desc */
>+		dw2 = (txq->cmd_ring.gen ^ 0x1) << VMXNET3_TXD_GEN_SHIFT;
>+		first2fill = txq->cmd_ring.next2fill;
>+		do {
>+			/* Remember the transmit buffer for cleanup */
> 			tbi = txq->cmd_ring.buf_info + txq->cmd_ring.next2fill;
>-			tbi->bufPA = RTE_MBUF_DATA_DMA_ADDR(txm);
>-			if (copy_size)
>-				txd->addr = rte_cpu_to_le_64(txq->data_ring.basePA +
>-							txq->cmd_ring.next2fill *
>-							sizeof(struct Vmxnet3_TxDataDesc));
>-			else
>-				txd->addr = tbi->bufPA;
>-			txd->len = txm->data_len;
>+			tbi->m = m_seg;
> 
>-			/* Mark the last descriptor as End of Packet. */
>-			txd->cq = 1;
>-			txd->eop = 1;
>+			/* NB: the following assumes that VMXNET3 maximum
>+			   transmit buffer size (16K) is greater than
>+			   maximum sizeof mbuf segment size. */

Can you add VMXNET3_ASSERT(VMXNET3_MAX_TX_BUF_SIZE >= txd->len)?

>+			gdesc = txq->cmd_ring.base + txq->cmd_ring.next2fill;
>+			gdesc->txd.addr = RTE_MBUF_DATA_DMA_ADDR(m_seg);
>+			gdesc->dword[2] = dw2 | m_seg->data_len;
>+			gdesc->dword[3] = 0;
> 
>-			/* Add VLAN tag if requested */
>-			if (txm->ol_flags & PKT_TX_VLAN_PKT) {
>-				txd->ti = 1;
>-				txd->tci = rte_cpu_to_le_16(txm->vlan_tci);
>-			}
>+			/* move to the next2fill descriptor */
>+			vmxnet3_cmd_ring_adv_next2fill(&txq->cmd_ring);
> 
>-			/* Record current mbuf for freeing it later in tx complete */
>-#ifdef RTE_LIBRTE_VMXNET3_DEBUG_DRIVER
>-			VMXNET3_ASSERT(txm);
>-#endif
>-			tbi->m = txm;
>+			/* use the right gen for non-SOP desc */
>+			dw2 = txq->cmd_ring.gen << VMXNET3_TXD_GEN_SHIFT;
>+		} while ( (m_seg = m_seg->next) != NULL);
> 
>-			/* Set the offloading mode to default */
>-			txd->hlen = 0;
>-			txd->om = VMXNET3_OM_NONE;
>-			txd->msscof = 0;
>+		/* Update the EOP descriptor */
>+		gdesc->dword[3] |= VMXNET3_TXD_EOP | VMXNET3_TXD_CQ;
> 
>-			/* finally flip the GEN bit of the SOP desc  */
>-			txd->gen = txq->cmd_ring.gen;
>-			txq->shared->ctrl.txNumDeferred++;
>+		/* Add VLAN tag if present */
>+		gdesc = txq->cmd_ring.base + first2fill;
>+		if (txm->ol_flags & PKT_TX_VLAN_PKT) {
>+			gdesc->txd.ti = 1;
>+			gdesc->txd.tci = txm->vlan_tci;
>+		}
> 
>-			/* move to the next2fill descriptor */
>-			vmxnet3_cmd_ring_adv_next2fill(&txq->cmd_ring);
>-			nb_tx++;
>+		/* TODO: Add transmit checksum offload here */
>-		} else {
>-			PMD_TX_LOG(DEBUG, "No free tx cmd desc(s)");
>-			txq->stats.drop_total += (nb_pkts - nb_tx);
>-			break;
>-		}
>+		/* flip the GEN bit on the SOP */
>+		rte_compiler_barrier();
>+		gdesc->dword[2] ^= VMXNET3_TXD_GEN;
>+
>+		txq->shared->ctrl.txNumDeferred++;
>+		nb_tx++;
> 	}
> 
> 	PMD_TX_LOG(DEBUG, "vmxnet3 txThreshold: %u",
>txq->shared->ctrl.txThreshold);
>@@ -722,12 +698,6 @@ vmxnet3_dev_tx_queue_setup(struct rte_eth_dev *dev,
> 
> 	PMD_INIT_FUNC_TRACE();
> 
>-	if ((tx_conf->txq_flags & ETH_TXQ_FLAGS_NOMULTSEGS) !=
>-	    ETH_TXQ_FLAGS_NOMULTSEGS) {
>-		PMD_INIT_LOG(ERR, "TX Multi segment not support yet");
>-		return -EINVAL;
>-	}
>-
> 	if ((tx_conf->txq_flags & ETH_TXQ_FLAGS_NOOFFLOADS) !=
> 	    ETH_TXQ_FLAGS_NOOFFLOADS) {
> 		PMD_INIT_LOG(ERR, "TX not support offload function yet");
>-- 
>2.1.3
>