[dpdk-dev] [PATCH v2 5/6] ixgbe: add Tx preparation

[Kulasek, TomaszX]

Hi Konstantin,

> -----Original Message-----
> From: Ananyev, Konstantin
> Sent: Monday, September 19, 2016 14:55
> To: Kulasek, TomaszX <tomaszx.kulasek at intel.com>; dev at dpdk.org
> Cc: jerin.jacob at caviumnetworks.com
> Subject: RE: [dpdk-dev] [PATCH v2 5/6] ixgbe: add Tx preparation
> 
> 
> Hi Tomasz,
> 

[...]

> > +uint16_t
> > +ixgbe_prep_pkts(void *tx_queue, struct rte_mbuf **tx_pkts, uint16_t
> > +nb_pkts) {
> > +	int i, ret;
> > +	struct rte_mbuf *m;
> > +	struct ixgbe_tx_queue *txq = (struct ixgbe_tx_queue *)tx_queue;
> > +
> > +	for (i = 0; i < nb_pkts; i++) {
> > +		m = tx_pkts[i];
> > +
> > +		/**
> > +		 * Check if packet meets requirements for number of segments
> > +		 *
> > +		 * NOTE: for ixgbe it's always (40 - WTHRESH) for both TSO and
> non-TSO
> > +		 */
> > +
> > +		if (m->nb_segs > IXGBE_TX_MAX_SEG - txq->wthresh) {
> > +			rte_errno = -EINVAL;
> > +			return i;
> > +		}
> > +
> > +		if ((m->ol_flags & PKT_TX_OFFLOAD_MASK) !=
> > +				(m->ol_flags & IXGBE_TX_OFFLOAD_MASK)) {
> 
> 
> As a nit, it probably makes sense to:
> #define IXGBE_TX_OFFLOAD_NOTSUP_MASK (PKT_TX_OFFLOAD_MASK ^
> IXGBE_TX_OFFLOAD_MASK)
> 
> and then here:
> (m->ol_flags & IXGBE_TX_OFFLOAD_NOTSUP_MASK)
> 
> Might help to save few cycles.
> 

Ok.

> 
> > +			rte_errno = -EINVAL;
> > +			return i;
> > +		}
> > +
> > +#ifdef RTE_LIBRTE_ETHDEV_DEBUG
> > +		ret = rte_validate_tx_offload(m);
> > +		if (ret != 0) {
> > +			rte_errno = ret;
> > +			return i;
> > +		}
> > +#endif
> > +		ret = rte_phdr_cksum_fix(m);
> 
> We probable need to update rte_phdr_cksum_fix() to take into account
> tx_offload outer lengths in case PKT_TX_OUTER_IP_CKSUM is defined.
> As both ixgbe and i40e can do it these days.
> Sorry for not spotting that earlier.
> 

Ok.

> 
> > +		if (ret != 0) {
> > +			rte_errno = ret;
> > +			return i;
> > +		}
> > +	}
> > +
> > +	return i;
> > +}
> > +

[...]

> >
> > **********************************************************************
> > / @@ -2290,6 +2369,7 @@ ixgbe_set_tx_function(struct rte_eth_dev *dev,
> > struct ixgbe_tx_queue *txq)
> >  		} else
> >  #endif
> >  		dev->tx_pkt_burst = ixgbe_xmit_pkts_simple;
> > +		dev->tx_pkt_prep = ixgbe_prep_pkts_simple;
> 
> Shouldn't we setup ixgbe_prep_pkts_simple when vTX is selected too?
> 


It is, but source code is formatted like below:

#ifdef RTE_IXGBE_INC_VECTOR
		if (txq->tx_rs_thresh <= RTE_IXGBE_TX_MAX_FREE_BUF_SZ &&
				(rte_eal_process_type() != RTE_PROC_PRIMARY ||
					ixgbe_txq_vec_setup(txq) == 0)) {
			PMD_INIT_LOG(DEBUG, "Vector tx enabled.");
			dev->tx_pkt_burst = ixgbe_xmit_pkts_vec;
		} else
#endif
		dev->tx_pkt_burst = ixgbe_xmit_pkts_simple;
		dev->tx_pkt_prep = ixgbe_prep_pkts_simple;


> >  	} else {
> >  		PMD_INIT_LOG(DEBUG, "Using full-featured tx code path");
> >  		PMD_INIT_LOG(DEBUG,
> > @@ -2301,6 +2381,7 @@ ixgbe_set_tx_function(struct rte_eth_dev *dev,
> struct ixgbe_tx_queue *txq)
> >  				(unsigned long)txq->tx_rs_thresh,
> >  				(unsigned long)RTE_PMD_IXGBE_TX_MAX_BURST);
> >  		dev->tx_pkt_burst = ixgbe_xmit_pkts;
> > +		dev->tx_pkt_prep = ixgbe_prep_pkts;
> >  	}
> >  }
> >
> > diff --git a/drivers/net/ixgbe/ixgbe_rxtx.h
> > b/drivers/net/ixgbe/ixgbe_rxtx.h index 2608b36..7bbd9b8 100644
> > --- a/drivers/net/ixgbe/ixgbe_rxtx.h
> > +++ b/drivers/net/ixgbe/ixgbe_rxtx.h
> > @@ -80,6 +80,8 @@
> >  #define RTE_IXGBE_WAIT_100_US               100
> >  #define RTE_IXGBE_VMTXSW_REGISTER_COUNT     2
> >
> > +#define IXGBE_TX_MAX_SEG                    40
> > +
> >  #define IXGBE_PACKET_TYPE_MASK_82599        0X7F
> >  #define IXGBE_PACKET_TYPE_MASK_X550         0X10FF
> >  #define IXGBE_PACKET_TYPE_MASK_TUNNEL       0XFF
> > --
> > 1.7.9.5

Tomasz.