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

[Kulasek, TomaszX]

Hi Konstantin,

> -----Original Message-----
> From: Ananyev, Konstantin
> Sent: Thursday, September 29, 2016 13:09
> To: Kulasek, TomaszX <tomaszx.kulasek at intel.com>; dev at dpdk.org
> Subject: RE: [PATCH v3 5/6] ixgbe: add Tx preparation
> 
> Hi Tomasz,
> 
> > Signed-off-by: Tomasz Kulasek <tomaszx.kulasek at intel.com>
> > ---

...

> > +*/
> > +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 & IXGBE_TX_OFFLOAD_NOTSUP_MASK) {
> > +			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);
> > +		if (ret != 0) {
> > +			rte_errno = ret;
> > +			return i;
> > +		}
> > +	}
> > +
> > +	return i;
> > +}
> > +
> > +/* ixgbe simple path as well as vector TX doesn't support tx offloads
> > +*/ uint16_t ixgbe_prep_pkts_simple(void *tx_queue __rte_unused,
> > +struct rte_mbuf **tx_pkts,
> > +		uint16_t nb_pkts)
> > +{
> > +	int i;
> > +	struct rte_mbuf *m;
> > +	uint64_t ol_flags;
> > +
> > +	for (i = 0; i < nb_pkts; i++) {
> > +		m = tx_pkts[i];
> > +		ol_flags = m->ol_flags;
> > +
> > +		/* simple tx path doesn't support multi-segments */
> > +		if (m->nb_segs != 1) {
> > +			rte_errno = -EINVAL;
> > +			return i;
> > +		}
> > +
> > +		/* For simple path (simple and vector) no tx offloads are
> supported */
> > +		if (ol_flags & PKT_TX_OFFLOAD_MASK) {
> > +			rte_errno = -EINVAL;
> > +			return i;
> > +		}
> > +	}
> > +
> > +	return i;
> > +}
> 
> Just thought about it once again:
> As now inside rte_eth_tx_prep() we do now:
> +
> +	if (!dev->tx_pkt_prep)
> +		return nb_pkts;
> 
> Then there might be a better approach to set
> dev->tx_pkt_prep = NULL
> for simple and vector TX functions?
> 
> After all, prep_simple() does nothing but returns an error if conditions are
> not met.
> And if simple TX was already selected, then that means that user deliberately
> disabled all HW TX offloads in favor of faster TX and there is no point to slow
> him down with extra checks here.
> Same for i40e and fm10k.
> What is your opinion?
> 
> Konstantin
> 

Yes, if performance is a key, and, while the limitations of vector/simple path are quite well documented, these additional checks are a bit overzealous. We may assume that to made tx offloads working, we need to configure driver in a right way, and this is a configuration issue if something doesn't work.

I will remove it.

Tomasz