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

  Signed-off-by: Tomasz Kulasek <tomaszx.kulasek@intel.com>
---
 drivers/net/ixgbe/ixgbe_ethdev.c |    3 ++
 drivers/net/ixgbe/ixgbe_ethdev.h |    8 +++-
 drivers/net/ixgbe/ixgbe_rxtx.c   |   85 +++++++++++++++++++++++++++++++++++++-
 drivers/net/ixgbe/ixgbe_rxtx.h   |    2 +
 4 files changed, 96 insertions(+), 2 deletions(-)
  

Hi Tomasz,

> Signed-off-by: Tomasz Kulasek <tomaszx.kulasek@intel.com>
> ---
>  drivers/net/ixgbe/ixgbe_ethdev.c |    3 ++
>  drivers/net/ixgbe/ixgbe_ethdev.h |    8 +++-
>  drivers/net/ixgbe/ixgbe_rxtx.c   |   85 +++++++++++++++++++++++++++++++++++++-
>  drivers/net/ixgbe/ixgbe_rxtx.h   |    2 +
>  4 files changed, 96 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/ixgbe/ixgbe_ethdev.c b/drivers/net/ixgbe/ixgbe_ethdev.c
> index 73a406b..fa6f045 100644
> --- a/drivers/net/ixgbe/ixgbe_ethdev.c
> +++ b/drivers/net/ixgbe/ixgbe_ethdev.c
> @@ -515,6 +515,8 @@ static const struct rte_eth_desc_lim tx_desc_lim = {
>  	.nb_max = IXGBE_MAX_RING_DESC,
>  	.nb_min = IXGBE_MIN_RING_DESC,
>  	.nb_align = IXGBE_TXD_ALIGN,
> +	.nb_seg_max = IXGBE_TX_MAX_SEG,
> +	.nb_mtu_seg_max = IXGBE_TX_MAX_SEG,
>  };
> 
>  static const struct eth_dev_ops ixgbe_eth_dev_ops = { @@ -1101,6 +1103,7 @@ eth_ixgbe_dev_init(struct rte_eth_dev *eth_dev)
>  	eth_dev->dev_ops = &ixgbe_eth_dev_ops;
>  	eth_dev->rx_pkt_burst = &ixgbe_recv_pkts;
>  	eth_dev->tx_pkt_burst = &ixgbe_xmit_pkts;
> +	eth_dev->tx_pkt_prep = &ixgbe_prep_pkts;
> 
>  	/*
>  	 * For secondary processes, we don't initialise any further as primary diff --git a/drivers/net/ixgbe/ixgbe_ethdev.h
> b/drivers/net/ixgbe/ixgbe_ethdev.h
> index 4ff6338..09d96de 100644
> --- a/drivers/net/ixgbe/ixgbe_ethdev.h
> +++ b/drivers/net/ixgbe/ixgbe_ethdev.h
> @@ -1,7 +1,7 @@
>  /*-
>   *   BSD LICENSE
>   *
> - *   Copyright(c) 2010-2015 Intel Corporation. All rights reserved.
> + *   Copyright(c) 2010-2016 Intel Corporation. All rights reserved.
>   *   All rights reserved.
>   *
>   *   Redistribution and use in source and binary forms, with or without
> @@ -396,6 +396,12 @@ uint16_t ixgbe_xmit_pkts(void *tx_queue, struct rte_mbuf **tx_pkts,  uint16_t
> ixgbe_xmit_pkts_simple(void *tx_queue, struct rte_mbuf **tx_pkts,
>  		uint16_t nb_pkts);
> 
> +uint16_t ixgbe_prep_pkts(void *tx_queue, struct rte_mbuf **tx_pkts,
> +		uint16_t nb_pkts);
> +
> +uint16_t ixgbe_prep_pkts_simple(void *tx_queue, struct rte_mbuf **tx_pkts,
> +		uint16_t nb_pkts);
> +
>  int ixgbe_dev_rss_hash_update(struct rte_eth_dev *dev,
>  			      struct rte_eth_rss_conf *rss_conf);
> 
> diff --git a/drivers/net/ixgbe/ixgbe_rxtx.c b/drivers/net/ixgbe/ixgbe_rxtx.c index 8b99282..2489db4 100644
> --- a/drivers/net/ixgbe/ixgbe_rxtx.c
> +++ b/drivers/net/ixgbe/ixgbe_rxtx.c
> @@ -1,7 +1,7 @@
>  /*-
>   *   BSD LICENSE
>   *
> - *   Copyright(c) 2010-2015 Intel Corporation. All rights reserved.
> + *   Copyright(c) 2010-2016 Intel Corporation. All rights reserved.
>   *   Copyright 2014 6WIND S.A.
>   *   All rights reserved.
>   *
> @@ -70,6 +70,7 @@
>  #include <rte_string_fns.h>
>  #include <rte_errno.h>
>  #include <rte_ip.h>
> +#include <rte_pkt.h>
> 
>  #include "ixgbe_logs.h"
>  #include "base/ixgbe_api.h"
> @@ -87,6 +88,9 @@
>  		PKT_TX_TCP_SEG |		 \
>  		PKT_TX_OUTER_IP_CKSUM)
> 
> +#define IXGBE_TX_OFFLOAD_NOTSUP_MASK \
> +		(PKT_TX_OFFLOAD_MASK ^ IXGBE_TX_OFFLOAD_MASK)
> +
>  #if 1
>  #define RTE_PMD_USE_PREFETCH
>  #endif
> @@ -905,6 +909,83 @@ end_of_tx:
> 
>  /*********************************************************************
>   *
> + *  TX prep functions
> + *
> +
> +**********************************************************************/
> +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

> +
> +/*********************************************************************
> + *
>   *  RX functions
>   *
>   **********************************************************************/
> @@ -2280,6 +2361,7 @@ ixgbe_set_tx_function(struct rte_eth_dev *dev, struct ixgbe_tx_queue *txq)
>  	if (((txq->txq_flags & IXGBE_SIMPLE_FLAGS) == IXGBE_SIMPLE_FLAGS)
>  			&& (txq->tx_rs_thresh >= RTE_PMD_IXGBE_TX_MAX_BURST)) {
>  		PMD_INIT_LOG(DEBUG, "Using simple tx code path");
> +		dev->tx_pkt_prep = ixgbe_prep_pkts_simple;
>  #ifdef RTE_IXGBE_INC_VECTOR
>  		if (txq->tx_rs_thresh <= RTE_IXGBE_TX_MAX_FREE_BUF_SZ &&
>  				(rte_eal_process_type() != RTE_PROC_PRIMARY || @@ -2300,6 +2382,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
  

Hi Konstantin,

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

I Tomasz,

> >
> > 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.

Great, thanks.
Konstantin