[RFC PATCH 0/4] ethdev new offloads API

[RFC PATCH 0/4] ethdev new offloads API

[Shahaf Shuler]

Tx offloads configuration is per queue. Tx offloads are enabled by default, 
and can be disabled using ETH_TXQ_FLAGS_NO* flags. 
This behaviour is not consistent with the Rx side where the Rx offloads
configuration is per port. Rx offloads are disabled by default and enabled 
according to bit field in rte_eth_rxmode structure.

Moreover, considering more Tx and Rx offloads will be added 
over time, the cost of managing them all inside the PMD will be tremendous,
as the PMD will need to check the matching for the entire offload set 
for each mbuf it handles.
In addition, on the current approach each Rx offload added breaks the
ABI compatibility as it requires to add entries to existing bit-fields.
 
The RFC address above issues by defining a new offloads API.
With the new API, Tx and Rx offloads configuration is per queue.
The offloads are disabled by default. Each offload can be enabled or
disabled using the existing DEV_TX_OFFLOADS_* or DEV_RX_OFFLOADS_* flags.
Such API will enable to easily add or remove offloads, without breaking the
ABI compatibility.

The new API does not have an equivalent for the below Tx flags:

* ETH_TXQ_FLAGS_NOREFCOUNT
* ETH_TXQ_FLAGS_NOMULTMEMP

The reason is that those flags are not to manage offloads, rather some
guarantee from application on the way it uses mbufs, therefore could not be
present as part of DEV_TX_OFFLOADS_*.
Such flags are useful only for benchmarks, and therefore provide a non-realistic    
performance for DPDK customers using simple benchmarks for evaluation.
Leveraging the work being done in this series to clean up those flags.

In order to provide a smooth transition between the APIs the following actions
were taken:
*  The old offloads API is kept for the meanwhile.
*  New capabilities were added for PMD to advertize it has moved to the new
   offloads API.
*  Helper function which copy from old to new API and vice versa were added to ethdev,
   enabling the PMD to support only one of the APIs, and the application to move to
   the new API regardless the underlying device and without extra branching.

Shahaf Shuler (4):
  ethdev: rename Rx and Tx configuration structs
  ethdev: introduce Rx queue offloads API
  ethdev: introduce Tx queue offloads API
  ethdev: add helpers to move to the new offloads API

 lib/librte_ether/rte_ethdev.c | 144 ++++++++++++++++++++++++++++++++++++-
 lib/librte_ether/rte_ethdev.h |  72 +++++++++++++++----
 2 files changed, 202 insertions(+), 14 deletions(-)

-- 
2.12.0

[RFC PATCH 1/4] ethdev: rename Rx and Tx configuration structs

[Shahaf Shuler]

Rename the structs rte_eth_txconf and rte_eth_rxconf to
rte_eth_txq_conf and rte_eth_rxq_conf respectively as those
structs represent per queue configuration.

Signed-off-by: Shahaf Shuler <shahafs@mellanox.com>
---
 lib/librte_ether/rte_ethdev.c |  4 ++--
 lib/librte_ether/rte_ethdev.h | 24 +++++++++++++-----------
 2 files changed, 15 insertions(+), 13 deletions(-)

diff --git a/lib/librte_ether/rte_ethdev.c b/lib/librte_ether/rte_ethdev.c
index d4ebb1b67..f73307e99 100644
--- a/lib/librte_ether/rte_ethdev.c
+++ b/lib/librte_ether/rte_ethdev.c
@@ -1006,7 +1006,7 @@ rte_eth_dev_close(uint8_t port_id)
 int
 rte_eth_rx_queue_setup(uint8_t port_id, uint16_t rx_queue_id,
 		       uint16_t nb_rx_desc, unsigned int socket_id,
-		       const struct rte_eth_rxconf *rx_conf,
+		       const struct rte_eth_rxq_conf *rx_conf,
 		       struct rte_mempool *mp)
 {
 	int ret;
@@ -1097,7 +1097,7 @@ rte_eth_rx_queue_setup(uint8_t port_id, uint16_t rx_queue_id,
 int
 rte_eth_tx_queue_setup(uint8_t port_id, uint16_t tx_queue_id,
 		       uint16_t nb_tx_desc, unsigned int socket_id,
-		       const struct rte_eth_txconf *tx_conf)
+		       const struct rte_eth_txq_conf *tx_conf)
 {
 	struct rte_eth_dev *dev;
 	struct rte_eth_dev_info dev_info;
diff --git a/lib/librte_ether/rte_ethdev.h b/lib/librte_ether/rte_ethdev.h
index 0e99090f6..f7a44578c 100644
--- a/lib/librte_ether/rte_ethdev.h
+++ b/lib/librte_ether/rte_ethdev.h
@@ -686,7 +686,7 @@ struct rte_eth_txmode {
 /**
  * A structure used to configure an RX ring of an Ethernet port.
  */
-struct rte_eth_rxconf {
+struct rte_eth_rxq_conf {
 	struct rte_eth_thresh rx_thresh; /**< RX ring threshold registers. */
 	uint16_t rx_free_thresh; /**< Drives the freeing of RX descriptors. */
 	uint8_t rx_drop_en; /**< Drop packets if no descriptors are available. */
@@ -709,7 +709,7 @@ struct rte_eth_rxconf {
 /**
  * A structure used to configure a TX ring of an Ethernet port.
  */
-struct rte_eth_txconf {
+struct rte_eth_txq_conf {
 	struct rte_eth_thresh tx_thresh; /**< TX ring threshold registers. */
 	uint16_t tx_rs_thresh; /**< Drives the setting of RS bit on TXDs. */
 	uint16_t tx_free_thresh; /**< Start freeing TX buffers if there are
@@ -956,8 +956,10 @@ struct rte_eth_dev_info {
 	uint8_t hash_key_size; /**< Hash key size in bytes */
 	/** Bit mask of RSS offloads, the bit offset also means flow type */
 	uint64_t flow_type_rss_offloads;
-	struct rte_eth_rxconf default_rxconf; /**< Default RX configuration */
-	struct rte_eth_txconf default_txconf; /**< Default TX configuration */
+	struct rte_eth_rxq_conf default_rx_conf;
+	/**< Default RX queue configuration */
+	struct rte_eth_txq_conf default_tx_conf;
+	/**< Default TX queue configuration */
 	uint16_t vmdq_queue_base; /**< First queue ID for VMDQ pools. */
 	uint16_t vmdq_queue_num;  /**< Queue number for VMDQ pools. */
 	uint16_t vmdq_pool_base;  /**< First ID of VMDQ pools. */
@@ -975,7 +977,7 @@ struct rte_eth_dev_info {
  */
 struct rte_eth_rxq_info {
 	struct rte_mempool *mp;     /**< mempool used by that queue. */
-	struct rte_eth_rxconf conf; /**< queue config parameters. */
+	struct rte_eth_rxq_conf conf; /**< queue config parameters. */
 	uint8_t scattered_rx;       /**< scattered packets RX supported. */
 	uint16_t nb_desc;           /**< configured number of RXDs. */
 } __rte_cache_min_aligned;
@@ -985,7 +987,7 @@ struct rte_eth_rxq_info {
  * Used to retieve information about configured queue.
  */
 struct rte_eth_txq_info {
-	struct rte_eth_txconf conf; /**< queue config parameters. */
+	struct rte_eth_txq_conf conf; /**< queue config parameters. */
 	uint16_t nb_desc;           /**< configured number of TXDs. */
 } __rte_cache_min_aligned;
 
@@ -1185,7 +1187,7 @@ typedef int (*eth_rx_queue_setup_t)(struct rte_eth_dev *dev,
 				    uint16_t rx_queue_id,
 				    uint16_t nb_rx_desc,
 				    unsigned int socket_id,
-				    const struct rte_eth_rxconf *rx_conf,
+				    const struct rte_eth_rxq_conf *rx_conf,
 				    struct rte_mempool *mb_pool);
 /**< @internal Set up a receive queue of an Ethernet device. */
 
@@ -1193,7 +1195,7 @@ typedef int (*eth_tx_queue_setup_t)(struct rte_eth_dev *dev,
 				    uint16_t tx_queue_id,
 				    uint16_t nb_tx_desc,
 				    unsigned int socket_id,
-				    const struct rte_eth_txconf *tx_conf);
+				    const struct rte_eth_txq_conf *tx_conf);
 /**< @internal Setup a transmit queue of an Ethernet device. */
 
 typedef int (*eth_rx_enable_intr_t)(struct rte_eth_dev *dev,
@@ -1936,7 +1938,7 @@ void _rte_eth_dev_reset(struct rte_eth_dev *dev);
  */
 int rte_eth_rx_queue_setup(uint8_t port_id, uint16_t rx_queue_id,
 		uint16_t nb_rx_desc, unsigned int socket_id,
-		const struct rte_eth_rxconf *rx_conf,
+		const struct rte_eth_rxq_conf *rx_conf,
 		struct rte_mempool *mb_pool);
 
 /**
@@ -1984,7 +1986,7 @@ int rte_eth_rx_queue_setup(uint8_t port_id, uint16_t rx_queue_id,
  */
 int rte_eth_tx_queue_setup(uint8_t port_id, uint16_t tx_queue_id,
 		uint16_t nb_tx_desc, unsigned int socket_id,
-		const struct rte_eth_txconf *tx_conf);
+		const struct rte_eth_txq_conf *tx_conf);
 
 /**
  * Return the NUMA socket to which an Ethernet device is connected
@@ -2971,7 +2973,7 @@ static inline int rte_eth_tx_descriptor_status(uint8_t port_id,
  *
  * If the PMD is DEV_TX_OFFLOAD_MT_LOCKFREE capable, multiple threads can
  * invoke this function concurrently on the same tx queue without SW lock.
- * @see rte_eth_dev_info_get, struct rte_eth_txconf::txq_flags
+ * @see rte_eth_dev_info_get, struct rte_eth_txq_conf::txq_flags
  *
  * @param port_id
  *   The port identifier of the Ethernet device.
-- 
2.12.0

Re: [RFC PATCH 1/4] ethdev: rename Rx and Tx configuration structs

[Thomas Monjalon]

07/08/2017 12:54, Shahaf Shuler:
> Rename the structs rte_eth_txconf and rte_eth_rxconf to
> rte_eth_txq_conf and rte_eth_rxq_conf respectively as those
> structs represent per queue configuration.
> 
> Signed-off-by: Shahaf Shuler <shahafs@mellanox.com>

Acked-by: Thomas Monjalon <thomas@monjalon.net>

[RFC PATCH 2/4] ethdev: introduce Rx queue offloads API

[Shahaf Shuler]

Introduce a new API to configure Rx offloads.

The new API will re-use existing DEV_RX_OFFLOAD_* flags
to enable the different offloads. This will ease the process
of adding a new Rx offloads, as no ABI breakage is involved.
In addition, the offload configuration can be done per queue,
instead of per port.

The Rx queue offload API can be used only with devices which advertize
the RTE_ETH_DEV_RXQ_OFFLOAD capability.

The old Rx offloads API is kept for the meanwhile, in order to enable a
smooth transition for PMDs and application to the new API.

Signed-off-by: Shahaf Shuler <shahafs@mellanox.com>
---
 lib/librte_ether/rte_ethdev.h | 30 +++++++++++++++++++++++++++++-
 1 file changed, 29 insertions(+), 1 deletion(-)

diff --git a/lib/librte_ether/rte_ethdev.h b/lib/librte_ether/rte_ethdev.h
index f7a44578c..ce33c61c4 100644
--- a/lib/librte_ether/rte_ethdev.h
+++ b/lib/librte_ether/rte_ethdev.h
@@ -357,7 +357,14 @@ struct rte_eth_rxmode {
 		jumbo_frame      : 1, /**< Jumbo Frame Receipt enable. */
 		hw_strip_crc     : 1, /**< Enable CRC stripping by hardware. */
 		enable_scatter   : 1, /**< Enable scatter packets rx handler */
-		enable_lro       : 1; /**< Enable LRO */
+		enable_lro       : 1, /**< Enable LRO */
+		ignore		 : 1;
+		/**
+		 * When set the rxmode offloads should be ignored,
+		 * instead the Rx offloads will be set on rte_eth_rxq_conf.
+		 * This bit is temporary till rxmode Rx offloads API will
+		 * be deprecated.
+		 */
 };
 
 /**
@@ -691,6 +698,12 @@ struct rte_eth_rxq_conf {
 	uint16_t rx_free_thresh; /**< Drives the freeing of RX descriptors. */
 	uint8_t rx_drop_en; /**< Drop packets if no descriptors are available. */
 	uint8_t rx_deferred_start; /**< Do not start queue with rte_eth_dev_start(). */
+	uint64_t offloads;
+	/**
+	 * Enable Rx offloads using DEV_RX_OFFLOAD_* flags.
+	 * Supported only for devices which advertize the
+	 * RTE_ETH_DEV_RXQ_OFFLOAD capability.
+	 */
 };
 
 #define ETH_TXQ_FLAGS_NOMULTSEGS 0x0001 /**< nb_segs=1 for all mbufs */
@@ -907,6 +920,19 @@ struct rte_eth_conf {
 #define DEV_RX_OFFLOAD_QINQ_STRIP  0x00000020
 #define DEV_RX_OFFLOAD_OUTER_IPV4_CKSUM 0x00000040
 #define DEV_RX_OFFLOAD_MACSEC_STRIP     0x00000080
+#define DEV_RX_OFFLOAD_HEADER_SPLIT	0x00000100
+#define DEV_RX_OFFLOAD_VLAN_FILTER	0x00000200
+#define DEV_RX_OFFLOAD_VLAN_EXTEND	0x00000400
+#define DEV_RX_OFFLOAD_JUMBO_FRAME	0x00000800
+#define DEV_RX_OFFLOAD_CRC_STRIP	0x00001000
+#define DEV_RX_OFFLOAD_SCATTER		0x00002000
+#define DEV_RX_OFFLOAD_LRO		0x00004000
+#define DEV_RX_OFFLOAD_CHECKSUM (DEV_RX_OFFLOAD_IPV4_CKSUM | \
+				 DEV_RX_OFFLOAD_UDP_CKSUM | \
+				 DEV_RX_OFFLOAD_TCP_CKSUM)
+#define DEV_RX_OFFLOAD_VLAN (DEV_RX_OFFLOAD_VLAN_STRIP | \
+			     DEV_RX_OFFLOAD_VLAN_FILTER | \
+			     DEV_RX_OFFLOAD_VLAN_EXTEND)
 
 /**
  * TX offload capabilities of a device.
@@ -1723,6 +1749,8 @@ struct rte_eth_dev_data {
 #define RTE_ETH_DEV_BONDED_SLAVE 0x0004
 /** Device supports device removal interrupt */
 #define RTE_ETH_DEV_INTR_RMV     0x0008
+/** Device supports the rte_eth_rxq_conf offloads API */
+#define RTE_ETH_DEV_RXQ_OFFLOAD 0x0010
 
 /**
  * @internal
-- 
2.12.0

Re: [RFC PATCH 2/4] ethdev: introduce Rx queue offloads API

[Ananyev, Konstantin]

Hi,

> Introduce a new API to configure Rx offloads.
> 
> The new API will re-use existing DEV_RX_OFFLOAD_* flags
> to enable the different offloads. This will ease the process
> of adding a new Rx offloads, as no ABI breakage is involved.
> In addition, the offload configuration can be done per queue,
> instead of per port.
> 
> The Rx queue offload API can be used only with devices which advertize
> the RTE_ETH_DEV_RXQ_OFFLOAD capability.

Not sure why do we need that?
Why (till the deprication) user can't use both old rxmode and new offload flags?
I.E. at rte_ethdev_rx_queue_setup() we always convert rxmode to new offload flags
(as I can see you already have a function for it), then we call dev_ops->rx_queue_setup().
That way both new and old ethdev API should work.
Of course that mean that all PMDs have to support new way to setup rx offloads...
But I suppose that have to be done anyway.
Same for TX path.
Konstantin

> 
> The old Rx offloads API is kept for the meanwhile, in order to enable a
> smooth transition for PMDs and application to the new API.
> 
> Signed-off-by: Shahaf Shuler <shahafs@mellanox.com>
> ---
>  lib/librte_ether/rte_ethdev.h | 30 +++++++++++++++++++++++++++++-
>  1 file changed, 29 insertions(+), 1 deletion(-)
> 
> diff --git a/lib/librte_ether/rte_ethdev.h b/lib/librte_ether/rte_ethdev.h
> index f7a44578c..ce33c61c4 100644
> --- a/lib/librte_ether/rte_ethdev.h
> +++ b/lib/librte_ether/rte_ethdev.h
> @@ -357,7 +357,14 @@ struct rte_eth_rxmode {
>  		jumbo_frame      : 1, /**< Jumbo Frame Receipt enable. */
>  		hw_strip_crc     : 1, /**< Enable CRC stripping by hardware. */
>  		enable_scatter   : 1, /**< Enable scatter packets rx handler */
> -		enable_lro       : 1; /**< Enable LRO */
> +		enable_lro       : 1, /**< Enable LRO */
> +		ignore		 : 1;
> +		/**
> +		 * When set the rxmode offloads should be ignored,
> +		 * instead the Rx offloads will be set on rte_eth_rxq_conf.
> +		 * This bit is temporary till rxmode Rx offloads API will
> +		 * be deprecated.
> +		 */
>  };
> 
>  /**
> @@ -691,6 +698,12 @@ struct rte_eth_rxq_conf {
>  	uint16_t rx_free_thresh; /**< Drives the freeing of RX descriptors. */
>  	uint8_t rx_drop_en; /**< Drop packets if no descriptors are available. */
>  	uint8_t rx_deferred_start; /**< Do not start queue with rte_eth_dev_start(). */
> +	uint64_t offloads;
> +	/**
> +	 * Enable Rx offloads using DEV_RX_OFFLOAD_* flags.
> +	 * Supported only for devices which advertize the
> +	 * RTE_ETH_DEV_RXQ_OFFLOAD capability.
> +	 */
>  };
> 
>  #define ETH_TXQ_FLAGS_NOMULTSEGS 0x0001 /**< nb_segs=1 for all mbufs */
> @@ -907,6 +920,19 @@ struct rte_eth_conf {
>  #define DEV_RX_OFFLOAD_QINQ_STRIP  0x00000020
>  #define DEV_RX_OFFLOAD_OUTER_IPV4_CKSUM 0x00000040
>  #define DEV_RX_OFFLOAD_MACSEC_STRIP     0x00000080
> +#define DEV_RX_OFFLOAD_HEADER_SPLIT	0x00000100
> +#define DEV_RX_OFFLOAD_VLAN_FILTER	0x00000200
> +#define DEV_RX_OFFLOAD_VLAN_EXTEND	0x00000400
> +#define DEV_RX_OFFLOAD_JUMBO_FRAME	0x00000800
> +#define DEV_RX_OFFLOAD_CRC_STRIP	0x00001000
> +#define DEV_RX_OFFLOAD_SCATTER		0x00002000
> +#define DEV_RX_OFFLOAD_LRO		0x00004000
> +#define DEV_RX_OFFLOAD_CHECKSUM (DEV_RX_OFFLOAD_IPV4_CKSUM | \
> +				 DEV_RX_OFFLOAD_UDP_CKSUM | \
> +				 DEV_RX_OFFLOAD_TCP_CKSUM)
> +#define DEV_RX_OFFLOAD_VLAN (DEV_RX_OFFLOAD_VLAN_STRIP | \
> +			     DEV_RX_OFFLOAD_VLAN_FILTER | \
> +			     DEV_RX_OFFLOAD_VLAN_EXTEND)
> 
>  /**
>   * TX offload capabilities of a device.
> @@ -1723,6 +1749,8 @@ struct rte_eth_dev_data {
>  #define RTE_ETH_DEV_BONDED_SLAVE 0x0004
>  /** Device supports device removal interrupt */
>  #define RTE_ETH_DEV_INTR_RMV     0x0008
> +/** Device supports the rte_eth_rxq_conf offloads API */
> +#define RTE_ETH_DEV_RXQ_OFFLOAD 0x0010
> 
>  /**
>   * @internal
> --
> 2.12.0

Re: [RFC PATCH 2/4] ethdev: introduce Rx queue offloads API

[Shahaf Shuler]

Wednesday, August 23, 2017 3:21 PM, Ananyev, Konstantin:
> Hi,
> 
> > Introduce a new API to configure Rx offloads.
> >
> > The new API will re-use existing DEV_RX_OFFLOAD_* flags to enable the
> > different offloads. This will ease the process of adding a new Rx
> > offloads, as no ABI breakage is involved.
> > In addition, the offload configuration can be done per queue, instead
> > of per port.
> >
> > The Rx queue offload API can be used only with devices which advertize
> > the RTE_ETH_DEV_RXQ_OFFLOAD capability.
> 
> Not sure why do we need that?
> Why (till the deprication) user can't use both old rxmode and new offload
> flags?
> I.E. at rte_ethdev_rx_queue_setup() we always convert rxmode to new
> offload flags (as I can see you already have a function for it), then we call
> dev_ops->rx_queue_setup().
> That way both new and old ethdev API should work.
> Of course that mean that all PMDs have to support new way to setup rx
> offloads...
> But I suppose that have to be done anyway.
> Same for TX path.

The reason for this capability is to enable the different PMDs to *gradually move* to the new API.
Offloads management on the PMD is complex, it has to deal with the internal of the device on which they work. For example some devices probably
have offloads configuration they can do only per port.  Another example can be that some PMDs enable Tx offloads from the PMD specific parameters (like mlx5). 

This is why I believe that the task to move the PMD to the new API should be done by the PMD developers/maintainers.
This work provides the means (the copy functions and the capability bits) for both APIs to co-exists and to be used on top of PMDs which support either old or new API.

Once the majority of the PMD made the transition, we can remove and deprecate this cap (and the old API).

> Konstantin
> 
> >
> > The old Rx offloads API is kept for the meanwhile, in order to enable
> > a smooth transition for PMDs and application to the new API.
> >
> > Signed-off-by: Shahaf Shuler <shahafs@mellanox.com>
> > ---
> >  lib/librte_ether/rte_ethdev.h | 30 +++++++++++++++++++++++++++++-
> >  1 file changed, 29 insertions(+), 1 deletion(-)
> >
> > diff --git a/lib/librte_ether/rte_ethdev.h
> > b/lib/librte_ether/rte_ethdev.h index f7a44578c..ce33c61c4 100644
> > --- a/lib/librte_ether/rte_ethdev.h
> > +++ b/lib/librte_ether/rte_ethdev.h
> > @@ -357,7 +357,14 @@ struct rte_eth_rxmode {
> >  		jumbo_frame      : 1, /**< Jumbo Frame Receipt enable. */
> >  		hw_strip_crc     : 1, /**< Enable CRC stripping by hardware. */
> >  		enable_scatter   : 1, /**< Enable scatter packets rx handler */
> > -		enable_lro       : 1; /**< Enable LRO */
> > +		enable_lro       : 1, /**< Enable LRO */
> > +		ignore		 : 1;
> > +		/**
> > +		 * When set the rxmode offloads should be ignored,
> > +		 * instead the Rx offloads will be set on rte_eth_rxq_conf.
> > +		 * This bit is temporary till rxmode Rx offloads API will
> > +		 * be deprecated.
> > +		 */
> >  };
> >
> >  /**
> > @@ -691,6 +698,12 @@ struct rte_eth_rxq_conf {
> >  	uint16_t rx_free_thresh; /**< Drives the freeing of RX descriptors.
> */
> >  	uint8_t rx_drop_en; /**< Drop packets if no descriptors are
> available. */
> >  	uint8_t rx_deferred_start; /**< Do not start queue with
> > rte_eth_dev_start(). */
> > +	uint64_t offloads;
> > +	/**
> > +	 * Enable Rx offloads using DEV_RX_OFFLOAD_* flags.
> > +	 * Supported only for devices which advertize the
> > +	 * RTE_ETH_DEV_RXQ_OFFLOAD capability.
> > +	 */
> >  };
> >
> >  #define ETH_TXQ_FLAGS_NOMULTSEGS 0x0001 /**< nb_segs=1 for all
> mbufs
> > */ @@ -907,6 +920,19 @@ struct rte_eth_conf {  #define
> > DEV_RX_OFFLOAD_QINQ_STRIP  0x00000020  #define
> > DEV_RX_OFFLOAD_OUTER_IPV4_CKSUM 0x00000040
> >  #define DEV_RX_OFFLOAD_MACSEC_STRIP     0x00000080
> > +#define DEV_RX_OFFLOAD_HEADER_SPLIT	0x00000100
> > +#define DEV_RX_OFFLOAD_VLAN_FILTER	0x00000200
> > +#define DEV_RX_OFFLOAD_VLAN_EXTEND	0x00000400
> > +#define DEV_RX_OFFLOAD_JUMBO_FRAME	0x00000800
> > +#define DEV_RX_OFFLOAD_CRC_STRIP	0x00001000
> > +#define DEV_RX_OFFLOAD_SCATTER		0x00002000
> > +#define DEV_RX_OFFLOAD_LRO		0x00004000
> > +#define DEV_RX_OFFLOAD_CHECKSUM
> (DEV_RX_OFFLOAD_IPV4_CKSUM | \
> > +				 DEV_RX_OFFLOAD_UDP_CKSUM | \
> > +				 DEV_RX_OFFLOAD_TCP_CKSUM)
> > +#define DEV_RX_OFFLOAD_VLAN (DEV_RX_OFFLOAD_VLAN_STRIP | \
> > +			     DEV_RX_OFFLOAD_VLAN_FILTER | \
> > +			     DEV_RX_OFFLOAD_VLAN_EXTEND)
> >
> >  /**
> >   * TX offload capabilities of a device.
> > @@ -1723,6 +1749,8 @@ struct rte_eth_dev_data {  #define
> > RTE_ETH_DEV_BONDED_SLAVE 0x0004
> >  /** Device supports device removal interrupt */
> >  #define RTE_ETH_DEV_INTR_RMV     0x0008
> > +/** Device supports the rte_eth_rxq_conf offloads API */ #define
> > +RTE_ETH_DEV_RXQ_OFFLOAD 0x0010
> >
> >  /**
> >   * @internal
> > --
> > 2.12.0

Re: [RFC PATCH 2/4] ethdev: introduce Rx queue offloads API

[Thomas Monjalon]

07/08/2017 12:54, Shahaf Shuler:
> Introduce a new API to configure Rx offloads.
> 
> The new API will re-use existing DEV_RX_OFFLOAD_* flags
> to enable the different offloads. This will ease the process
> of adding a new Rx offloads, as no ABI breakage is involved.
> In addition, the offload configuration can be done per queue,
> instead of per port.
> 
> The Rx queue offload API can be used only with devices which advertize
> the RTE_ETH_DEV_RXQ_OFFLOAD capability.
> 
> The old Rx offloads API is kept for the meanwhile, in order to enable a
> smooth transition for PMDs and application to the new API.
> 
> Signed-off-by: Shahaf Shuler <shahafs@mellanox.com>
[...]
> --- a/lib/librte_ether/rte_ethdev.h
> +++ b/lib/librte_ether/rte_ethdev.h
> @@ -357,7 +357,14 @@ struct rte_eth_rxmode {
>  		jumbo_frame      : 1, /**< Jumbo Frame Receipt enable. */
>  		hw_strip_crc     : 1, /**< Enable CRC stripping by hardware. */
>  		enable_scatter   : 1, /**< Enable scatter packets rx handler */
> -		enable_lro       : 1; /**< Enable LRO */
> +		enable_lro       : 1, /**< Enable LRO */
> +		ignore		 : 1;
> +		/**
> +		 * When set the rxmode offloads should be ignored,
> +		 * instead the Rx offloads will be set on rte_eth_rxq_conf.
> +		 * This bit is temporary till rxmode Rx offloads API will
> +		 * be deprecated.
> +		 */

Who is responsible to set the "ignore" flag?

Should it be documented in queue config functions?

> +/** Device supports the rte_eth_rxq_conf offloads API */
> +#define RTE_ETH_DEV_RXQ_OFFLOAD 0x0010

Otherwise, looks good

Re: [RFC PATCH 2/4] ethdev: introduce Rx queue offloads API

[Ferruh Yigit]

On 8/7/2017 11:54 AM, Shahaf Shuler wrote:
> Introduce a new API to configure Rx offloads.
> 
> The new API will re-use existing DEV_RX_OFFLOAD_* flags
> to enable the different offloads. This will ease the process
> of adding a new Rx offloads, as no ABI breakage is involved.
> In addition, the offload configuration can be done per queue,
> instead of per port.

If a device doesn't have capability to set the offload per queue how
should it behave, I think it is good to define this.

> 
> The Rx queue offload API can be used only with devices which advertize
> the RTE_ETH_DEV_RXQ_OFFLOAD capability.
> 
> The old Rx offloads API is kept for the meanwhile, in order to enable a
> smooth transition for PMDs and application to the new API.
> 
> Signed-off-by: Shahaf Shuler <shahafs@mellanox.com>

<...>

> @@ -357,7 +357,14 @@ struct rte_eth_rxmode {
>  		jumbo_frame      : 1, /**< Jumbo Frame Receipt enable. */
>  		hw_strip_crc     : 1, /**< Enable CRC stripping by hardware. */
>  		enable_scatter   : 1, /**< Enable scatter packets rx handler */
> -		enable_lro       : 1; /**< Enable LRO */
> +		enable_lro       : 1, /**< Enable LRO */
> +		ignore		 : 1;

what do you think making this variable more verbose, like
"ignore_rx_offloads"

"dev_conf.rxmode.ignore" doesn't say on its own what is ignored.

> +		/**
> +		 * When set the rxmode offloads should be ignored,
> +		 * instead the Rx offloads will be set on rte_eth_rxq_conf.
> +		 * This bit is temporary till rxmode Rx offloads API will
> +		 * be deprecated.
> +		 */
>  };

<...>

> +/** Device supports the rte_eth_rxq_conf offloads API */
> +#define RTE_ETH_DEV_RXQ_OFFLOAD 0x0010
Since this is temporary flag and with current implementation this is
local to library, should we put this into public header?

Later when all PMDs implemented this new method and we want to remove
the flag, can we remove them or do we have to keep them reserved for any
conflict for further new values?

I guess this should be part of missing pmd-ethdev interface file
(rte_ethdev_pmd.h ?).

>  
>  /**
>   * @internal
> 

Re: [RFC PATCH 2/4] ethdev: introduce Rx queue offloads API

[Shahaf Shuler]

Tuesday, August 29, 2017 3:50 PM, Ferruh Yigit:
> On 8/7/2017 11:54 AM, Shahaf Shuler wrote:
> > Introduce a new API to configure Rx offloads.
> >
> > The new API will re-use existing DEV_RX_OFFLOAD_* flags to enable the
> > different offloads. This will ease the process of adding a new Rx
> > offloads, as no ABI breakage is involved.
> > In addition, the offload configuration can be done per queue, instead
> > of per port.
> 
> If a device doesn't have capability to set the offload per queue how should it
> behave, I think it is good to define this.

Yes, will add documentation. 
How about If device cannot set offloads per queue, then the queue_setup function should return with ENOTSUP ?

> 
> >
> > The Rx queue offload API can be used only with devices which advertize
> > the RTE_ETH_DEV_RXQ_OFFLOAD capability.
> >
> > The old Rx offloads API is kept for the meanwhile, in order to enable
> > a smooth transition for PMDs and application to the new API.
> >
> > Signed-off-by: Shahaf Shuler <shahafs@mellanox.com>
> 
> <...>
> 
> > @@ -357,7 +357,14 @@ struct rte_eth_rxmode {
> >  		jumbo_frame      : 1, /**< Jumbo Frame Receipt enable. */
> >  		hw_strip_crc     : 1, /**< Enable CRC stripping by hardware. */
> >  		enable_scatter   : 1, /**< Enable scatter packets rx handler */
> > -		enable_lro       : 1; /**< Enable LRO */
> > +		enable_lro       : 1, /**< Enable LRO */
> > +		ignore		 : 1;
> 
> what do you think making this variable more verbose, like
> "ignore_rx_offloads"
> 
> "dev_conf.rxmode.ignore" doesn't say on its own what is ignored.

Maybe ignore_offloads ? Rx is quite explicit from rxomde.

> 
> > +		/**
> > +		 * When set the rxmode offloads should be ignored,
> > +		 * instead the Rx offloads will be set on rte_eth_rxq_conf.
> > +		 * This bit is temporary till rxmode Rx offloads API will
> > +		 * be deprecated.
> > +		 */
> >  };
> 
> <...>
> 
> > +/** Device supports the rte_eth_rxq_conf offloads API */ #define
> > +RTE_ETH_DEV_RXQ_OFFLOAD 0x0010
> Since this is temporary flag and with current implementation this is local to
> library, should we put this into public header?
> 
> Later when all PMDs implemented this new method and we want to remove
> the flag, can we remove them or do we have to keep them reserved for any
> conflict for further new values?
> 
> I guess this should be part of missing pmd-ethdev interface file
> (rte_ethdev_pmd.h ?).

Yes it is better fits to inner interface between ethdev and PMDs.
Wondering, do we have other motivation to have such header? 

> 
> >
> >  /**
> >   * @internal
> >

Re: [RFC PATCH 2/4] ethdev: introduce Rx queue offloads API

[Ferruh Yigit]

On 8/7/2017 11:54 AM, Shahaf Shuler wrote:
> Introduce a new API to configure Rx offloads.
> 
> The new API will re-use existing DEV_RX_OFFLOAD_* flags
> to enable the different offloads. This will ease the process
> of adding a new Rx offloads, as no ABI breakage is involved.
> In addition, the offload configuration can be done per queue,
> instead of per port.
> 
> The Rx queue offload API can be used only with devices which advertize
> the RTE_ETH_DEV_RXQ_OFFLOAD capability.
> 
> The old Rx offloads API is kept for the meanwhile, in order to enable a
> smooth transition for PMDs and application to the new API.
> 
> Signed-off-by: Shahaf Shuler <shahafs@mellanox.com>

<...>

>  /**
> @@ -691,6 +698,12 @@ struct rte_eth_rxq_conf {
>  	uint16_t rx_free_thresh; /**< Drives the freeing of RX descriptors. */
>  	uint8_t rx_drop_en; /**< Drop packets if no descriptors are available. */
>  	uint8_t rx_deferred_start; /**< Do not start queue with rte_eth_dev_start(). */
> +	uint64_t offloads;

Also there is a documentation aims to help PMDs on developing features
[1], currently for Rx offload features it documents rxmode fields as to
be used.

Can you please update that document too, according new API, new field to
use, both for Rx and Tx.

[1]
doc/guides/nics/features.rst

[RFC PATCH 3/4] ethdev: introduce Tx queue offloads API

[Shahaf Shuler]

Introduce a new API to configure Tx offloads.

The new API will re-use existing DEV_TX_OFFLOAD_* flags
to enable the different offloads. This will ease the process
of adding a new Tx offloads, as no ABI breakage is involved.
In addition, the Tx offloads will be disabled by default and be
enabled per application needs. This will much simplify PMD management of
the different offloads.

The new API does not have an equivalent for the below, benchmark
specific, flags:
ETH_TXQ_FLAGS_NOREFCOUNT
ETH_TXQ_FLAGS_NOMULTMEMP

The Tx queue offload API can be used only with devices which advertize
the RTE_ETH_DEV_TXQ_OFFLOAD capability.

The old Tx offloads API is kept for the meanwhile, in order to enable a
smooth transition for PMDs and application to the new API.

Signed-off-by: Shahaf Shuler <shahafs@mellanox.com>
---
 lib/librte_ether/rte_ethdev.h | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

diff --git a/lib/librte_ether/rte_ethdev.h b/lib/librte_ether/rte_ethdev.h
index ce33c61c4..fd17ee81e 100644
--- a/lib/librte_ether/rte_ethdev.h
+++ b/lib/librte_ether/rte_ethdev.h
@@ -719,6 +719,14 @@ struct rte_eth_rxq_conf {
 #define ETH_TXQ_FLAGS_NOXSUMS \
 		(ETH_TXQ_FLAGS_NOXSUMSCTP | ETH_TXQ_FLAGS_NOXSUMUDP | \
 		 ETH_TXQ_FLAGS_NOXSUMTCP)
+#define ETH_TXQ_FLAGS_IGNORE	0x8000
+	/**
+	 * When set the txq_flags should be ignored,
+	 * instead the Tx offloads will be set on offloads field
+	 * located on rte_eth_txq_conf struct.
+	 * This flag is temporary till the rte_eth_txq_conf.txq_flags
+	 * API will be deprecated.
+	 */
 /**
  * A structure used to configure a TX ring of an Ethernet port.
  */
@@ -730,6 +738,12 @@ struct rte_eth_txq_conf {
 
 	uint32_t txq_flags; /**< Set flags for the Tx queue */
 	uint8_t tx_deferred_start; /**< Do not start queue with rte_eth_dev_start(). */
+	uint64_t offloads;
+	/**
+	 * Enable Tx offloads using DEV_TX_OFFLOAD_* flags.
+	 * Supported only for devices which advertize the
+	 * RTE_ETH_DEV_TXQ_OFFLOAD capability.
+	 */
 };
 
 /**
@@ -955,6 +969,8 @@ struct rte_eth_conf {
 /**< Multiple threads can invoke rte_eth_tx_burst() concurrently on the same
  * tx queue without SW lock.
  */
+#define DEV_TX_OFFLOAD_MULTI_SEGS	0x00008000
+/**< multi segment send is supported. */
 
 struct rte_pci_device;
 
@@ -1751,6 +1767,8 @@ struct rte_eth_dev_data {
 #define RTE_ETH_DEV_INTR_RMV     0x0008
 /** Device supports the rte_eth_rxq_conf offloads API */
 #define RTE_ETH_DEV_RXQ_OFFLOAD 0x0010
+/** Device supports the rte_eth_txq_conf offloads API */
+#define RTE_ETH_DEV_TXQ_OFFLOAD 0x0020
 
 /**
  * @internal
-- 
2.12.0

[RFC PATCH 4/4] ethdev: add helpers to move to the new offloads API

[Shahaf Shuler]

A new offloads API was introduced by commits:

commit 8b07fcae6061 ("ethdev: introduce Tx queue offloads API")
commit c6504557763e ("ethdev: introduce Rx queue offloads API")

In order to enable PMDs to support only one of the APIs,
and applications to avoid branching according to the underlying device
a copy functions to/from the old/new APIs were added.

Signed-off-by: Shahaf Shuler <shahafs@mellanox.com>
---
 lib/librte_ether/rte_ethdev.c | 140 +++++++++++++++++++++++++++++++++++++
 1 file changed, 140 insertions(+)

diff --git a/lib/librte_ether/rte_ethdev.c b/lib/librte_ether/rte_ethdev.c
index f73307e99..2b4a28c97 100644
--- a/lib/librte_ether/rte_ethdev.c
+++ b/lib/librte_ether/rte_ethdev.c
@@ -1003,6 +1003,63 @@ rte_eth_dev_close(uint8_t port_id)
 	dev->data->tx_queues = NULL;
 }
 
+/**
+ * A copy function from rxmode offloads API to rte_eth_rxq_conf
+ * offloads API, to enable PMDs to support only one of the APIs.
+ */
+static void
+rte_eth_copy_rxmode_offloads(struct rte_eth_rxmode *rxmode,
+			     struct rte_eth_rxq_conf *rxq_conf)
+{
+	if (rxmode->header_split == 1)
+		rxq_conf->offloads |= DEV_RX_OFFLOAD_HEADER_SPLIT;
+	if (rxmode->hw_ip_checksum == 1)
+		rxq_conf->offloads |= DEV_RX_OFFLOAD_CHECKSUM;
+	if (rxmode->hw_vlan_filter == 1)
+		rxq_conf->offloads |= DEV_RX_OFFLOAD_VLAN_FILTER;
+	if (rxmode->hw_vlan_strip == 1)
+		rxq_conf->offloads |= DEV_RX_OFFLOAD_VLAN_STRIP;
+	if (rxmode->hw_vlan_extend == 1)
+		rxq_conf->offloads |= DEV_RX_OFFLOAD_VLAN_EXTEND;
+	if (rxmode->jumbo_frame == 1)
+		rxq_conf->offloads |= DEV_RX_OFFLOAD_JUMBO_FRAME;
+	if (rxmode->hw_strip_crc == 1)
+		rxq_conf->offloads |= DEV_RX_OFFLOAD_CRC_STRIP;
+	if (rxmode->enable_scatter == 1)
+		rxq_conf->offloads |= DEV_RX_OFFLOAD_SCATTER;
+	if (rxmode->enable_lro == 1)
+		rxq_conf->offloads |= DEV_RX_OFFLOAD_LRO;
+}
+
+/**
+ * A copy function between rte_eth_rxq_conf offloads API to rxmode
+ * offloads API, to enable application to be agnostic to the PMD supported
+ * offload API.
+ */
+static void
+rte_eth_copy_rxq_offloads(struct rte_eth_rxmode *rxmode,
+			  struct rte_eth_rxq_conf *rxq_conf)
+{
+	if (rxq_conf->offloads & DEV_RX_OFFLOAD_HEADER_SPLIT)
+		rxmode->header_split = 1;
+	if (rxq_conf->offloads & DEV_RX_OFFLOAD_CHECKSUM)
+		rxmode->hw_ip_checksum = 1;
+	if (rxq_conf->offloads & DEV_RX_OFFLOAD_VLAN_FILTER)
+		rxmode->hw_vlan_filter = 1;
+	if (rxq_conf->offloads & DEV_RX_OFFLOAD_VLAN_STRIP)
+		rxmode->hw_vlan_strip = 1;
+	if (rxq_conf->offloads & DEV_RX_OFFLOAD_VLAN_EXTEND)
+		rxmode->hw_vlan_extend = 1;
+	if (rxq_conf->offloads & DEV_RX_OFFLOAD_JUMBO_FRAME)
+		rxmode->jumbo_frame = 1;
+	if (rxq_conf->offloads & DEV_RX_OFFLOAD_CRC_STRIP)
+		rxmode->hw_strip_crc = 1;
+	if (rxq_conf->offloads & DEV_RX_OFFLOAD_SCATTER)
+		rxmode->enable_scatter = 1;
+	if (rxq_conf->offloads & DEV_RX_OFFLOAD_LRO)
+		rxmode->enable_lro = 1;
+}
+
 int
 rte_eth_rx_queue_setup(uint8_t port_id, uint16_t rx_queue_id,
 		       uint16_t nb_rx_desc, unsigned int socket_id,
@@ -1083,6 +1140,37 @@ rte_eth_rx_queue_setup(uint8_t port_id, uint16_t rx_queue_id,
 	if (rx_conf == NULL)
 		rx_conf = &dev_info.default_rxconf;
 
+	if ((dev->data->dev_flags & RTE_ETH_DEV_RXQ_OFFLOAD) &&
+	    (dev->data->dev_conf.rxmode.ignore == 0)) {
+		rte_eth_copy_rxmode_offloads(&dev->data->dev_conf.rxmode,
+					     rx_conf);
+	} else if ((!(dev->data->dev_flags & RTE_ETH_DEV_RXQ_OFFLOAD)) &&
+		   (dev->data->dev_conf.rxmode.ignore == 1)) {
+		int ret;
+		struct rte_eth_rxmode rxmode;
+
+		rte_eth_copy_rxq_offloads(&rxmode, rx_conf);
+		if (memcmp(&rxmode, &dev->data->dev_conf.rxmode,
+			   sizeof(rxmode))) {
+			/*
+			 * device which work with rxmode offloads API requires
+			 * a re-configuration in order to apply the new offloads
+			 * configuration.
+			 */
+			dev->data->dev_conf.rxmode = rxmode;
+			ret = rte_eth_dev_configure(port_id,
+					dev->data->nb_rx_queues,
+					dev->data->nb_tx_queues,
+					&dev->data->dev_conf);
+			if (ret < 0) {
+				RTE_PMD_DEBUG_TRACE(
+					"unable to re-configure port %d "
+					"in order to apply rxq offloads "
+					"configuration\n", port_id);
+			}
+		}
+	}
+
 	ret = (*dev->dev_ops->rx_queue_setup)(dev, rx_queue_id, nb_rx_desc,
 					      socket_id, rx_conf, mp);
 	if (!ret) {
@@ -1094,6 +1182,51 @@ rte_eth_rx_queue_setup(uint8_t port_id, uint16_t rx_queue_id,
 	return ret;
 }
 
+/**
+ * A copy function from txq_flags to rte_eth_txq_conf offloads API,
+ * to enable PMDs to support only one of the APIs.
+ */
+static void
+rte_eth_copy_txq_flags(struct rte_eth_txq_conf *txq_conf)
+{
+	uint32_t txq_flags = txq_conf->txq_flags
+	uint64_t *offloads = &txq_conf->offloads;
+
+	if (!(txq_flags & ETH_TXQ_FLAGS_NOMULTSEGS))
+		*offloads |= DEV_TX_OFFLOAD_MULTI_SEGS;
+	if (!(txq_flags & ETH_TXQ_FLAGS_NOVLANOFFL))
+		*offloads |= DEV_TX_OFFLOAD_VLAN_INSERT;
+	if (!(txq_flags & ETH_TXQ_FLAGS_NOXSUMSCTP))
+		*offloads |= DEV_TX_OFFLOAD_SCTP_CKSUM;
+	if (!(txq_flags & ETH_TXQ_FLAGS_NOXSUMUDP))
+		*offloads |= DEV_TX_OFFLOAD_UDP_CKSUM;
+	if (!(txq_flags & ETH_TXQ_FLAGS_NOXSUMTCP))
+		*offloads |= DEV_TX_OFFLOAD_TCP_CKSUM;
+}
+
+/**
+ * A copy function between rte_eth_txq_conf offloads API to txq_flags
+ * offloads API, to enable application to be agnostic to the PMD supported
+ * API.
+ */
+static void
+rte_eth_copy_txq_offloads(struct rte_eth_txq_conf *txq_conf)
+{
+	uint32_t *txq_flags = &txq_conf->txq_flags
+	uint64_t offloads = txq_conf->offloads;
+
+	if (!(offloads & DEV_TX_OFFLOAD_MULTI_SEGS))
+		*txq_flags |= ETH_TXQ_FLAGS_NOMULTSEGS;
+	if (!(offloads & DEV_TX_OFFLOAD_VLAN_INSERT))
+		*txq_flags |= ETH_TXQ_FLAGS_NOVLANOFFL;
+	if (!(offloads & DEV_TX_OFFLOAD_SCTP_CKSUM))
+		*txq_flags |= ETH_TXQ_FLAGS_NOXSUMSCTP;
+	if (!(offloads & DEV_TX_OFFLOAD_UDP_CKSUM))
+		*txq_flags |= ETH_TXQ_FLAGS_NOXSUMUDP;
+	if (!(offloads & DEV_TX_OFFLOAD_TCP_CKSUM))
+		*txq_flags |= ETH_TXQ_FLAGS_NOXSUMTCP;
+}
+
 int
 rte_eth_tx_queue_setup(uint8_t port_id, uint16_t tx_queue_id,
 		       uint16_t nb_tx_desc, unsigned int socket_id,
@@ -1145,6 +1278,13 @@ rte_eth_tx_queue_setup(uint8_t port_id, uint16_t tx_queue_id,
 	if (tx_conf == NULL)
 		tx_conf = &dev_info.default_txconf;
 
+	if ((dev->data->dev_flags & RTE_ETH_DEV_TXQ_OFFLOAD) &&
+	    (!(tx_conf->txq_flags & ETH_TXQ_FLAGS_IGNORE)))
+		rte_eth_copy_txq_flags(tx_conf);
+	else if (!(dev->data->dev_flags & RTE_ETH_DEV_TXQ_OFFLOAD) &&
+		   (tx_conf->txq_flags & ETH_TXQ_FLAGS_IGNORE))
+		rte_eth_copy_txq_offloads(tx_conf);
+
 	return (*dev->dev_ops->tx_queue_setup)(dev, tx_queue_id, nb_tx_desc,
 					       socket_id, tx_conf);
 }
-- 
2.12.0

Re: [RFC PATCH 4/4] ethdev: add helpers to move to the new offloads API

[Ananyev, Konstantin]

> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Shahaf Shuler
> Sent: Monday, August 7, 2017 1:55 PM
> To: dev@dpdk.org
> Subject: [dpdk-dev] [RFC PATCH 4/4] ethdev: add helpers to move to the new offloads API
> 
> A new offloads API was introduced by commits:
> 
> commit 8b07fcae6061 ("ethdev: introduce Tx queue offloads API")
> commit c6504557763e ("ethdev: introduce Rx queue offloads API")
> 
> In order to enable PMDs to support only one of the APIs,
> and applications to avoid branching according to the underlying device
> a copy functions to/from the old/new APIs were added.
> 
> Signed-off-by: Shahaf Shuler <shahafs@mellanox.com>
> ---
>  lib/librte_ether/rte_ethdev.c | 140 +++++++++++++++++++++++++++++++++++++
>  1 file changed, 140 insertions(+)
> 
> diff --git a/lib/librte_ether/rte_ethdev.c b/lib/librte_ether/rte_ethdev.c
> index f73307e99..2b4a28c97 100644
> --- a/lib/librte_ether/rte_ethdev.c
> +++ b/lib/librte_ether/rte_ethdev.c
> @@ -1003,6 +1003,63 @@ rte_eth_dev_close(uint8_t port_id)
>  	dev->data->tx_queues = NULL;
>  }
> 
> +/**
> + * A copy function from rxmode offloads API to rte_eth_rxq_conf
> + * offloads API, to enable PMDs to support only one of the APIs.
> + */
> +static void
> +rte_eth_copy_rxmode_offloads(struct rte_eth_rxmode *rxmode,
> +			     struct rte_eth_rxq_conf *rxq_conf)
> +{
> +	if (rxmode->header_split == 1)
> +		rxq_conf->offloads |= DEV_RX_OFFLOAD_HEADER_SPLIT;
> +	if (rxmode->hw_ip_checksum == 1)
> +		rxq_conf->offloads |= DEV_RX_OFFLOAD_CHECKSUM;
> +	if (rxmode->hw_vlan_filter == 1)
> +		rxq_conf->offloads |= DEV_RX_OFFLOAD_VLAN_FILTER;
> +	if (rxmode->hw_vlan_strip == 1)
> +		rxq_conf->offloads |= DEV_RX_OFFLOAD_VLAN_STRIP;
> +	if (rxmode->hw_vlan_extend == 1)
> +		rxq_conf->offloads |= DEV_RX_OFFLOAD_VLAN_EXTEND;
> +	if (rxmode->jumbo_frame == 1)
> +		rxq_conf->offloads |= DEV_RX_OFFLOAD_JUMBO_FRAME;
> +	if (rxmode->hw_strip_crc == 1)
> +		rxq_conf->offloads |= DEV_RX_OFFLOAD_CRC_STRIP;
> +	if (rxmode->enable_scatter == 1)
> +		rxq_conf->offloads |= DEV_RX_OFFLOAD_SCATTER;
> +	if (rxmode->enable_lro == 1)
> +		rxq_conf->offloads |= DEV_RX_OFFLOAD_LRO;
> +}
> +
> +/**
> + * A copy function between rte_eth_rxq_conf offloads API to rxmode
> + * offloads API, to enable application to be agnostic to the PMD supported
> + * offload API.
> + */
> +static void
> +rte_eth_copy_rxq_offloads(struct rte_eth_rxmode *rxmode,
> +			  struct rte_eth_rxq_conf *rxq_conf)
> +{
> +	if (rxq_conf->offloads & DEV_RX_OFFLOAD_HEADER_SPLIT)
> +		rxmode->header_split = 1;
> +	if (rxq_conf->offloads & DEV_RX_OFFLOAD_CHECKSUM)
> +		rxmode->hw_ip_checksum = 1;
> +	if (rxq_conf->offloads & DEV_RX_OFFLOAD_VLAN_FILTER)
> +		rxmode->hw_vlan_filter = 1;
> +	if (rxq_conf->offloads & DEV_RX_OFFLOAD_VLAN_STRIP)
> +		rxmode->hw_vlan_strip = 1;
> +	if (rxq_conf->offloads & DEV_RX_OFFLOAD_VLAN_EXTEND)
> +		rxmode->hw_vlan_extend = 1;
> +	if (rxq_conf->offloads & DEV_RX_OFFLOAD_JUMBO_FRAME)
> +		rxmode->jumbo_frame = 1;
> +	if (rxq_conf->offloads & DEV_RX_OFFLOAD_CRC_STRIP)
> +		rxmode->hw_strip_crc = 1;
> +	if (rxq_conf->offloads & DEV_RX_OFFLOAD_SCATTER)
> +		rxmode->enable_scatter = 1;
> +	if (rxq_conf->offloads & DEV_RX_OFFLOAD_LRO)
> +		rxmode->enable_lro = 1;
> +}
> +
>  int
>  rte_eth_rx_queue_setup(uint8_t port_id, uint16_t rx_queue_id,
>  		       uint16_t nb_rx_desc, unsigned int socket_id,
> @@ -1083,6 +1140,37 @@ rte_eth_rx_queue_setup(uint8_t port_id, uint16_t rx_queue_id,
>  	if (rx_conf == NULL)
>  		rx_conf = &dev_info.default_rxconf;
> 
> +	if ((dev->data->dev_flags & RTE_ETH_DEV_RXQ_OFFLOAD) &&
> +	    (dev->data->dev_conf.rxmode.ignore == 0)) {
> +		rte_eth_copy_rxmode_offloads(&dev->data->dev_conf.rxmode,
> +					     rx_conf);
> +	} else if ((!(dev->data->dev_flags & RTE_ETH_DEV_RXQ_OFFLOAD)) &&
> +		   (dev->data->dev_conf.rxmode.ignore == 1)) {
> +		int ret;
> +		struct rte_eth_rxmode rxmode;
> +
> +		rte_eth_copy_rxq_offloads(&rxmode, rx_conf);
> +		if (memcmp(&rxmode, &dev->data->dev_conf.rxmode,
> +			   sizeof(rxmode))) {
> +			/*
> +			 * device which work with rxmode offloads API requires
> +			 * a re-configuration in order to apply the new offloads
> +			 * configuration.
> +			 */
> +			dev->data->dev_conf.rxmode = rxmode;
> +			ret = rte_eth_dev_configure(port_id,
> +					dev->data->nb_rx_queues,
> +					dev->data->nb_tx_queues,
> +					&dev->data->dev_conf);


Hmm, and why we would need to reconfigure our device in the middle of rx queue setup? 

> +			if (ret < 0) {
> +				RTE_PMD_DEBUG_TRACE(
> +					"unable to re-configure port %d "
> +					"in order to apply rxq offloads "
> +					"configuration\n", port_id);
> +			}
> +		}
> +	}
> +
>  	ret = (*dev->dev_ops->rx_queue_setup)(dev, rx_queue_id, nb_rx_desc,
>  					      socket_id, rx_conf, mp);

BTW, I don't see changes in any PMD for new offload flags?
Is it because it is just a n RFC and full patch would contain such changes?


>  	if (!ret) {
> @@ -1094,6 +1182,51 @@ rte_eth_rx_queue_setup(uint8_t port_id, uint16_t rx_queue_id,
>  	return ret;
>  }
> 
> +/**
> + * A copy function from txq_flags to rte_eth_txq_conf offloads API,
> + * to enable PMDs to support only one of the APIs.
> + */
> +static void
> +rte_eth_copy_txq_flags(struct rte_eth_txq_conf *txq_conf)
> +{
> +	uint32_t txq_flags = txq_conf->txq_flags
> +	uint64_t *offloads = &txq_conf->offloads;
> +
> +	if (!(txq_flags & ETH_TXQ_FLAGS_NOMULTSEGS))
> +		*offloads |= DEV_TX_OFFLOAD_MULTI_SEGS;
> +	if (!(txq_flags & ETH_TXQ_FLAGS_NOVLANOFFL))
> +		*offloads |= DEV_TX_OFFLOAD_VLAN_INSERT;
> +	if (!(txq_flags & ETH_TXQ_FLAGS_NOXSUMSCTP))
> +		*offloads |= DEV_TX_OFFLOAD_SCTP_CKSUM;
> +	if (!(txq_flags & ETH_TXQ_FLAGS_NOXSUMUDP))
> +		*offloads |= DEV_TX_OFFLOAD_UDP_CKSUM;
> +	if (!(txq_flags & ETH_TXQ_FLAGS_NOXSUMTCP))
> +		*offloads |= DEV_TX_OFFLOAD_TCP_CKSUM;
> +}
> +
> +/**
> + * A copy function between rte_eth_txq_conf offloads API to txq_flags
> + * offloads API, to enable application to be agnostic to the PMD supported
> + * API.
> + */
> +static void
> +rte_eth_copy_txq_offloads(struct rte_eth_txq_conf *txq_conf)
> +{
> +	uint32_t *txq_flags = &txq_conf->txq_flags
> +	uint64_t offloads = txq_conf->offloads;
> +
> +	if (!(offloads & DEV_TX_OFFLOAD_MULTI_SEGS))
> +		*txq_flags |= ETH_TXQ_FLAGS_NOMULTSEGS;
> +	if (!(offloads & DEV_TX_OFFLOAD_VLAN_INSERT))
> +		*txq_flags |= ETH_TXQ_FLAGS_NOVLANOFFL;
> +	if (!(offloads & DEV_TX_OFFLOAD_SCTP_CKSUM))
> +		*txq_flags |= ETH_TXQ_FLAGS_NOXSUMSCTP;
> +	if (!(offloads & DEV_TX_OFFLOAD_UDP_CKSUM))
> +		*txq_flags |= ETH_TXQ_FLAGS_NOXSUMUDP;
> +	if (!(offloads & DEV_TX_OFFLOAD_TCP_CKSUM))
> +		*txq_flags |= ETH_TXQ_FLAGS_NOXSUMTCP;
> +}
> +
>  int
>  rte_eth_tx_queue_setup(uint8_t port_id, uint16_t tx_queue_id,
>  		       uint16_t nb_tx_desc, unsigned int socket_id,
> @@ -1145,6 +1278,13 @@ rte_eth_tx_queue_setup(uint8_t port_id, uint16_t tx_queue_id,
>  	if (tx_conf == NULL)
>  		tx_conf = &dev_info.default_txconf;
> 
> +	if ((dev->data->dev_flags & RTE_ETH_DEV_TXQ_OFFLOAD) &&
> +	    (!(tx_conf->txq_flags & ETH_TXQ_FLAGS_IGNORE)))
> +		rte_eth_copy_txq_flags(tx_conf);
> +	else if (!(dev->data->dev_flags & RTE_ETH_DEV_TXQ_OFFLOAD) &&
> +		   (tx_conf->txq_flags & ETH_TXQ_FLAGS_IGNORE))
> +		rte_eth_copy_txq_offloads(tx_conf);
> +

As I said in my previous mail - I think better to always convrert from old txq_flags
to new TX offload flags and make each PMD to understand new offload values only.
Konstantin

>  	return (*dev->dev_ops->tx_queue_setup)(dev, tx_queue_id, nb_tx_desc,
>  					       socket_id, tx_conf);
>  }
> --
> 2.12.0

Re: [RFC PATCH 4/4] ethdev: add helpers to move to the new offloads API

[Shahaf Shuler]

Wednesday, August 23, 2017 3:29 PM, Ananyev, Konstantin:
> 
> 
> > -----Original Message-----
> > From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Shahaf Shuler
> > Sent: Monday, August 7, 2017 1:55 PM
> > To: dev@dpdk.org
> > Subject: [dpdk-dev] [RFC PATCH 4/4] ethdev: add helpers to move to the
> > new offloads API
> >
> > A new offloads API was introduced by commits:
> >
> > commit 8b07fcae6061 ("ethdev: introduce Tx queue offloads API") commit
> > c6504557763e ("ethdev: introduce Rx queue offloads API")
> >
> > In order to enable PMDs to support only one of the APIs, and
> > applications to avoid branching according to the underlying device a
> > copy functions to/from the old/new APIs were added.
> >
> > Signed-off-by: Shahaf Shuler <shahafs@mellanox.com>
> > ---
> >  lib/librte_ether/rte_ethdev.c | 140
> > +++++++++++++++++++++++++++++++++++++
> >  1 file changed, 140 insertions(+)
> >
> > diff --git a/lib/librte_ether/rte_ethdev.c
> > b/lib/librte_ether/rte_ethdev.c index f73307e99..2b4a28c97 100644
> > --- a/lib/librte_ether/rte_ethdev.c
> > +++ b/lib/librte_ether/rte_ethdev.c
> > @@ -1003,6 +1003,63 @@ rte_eth_dev_close(uint8_t port_id)
> >  	dev->data->tx_queues = NULL;
> >  }
> >
> > +/**
> > + * A copy function from rxmode offloads API to rte_eth_rxq_conf
> > + * offloads API, to enable PMDs to support only one of the APIs.
> > + */
> > +static void
> > +rte_eth_copy_rxmode_offloads(struct rte_eth_rxmode *rxmode,
> > +			     struct rte_eth_rxq_conf *rxq_conf) {
> > +	if (rxmode->header_split == 1)
> > +		rxq_conf->offloads |= DEV_RX_OFFLOAD_HEADER_SPLIT;
> > +	if (rxmode->hw_ip_checksum == 1)
> > +		rxq_conf->offloads |= DEV_RX_OFFLOAD_CHECKSUM;
> > +	if (rxmode->hw_vlan_filter == 1)
> > +		rxq_conf->offloads |= DEV_RX_OFFLOAD_VLAN_FILTER;
> > +	if (rxmode->hw_vlan_strip == 1)
> > +		rxq_conf->offloads |= DEV_RX_OFFLOAD_VLAN_STRIP;
> > +	if (rxmode->hw_vlan_extend == 1)
> > +		rxq_conf->offloads |= DEV_RX_OFFLOAD_VLAN_EXTEND;
> > +	if (rxmode->jumbo_frame == 1)
> > +		rxq_conf->offloads |= DEV_RX_OFFLOAD_JUMBO_FRAME;
> > +	if (rxmode->hw_strip_crc == 1)
> > +		rxq_conf->offloads |= DEV_RX_OFFLOAD_CRC_STRIP;
> > +	if (rxmode->enable_scatter == 1)
> > +		rxq_conf->offloads |= DEV_RX_OFFLOAD_SCATTER;
> > +	if (rxmode->enable_lro == 1)
> > +		rxq_conf->offloads |= DEV_RX_OFFLOAD_LRO; }
> > +
> > +/**
> > + * A copy function between rte_eth_rxq_conf offloads API to rxmode
> > + * offloads API, to enable application to be agnostic to the PMD
> > +supported
> > + * offload API.
> > + */
> > +static void
> > +rte_eth_copy_rxq_offloads(struct rte_eth_rxmode *rxmode,
> > +			  struct rte_eth_rxq_conf *rxq_conf) {
> > +	if (rxq_conf->offloads & DEV_RX_OFFLOAD_HEADER_SPLIT)
> > +		rxmode->header_split = 1;
> > +	if (rxq_conf->offloads & DEV_RX_OFFLOAD_CHECKSUM)
> > +		rxmode->hw_ip_checksum = 1;
> > +	if (rxq_conf->offloads & DEV_RX_OFFLOAD_VLAN_FILTER)
> > +		rxmode->hw_vlan_filter = 1;
> > +	if (rxq_conf->offloads & DEV_RX_OFFLOAD_VLAN_STRIP)
> > +		rxmode->hw_vlan_strip = 1;
> > +	if (rxq_conf->offloads & DEV_RX_OFFLOAD_VLAN_EXTEND)
> > +		rxmode->hw_vlan_extend = 1;
> > +	if (rxq_conf->offloads & DEV_RX_OFFLOAD_JUMBO_FRAME)
> > +		rxmode->jumbo_frame = 1;
> > +	if (rxq_conf->offloads & DEV_RX_OFFLOAD_CRC_STRIP)
> > +		rxmode->hw_strip_crc = 1;
> > +	if (rxq_conf->offloads & DEV_RX_OFFLOAD_SCATTER)
> > +		rxmode->enable_scatter = 1;
> > +	if (rxq_conf->offloads & DEV_RX_OFFLOAD_LRO)
> > +		rxmode->enable_lro = 1;
> > +}
> > +
> >  int
> >  rte_eth_rx_queue_setup(uint8_t port_id, uint16_t rx_queue_id,
> >  		       uint16_t nb_rx_desc, unsigned int socket_id, @@ -1083,6
> > +1140,37 @@ rte_eth_rx_queue_setup(uint8_t port_id, uint16_t
> rx_queue_id,
> >  	if (rx_conf == NULL)
> >  		rx_conf = &dev_info.default_rxconf;
> >
> > +	if ((dev->data->dev_flags & RTE_ETH_DEV_RXQ_OFFLOAD) &&
> > +	    (dev->data->dev_conf.rxmode.ignore == 0)) {
> > +		rte_eth_copy_rxmode_offloads(&dev->data-
> >dev_conf.rxmode,
> > +					     rx_conf);
> > +	} else if ((!(dev->data->dev_flags & RTE_ETH_DEV_RXQ_OFFLOAD))
> &&
> > +		   (dev->data->dev_conf.rxmode.ignore == 1)) {
> > +		int ret;
> > +		struct rte_eth_rxmode rxmode;
> > +
> > +		rte_eth_copy_rxq_offloads(&rxmode, rx_conf);
> > +		if (memcmp(&rxmode, &dev->data->dev_conf.rxmode,
> > +			   sizeof(rxmode))) {
> > +			/*
> > +			 * device which work with rxmode offloads API
> requires
> > +			 * a re-configuration in order to apply the new
> offloads
> > +			 * configuration.
> > +			 */
> > +			dev->data->dev_conf.rxmode = rxmode;
> > +			ret = rte_eth_dev_configure(port_id,
> > +					dev->data->nb_rx_queues,
> > +					dev->data->nb_tx_queues,
> > +					&dev->data->dev_conf);
> 
> 
> Hmm, and why we would need to reconfigure our device in the middle of rx
> queue setup?

The reason is the old Rx offloads API is configured on device configure. 
This if section is for applications which already moved to the new offload API however the underlying PMD still uses the old one.

> 
> > +			if (ret < 0) {
> > +				RTE_PMD_DEBUG_TRACE(
> > +					"unable to re-configure port %d "
> > +					"in order to apply rxq offloads "
> > +					"configuration\n", port_id);
> > +			}
> > +		}
> > +	}
> > +
> >  	ret = (*dev->dev_ops->rx_queue_setup)(dev, rx_queue_id,
> nb_rx_desc,
> >  					      socket_id, rx_conf, mp);
> 
> BTW, I don't see changes in any PMD for new offload flags?
> Is it because it is just a n RFC and full patch would contain such changes?

Yes this is because this is an RFC.
 
The full patch I intend will move all examples and testpmd to the new offloads API.
In addition it will include the mlx5 PMD support for the new offloads API. 

As I said on previous mail, I believe that the work to move the different PMDs to the new API should be done by their developers or maintainers.

> 
> 
> >  	if (!ret) {
> > @@ -1094,6 +1182,51 @@ rte_eth_rx_queue_setup(uint8_t port_id,
> uint16_t rx_queue_id,
> >  	return ret;
> >  }
> >
> > +/**
> > + * A copy function from txq_flags to rte_eth_txq_conf offloads API,
> > + * to enable PMDs to support only one of the APIs.
> > + */
> > +static void
> > +rte_eth_copy_txq_flags(struct rte_eth_txq_conf *txq_conf) {
> > +	uint32_t txq_flags = txq_conf->txq_flags
> > +	uint64_t *offloads = &txq_conf->offloads;
> > +
> > +	if (!(txq_flags & ETH_TXQ_FLAGS_NOMULTSEGS))
> > +		*offloads |= DEV_TX_OFFLOAD_MULTI_SEGS;
> > +	if (!(txq_flags & ETH_TXQ_FLAGS_NOVLANOFFL))
> > +		*offloads |= DEV_TX_OFFLOAD_VLAN_INSERT;
> > +	if (!(txq_flags & ETH_TXQ_FLAGS_NOXSUMSCTP))
> > +		*offloads |= DEV_TX_OFFLOAD_SCTP_CKSUM;
> > +	if (!(txq_flags & ETH_TXQ_FLAGS_NOXSUMUDP))
> > +		*offloads |= DEV_TX_OFFLOAD_UDP_CKSUM;
> > +	if (!(txq_flags & ETH_TXQ_FLAGS_NOXSUMTCP))
> > +		*offloads |= DEV_TX_OFFLOAD_TCP_CKSUM; }
> > +
> > +/**
> > + * A copy function between rte_eth_txq_conf offloads API to txq_flags
> > + * offloads API, to enable application to be agnostic to the PMD
> > +supported
> > + * API.
> > + */
> > +static void
> > +rte_eth_copy_txq_offloads(struct rte_eth_txq_conf *txq_conf) {
> > +	uint32_t *txq_flags = &txq_conf->txq_flags
> > +	uint64_t offloads = txq_conf->offloads;
> > +
> > +	if (!(offloads & DEV_TX_OFFLOAD_MULTI_SEGS))
> > +		*txq_flags |= ETH_TXQ_FLAGS_NOMULTSEGS;
> > +	if (!(offloads & DEV_TX_OFFLOAD_VLAN_INSERT))
> > +		*txq_flags |= ETH_TXQ_FLAGS_NOVLANOFFL;
> > +	if (!(offloads & DEV_TX_OFFLOAD_SCTP_CKSUM))
> > +		*txq_flags |= ETH_TXQ_FLAGS_NOXSUMSCTP;
> > +	if (!(offloads & DEV_TX_OFFLOAD_UDP_CKSUM))
> > +		*txq_flags |= ETH_TXQ_FLAGS_NOXSUMUDP;
> > +	if (!(offloads & DEV_TX_OFFLOAD_TCP_CKSUM))
> > +		*txq_flags |= ETH_TXQ_FLAGS_NOXSUMTCP; }
> > +
> >  int
> >  rte_eth_tx_queue_setup(uint8_t port_id, uint16_t tx_queue_id,
> >  		       uint16_t nb_tx_desc, unsigned int socket_id, @@ -1145,6
> > +1278,13 @@ rte_eth_tx_queue_setup(uint8_t port_id, uint16_t
> tx_queue_id,
> >  	if (tx_conf == NULL)
> >  		tx_conf = &dev_info.default_txconf;
> >
> > +	if ((dev->data->dev_flags & RTE_ETH_DEV_TXQ_OFFLOAD) &&
> > +	    (!(tx_conf->txq_flags & ETH_TXQ_FLAGS_IGNORE)))
> > +		rte_eth_copy_txq_flags(tx_conf);
> > +	else if (!(dev->data->dev_flags & RTE_ETH_DEV_TXQ_OFFLOAD) &&
> > +		   (tx_conf->txq_flags & ETH_TXQ_FLAGS_IGNORE))
> > +		rte_eth_copy_txq_offloads(tx_conf);
> > +
> 
> As I said in my previous mail - I think better to always convrert from old
> txq_flags to new TX offload flags and make each PMD to understand new
> offload values only.
> Konstantin
> 
> >  	return (*dev->dev_ops->tx_queue_setup)(dev, tx_queue_id,
> nb_tx_desc,
> >  					       socket_id, tx_conf);
> >  }
> > --
> > 2.12.0

Re: [RFC PATCH 4/4] ethdev: add helpers to move to the new offloads API

[Thomas Monjalon]

23/08/2017 15:13, Shahaf Shuler:
> Wednesday, August 23, 2017 3:29 PM, Ananyev, Konstantin:
> > From: Shahaf Shuler
> > > In order to enable PMDs to support only one of the APIs, and
> > > applications to avoid branching according to the underlying device a
> > > copy functions to/from the old/new APIs were added.

Looks a good intent.
I would prefer the word "convert" instead of "copy".

> > >  int
> > >  rte_eth_rx_queue_setup(uint8_t port_id, uint16_t rx_queue_id,
[...]
> > > +	} else if ((!(dev->data->dev_flags & RTE_ETH_DEV_RXQ_OFFLOAD)) &&
> > > +		   (dev->data->dev_conf.rxmode.ignore == 1)) {
> > > +		int ret;
> > > +		struct rte_eth_rxmode rxmode;
> > > +
> > > +		rte_eth_copy_rxq_offloads(&rxmode, rx_conf);
> > > +		if (memcmp(&rxmode, &dev->data->dev_conf.rxmode,
> > > +			   sizeof(rxmode))) {
> > > +			/*
> > > +			 * device which work with rxmode offloads API requires
> > > +			 * a re-configuration in order to apply the new offloads
> > > +			 * configuration.
> > > +			 */
> > > +			dev->data->dev_conf.rxmode = rxmode;
> > > +			ret = rte_eth_dev_configure(port_id,
> > > +					dev->data->nb_rx_queues,
> > > +					dev->data->nb_tx_queues,
> > > +					&dev->data->dev_conf);
> > 
> > Hmm, and why we would need to reconfigure our device in the middle of rx
> > queue setup?
> 
> The reason is the old Rx offloads API is configured on device configure. 
> This if section is for applications which already moved to the new offload
> API however the underlying PMD still uses the old one.

Isn't it risky to re-run configure here?
We could also declare this case as an error.

I think applications which have migrated to the new API,
could use the convert functions themselves before calling configure
to support not migrated PMDs.
The cons of my solution are:
- discourage apps to migrate before all PMDs have migrated
- expose a temporary function to convert API
I propose it anyway because there is always someone to like bad ideas ;)

Re: [RFC PATCH 4/4] ethdev: add helpers to move to the new offloads API

[Shahaf Shuler]

Thursday, August 24, 2017 1:06 AM, Thomas Monjalon:
> 23/08/2017 15:13, Shahaf Shuler:
> > Wednesday, August 23, 2017 3:29 PM, Ananyev, Konstantin:
> > > From: Shahaf Shuler
> > > > In order to enable PMDs to support only one of the APIs, and
> > > > applications to avoid branching according to the underlying device
> > > > a copy functions to/from the old/new APIs were added.
> 
> Looks a good intent.
> I would prefer the word "convert" instead of "copy".
> 
> > > >  int
> > > >  rte_eth_rx_queue_setup(uint8_t port_id, uint16_t rx_queue_id,
> [...]
> > > > +	} else if ((!(dev->data->dev_flags & RTE_ETH_DEV_RXQ_OFFLOAD))
> &&
> > > > +		   (dev->data->dev_conf.rxmode.ignore == 1)) {
> > > > +		int ret;
> > > > +		struct rte_eth_rxmode rxmode;
> > > > +
> > > > +		rte_eth_copy_rxq_offloads(&rxmode, rx_conf);
> > > > +		if (memcmp(&rxmode, &dev->data->dev_conf.rxmode,
> > > > +			   sizeof(rxmode))) {
> > > > +			/*
> > > > +			 * device which work with rxmode offloads API
> requires
> > > > +			 * a re-configuration in order to apply the new
> offloads
> > > > +			 * configuration.
> > > > +			 */
> > > > +			dev->data->dev_conf.rxmode = rxmode;
> > > > +			ret = rte_eth_dev_configure(port_id,
> > > > +					dev->data->nb_rx_queues,
> > > > +					dev->data->nb_tx_queues,
> > > > +					&dev->data->dev_conf);
> > >
> > > Hmm, and why we would need to reconfigure our device in the middle
> > > of rx queue setup?
> >
> > The reason is the old Rx offloads API is configured on device configure.
> > This if section is for applications which already moved to the new
> > offload API however the underlying PMD still uses the old one.
> 
> Isn't it risky to re-run configure here?
> We could also declare this case as an error.
> 
> I think applications which have migrated to the new API, could use the
> convert functions themselves before calling configure to support not
> migrated PMDs.
> The cons of my solution are:
> - discourage apps to migrate before all PMDs have migrated
> - expose a temporary function to convert API I propose it anyway because
> there is always someone to like bad ideas ;)

Yes. I tried to make it as simple as possible for application to move to the new API.
Defining it as error flow, will enforce the application to check the PMD offload mode and branch accordingly. The conversion functions are a good helpers, yet the code remains complex due to the different cases with the different PMDs.

Considering the re-configuration is risky, and without other ideas I will need to fall back to the error flow case.
Are we OK with that?

Re: [RFC PATCH 4/4] ethdev: add helpers to move to the new offloads API

[Thomas Monjalon]

24/08/2017 09:12, Shahaf Shuler:
> Thursday, August 24, 2017 1:06 AM, Thomas Monjalon:
> > 23/08/2017 15:13, Shahaf Shuler:
> > > Wednesday, August 23, 2017 3:29 PM, Ananyev, Konstantin:
> > > > From: Shahaf Shuler
> > > > > In order to enable PMDs to support only one of the APIs, and
> > > > > applications to avoid branching according to the underlying device
> > > > > a copy functions to/from the old/new APIs were added.
> > 
> > Looks a good intent.
> > I would prefer the word "convert" instead of "copy".
> > 
> > > > >  int
> > > > >  rte_eth_rx_queue_setup(uint8_t port_id, uint16_t rx_queue_id,
> > [...]
> > > > > +	} else if ((!(dev->data->dev_flags & RTE_ETH_DEV_RXQ_OFFLOAD)) &&
> > > > > +		   (dev->data->dev_conf.rxmode.ignore == 1)) {
> > > > > +		int ret;
> > > > > +		struct rte_eth_rxmode rxmode;
> > > > > +
> > > > > +		rte_eth_copy_rxq_offloads(&rxmode, rx_conf);
> > > > > +		if (memcmp(&rxmode, &dev->data->dev_conf.rxmode,
> > > > > +			   sizeof(rxmode))) {
> > > > > +			/*
> > > > > +			 * device which work with rxmode offloads API requires
> > > > > +			 * a re-configuration in order to apply the new offloads
> > > > > +			 * configuration.
> > > > > +			 */
> > > > > +			dev->data->dev_conf.rxmode = rxmode;
> > > > > +			ret = rte_eth_dev_configure(port_id,
> > > > > +					dev->data->nb_rx_queues,
> > > > > +					dev->data->nb_tx_queues,
> > > > > +					&dev->data->dev_conf);
> > > >
> > > > Hmm, and why we would need to reconfigure our device in the middle
> > > > of rx queue setup?
> > >
> > > The reason is the old Rx offloads API is configured on device configure.
> > > This if section is for applications which already moved to the new
> > > offload API however the underlying PMD still uses the old one.
> > 
> > Isn't it risky to re-run configure here?
> > We could also declare this case as an error.
> > 
> > I think applications which have migrated to the new API, could use the
> > convert functions themselves before calling configure to support not
> > migrated PMDs.
> > The cons of my solution are:
> > - discourage apps to migrate before all PMDs have migrated
> > - expose a temporary function to convert API I propose it anyway because
> > there is always someone to like bad ideas ;)
> 
> Yes. I tried to make it as simple as possible for application to move to the new API.
> Defining it as error flow, will enforce the application to check the PMD offload mode and branch accordingly. The conversion functions are a good helpers, yet the code remains complex due to the different cases with the different PMDs.
> 
> Considering the re-configuration is risky, and without other ideas I will need to fall back to the error flow case.
> Are we OK with that?

I think we can take the risk of keeping this call to
rte_eth_dev_configure() in the middle of rte_eth_rx_queue_setup().
In theory it should be acceptable.
If we merge it soon, it can be better tested with every drivers.

Re: [RFC PATCH 4/4] ethdev: add helpers to move to the new offloads API

[Ferruh Yigit]

On 8/25/2017 2:26 PM, Thomas Monjalon wrote:
> 24/08/2017 09:12, Shahaf Shuler:
>> Thursday, August 24, 2017 1:06 AM, Thomas Monjalon:
>>> 23/08/2017 15:13, Shahaf Shuler:
>>>> Wednesday, August 23, 2017 3:29 PM, Ananyev, Konstantin:
>>>>> From: Shahaf Shuler
>>>>>> In order to enable PMDs to support only one of the APIs, and
>>>>>> applications to avoid branching according to the underlying device
>>>>>> a copy functions to/from the old/new APIs were added.
>>>
>>> Looks a good intent.
>>> I would prefer the word "convert" instead of "copy".
>>>
>>>>>>  int
>>>>>>  rte_eth_rx_queue_setup(uint8_t port_id, uint16_t rx_queue_id,
>>> [...]
>>>>>> +	} else if ((!(dev->data->dev_flags & RTE_ETH_DEV_RXQ_OFFLOAD)) &&
>>>>>> +		   (dev->data->dev_conf.rxmode.ignore == 1)) {
>>>>>> +		int ret;
>>>>>> +		struct rte_eth_rxmode rxmode;
>>>>>> +
>>>>>> +		rte_eth_copy_rxq_offloads(&rxmode, rx_conf);
>>>>>> +		if (memcmp(&rxmode, &dev->data->dev_conf.rxmode,
>>>>>> +			   sizeof(rxmode))) {
>>>>>> +			/*
>>>>>> +			 * device which work with rxmode offloads API requires
>>>>>> +			 * a re-configuration in order to apply the new offloads
>>>>>> +			 * configuration.
>>>>>> +			 */
>>>>>> +			dev->data->dev_conf.rxmode = rxmode;
>>>>>> +			ret = rte_eth_dev_configure(port_id,
>>>>>> +					dev->data->nb_rx_queues,
>>>>>> +					dev->data->nb_tx_queues,
>>>>>> +					&dev->data->dev_conf);
>>>>>
>>>>> Hmm, and why we would need to reconfigure our device in the middle
>>>>> of rx queue setup?
>>>>
>>>> The reason is the old Rx offloads API is configured on device configure.
>>>> This if section is for applications which already moved to the new
>>>> offload API however the underlying PMD still uses the old one.
>>>
>>> Isn't it risky to re-run configure here?
>>> We could also declare this case as an error.
>>>
>>> I think applications which have migrated to the new API, could use the
>>> convert functions themselves before calling configure to support not
>>> migrated PMDs.
>>> The cons of my solution are:
>>> - discourage apps to migrate before all PMDs have migrated
>>> - expose a temporary function to convert API I propose it anyway because
>>> there is always someone to like bad ideas ;)
>>
>> Yes. I tried to make it as simple as possible for application to move to the new API.
>> Defining it as error flow, will enforce the application to check the PMD offload mode and branch accordingly. The conversion functions are a good helpers, yet the code remains complex due to the different cases with the different PMDs.
>>
>> Considering the re-configuration is risky, and without other ideas I will need to fall back to the error flow case.
>> Are we OK with that?
> 
> I think we can take the risk of keeping this call to
> rte_eth_dev_configure() in the middle of rte_eth_rx_queue_setup().
> In theory it should be acceptable.
> If we merge it soon, it can be better tested with every drivers.

I doubt about taking that risk. Some driver does HW configuration via
configure() and combination of start/stop, setup_queue and configure can
be complex.

I am for generating error for this case.

Generating error also can be good motivation for PMDs to adapt new method.

Re: [RFC PATCH 4/4] ethdev: add helpers to move to the new offloads API

[Shahaf Shuler]

Tuesday, August 29, 2017 3:55 PM, Ferruh Yigit:
> >> Considering the re-configuration is risky, and without other ideas I will
> need to fall back to the error flow case.
> >> Are we OK with that?
> >
> > I think we can take the risk of keeping this call to
> > rte_eth_dev_configure() in the middle of rte_eth_rx_queue_setup().
> > In theory it should be acceptable.
> > If we merge it soon, it can be better tested with every drivers.
> 
> I doubt about taking that risk. Some driver does HW configuration via
> configure() and combination of start/stop, setup_queue and configure can
> be complex.
> 
> I am for generating error for this case.
> 
> Generating error also can be good motivation for PMDs to adapt new
> method.

Adding Ananyev suggestion from other thread:
For tx_prepare() work, we used the following approach:
1. submitted patch with changes in rte_ethdev and PMDs we  are familiar with (Intel ones).
    For other PMDs - patch contained just minimal changes to make it build cleanly.
2. Asked other PMD maintainers to review rte_ethdev changes and provide a proper patch
    for the PMD they own.

So I am OK with both suggestions. Meaning:
1. Define the case were application use the new offloads API with PMD which supports the old one as an error.
2. apply patches to ethdev with the above behavior.

Just to emphasize, it means that PMDs which won't move to the new API by the end of 17.11 will not be able to run with any of the examples and application on DPDK tree (and also with other applications which moved to the new API), as I plan to submit patches which convert them all to the new API.

Any objection to this approach? 

Re: [RFC PATCH 4/4] ethdev: add helpers to move to the new offloads API

[Ferruh Yigit]

On 8/30/2017 7:30 AM, Shahaf Shuler wrote:
> Tuesday, August 29, 2017 3:55 PM, Ferruh Yigit:
>>>> Considering the re-configuration is risky, and without other ideas I will
>> need to fall back to the error flow case.
>>>> Are we OK with that?
>>>
>>> I think we can take the risk of keeping this call to
>>> rte_eth_dev_configure() in the middle of rte_eth_rx_queue_setup().
>>> In theory it should be acceptable.
>>> If we merge it soon, it can be better tested with every drivers.
>>
>> I doubt about taking that risk. Some driver does HW configuration via
>> configure() and combination of start/stop, setup_queue and configure can
>> be complex.
>>
>> I am for generating error for this case.
>>
>> Generating error also can be good motivation for PMDs to adapt new
>> method.
> 
> Adding Ananyev suggestion from other thread:
> For tx_prepare() work, we used the following approach:
> 1. submitted patch with changes in rte_ethdev and PMDs we  are familiar with (Intel ones).
>     For other PMDs - patch contained just minimal changes to make it build cleanly.
> 2. Asked other PMD maintainers to review rte_ethdev changes and provide a proper patch
>     for the PMD they own.

tx_prepare() is a little different, since it was not clear if all PMDs
needs updating that is why asked to PMD owners, and the ones requires
updating already has been updated with ethdev patch. Here we know all
PMDs need updating, and they need proper time in advance.

> 
> So I am OK with both suggestions. Meaning:
> 1. Define the case were application use the new offloads API with PMD which supports the old one as an error.
> 2. apply patches to ethdev with the above behavior.
> 
> Just to emphasize, it means that PMDs which won't move to the new API by the end of 17.11 will not be able to run with any of the examples and application on DPDK tree (and also with other applications which moved to the new API), as I plan to submit patches which convert them all to the new API.

I think it is good idea to update samples/apps to new method, but this
can be short notice for PMD owners.

Can we wait one more release to update samples/apps, to give time for
PMDs to be updated, since old applications will work with new PMDs
(thanks to your helpers), I believe this won't be a problem.

> 
> Any objection to this approach? 
> 
> 

Re: [RFC PATCH 4/4] ethdev: add helpers to move to the new offloads API

[Ananyev, Konstantin]

Hi Ferruh,

> -----Original Message-----
> From: Yigit, Ferruh
> Sent: Wednesday, August 30, 2017 8:51 AM
> To: Shahaf Shuler <shahafs@mellanox.com>; Thomas Monjalon <thomas@monjalon.net>; Ananyev, Konstantin
> <konstantin.ananyev@intel.com>
> Cc: dev@dpdk.org
> Subject: Re: [dpdk-dev] [RFC PATCH 4/4] ethdev: add helpers to move to the new offloads API
> 
> On 8/30/2017 7:30 AM, Shahaf Shuler wrote:
> > Tuesday, August 29, 2017 3:55 PM, Ferruh Yigit:
> >>>> Considering the re-configuration is risky, and without other ideas I will
> >> need to fall back to the error flow case.
> >>>> Are we OK with that?
> >>>
> >>> I think we can take the risk of keeping this call to
> >>> rte_eth_dev_configure() in the middle of rte_eth_rx_queue_setup().
> >>> In theory it should be acceptable.
> >>> If we merge it soon, it can be better tested with every drivers.
> >>
> >> I doubt about taking that risk. Some driver does HW configuration via
> >> configure() and combination of start/stop, setup_queue and configure can
> >> be complex.
> >>
> >> I am for generating error for this case.
> >>
> >> Generating error also can be good motivation for PMDs to adapt new
> >> method.
> >
> > Adding Ananyev suggestion from other thread:
> > For tx_prepare() work, we used the following approach:
> > 1. submitted patch with changes in rte_ethdev and PMDs we  are familiar with (Intel ones).
> >     For other PMDs - patch contained just minimal changes to make it build cleanly.
> > 2. Asked other PMD maintainers to review rte_ethdev changes and provide a proper patch
> >     for the PMD they own.
> 
> tx_prepare() is a little different, since it was not clear if all PMDs
> needs updating that is why asked to PMD owners, and the ones requires
> updating already has been updated with ethdev patch. Here we know all
> PMDs need updating, and they need proper time in advance.
> 
> >
> > So I am OK with both suggestions. Meaning:
> > 1. Define the case were application use the new offloads API with PMD which supports the old one as an error.
> > 2. apply patches to ethdev with the above behavior.
> >
> > Just to emphasize, it means that PMDs which won't move to the new API by the end of 17.11 will not be able to run with any of the
> examples and application on DPDK tree (and also with other applications which moved to the new API), as I plan to submit patches which
> convert them all to the new API.
> 
> I think it is good idea to update samples/apps to new method, but this
> can be short notice for PMD owners.
> 
> Can we wait one more release to update samples/apps, to give time for
> PMDs to be updated, since old applications will work with new PMDs
> (thanks to your helpers), I believe this won't be a problem.

I am not sure what is your suggestion here?
Support both flavors of PMD API for 17.11? 
Konstantin

> 
> >
> > Any objection to this approach?
> >
> >

Re: [RFC PATCH 4/4] ethdev: add helpers to move to the new offloads API

[Ferruh Yigit]

On 8/30/2017 11:16 AM, Ananyev, Konstantin wrote:
> Hi Ferruh,
> 
>> -----Original Message-----
>> From: Yigit, Ferruh
>> Sent: Wednesday, August 30, 2017 8:51 AM
>> To: Shahaf Shuler <shahafs@mellanox.com>; Thomas Monjalon <thomas@monjalon.net>; Ananyev, Konstantin
>> <konstantin.ananyev@intel.com>
>> Cc: dev@dpdk.org
>> Subject: Re: [dpdk-dev] [RFC PATCH 4/4] ethdev: add helpers to move to the new offloads API
>>
>> On 8/30/2017 7:30 AM, Shahaf Shuler wrote:
>>> Tuesday, August 29, 2017 3:55 PM, Ferruh Yigit:
>>>>>> Considering the re-configuration is risky, and without other ideas I will
>>>> need to fall back to the error flow case.
>>>>>> Are we OK with that?
>>>>>
>>>>> I think we can take the risk of keeping this call to
>>>>> rte_eth_dev_configure() in the middle of rte_eth_rx_queue_setup().
>>>>> In theory it should be acceptable.
>>>>> If we merge it soon, it can be better tested with every drivers.
>>>>
>>>> I doubt about taking that risk. Some driver does HW configuration via
>>>> configure() and combination of start/stop, setup_queue and configure can
>>>> be complex.
>>>>
>>>> I am for generating error for this case.
>>>>
>>>> Generating error also can be good motivation for PMDs to adapt new
>>>> method.
>>>
>>> Adding Ananyev suggestion from other thread:
>>> For tx_prepare() work, we used the following approach:
>>> 1. submitted patch with changes in rte_ethdev and PMDs we  are familiar with (Intel ones).
>>>     For other PMDs - patch contained just minimal changes to make it build cleanly.
>>> 2. Asked other PMD maintainers to review rte_ethdev changes and provide a proper patch
>>>     for the PMD they own.
>>
>> tx_prepare() is a little different, since it was not clear if all PMDs
>> needs updating that is why asked to PMD owners, and the ones requires
>> updating already has been updated with ethdev patch. Here we know all
>> PMDs need updating, and they need proper time in advance.
>>
>>>
>>> So I am OK with both suggestions. Meaning:
>>> 1. Define the case were application use the new offloads API with PMD which supports the old one as an error.
>>> 2. apply patches to ethdev with the above behavior.
>>>
>>> Just to emphasize, it means that PMDs which won't move to the new API by the end of 17.11 will not be able to run with any of the
>> examples and application on DPDK tree (and also with other applications which moved to the new API), as I plan to submit patches which
>> convert them all to the new API.
>>
>> I think it is good idea to update samples/apps to new method, but this
>> can be short notice for PMD owners.
>>
>> Can we wait one more release to update samples/apps, to give time for
>> PMDs to be updated, since old applications will work with new PMDs
>> (thanks to your helpers), I believe this won't be a problem.
> 
> I am not sure what is your suggestion here?
> Support both flavors of PMD API for 17.11? 

Support both with an exception, when application uses new method but PMD
uses old one, throw an error (because of above discussed technical issue).

This lets existing applications run without problem, and pushes PMDs to
adapt new method.

Your suggestion to have only new method and convert all PMDs to new
method is good, but I am not sure if this is realistic for this release,
since different PMDs have different pace for updates.

ethdev updates can be done in this release, with the PMDs that already
changed to new method. The sample/app modifications Shahaf mentioned can
be done in the beginning of the next release, and this gives time to
remaining PMDs until end of next release. What do you think?

> Konstantin
> 
>>
>>>
>>> Any objection to this approach?
>>>
>>>
> 

Re: [RFC PATCH 4/4] ethdev: add helpers to move to the new offloads API

[Thomas Monjalon]

30/08/2017 14:42, Ferruh Yigit:
> On 8/30/2017 11:16 AM, Ananyev, Konstantin wrote:
> > Hi Ferruh,
> > From: Yigit, Ferruh
> >> On 8/30/2017 7:30 AM, Shahaf Shuler wrote:
> >>> Tuesday, August 29, 2017 3:55 PM, Ferruh Yigit:
> >>>>>> Considering the re-configuration is risky, and without other ideas I will
> >>>> need to fall back to the error flow case.
> >>>>>> Are we OK with that?
> >>>>>
> >>>>> I think we can take the risk of keeping this call to
> >>>>> rte_eth_dev_configure() in the middle of rte_eth_rx_queue_setup().
> >>>>> In theory it should be acceptable.
> >>>>> If we merge it soon, it can be better tested with every drivers.
> >>>>
> >>>> I doubt about taking that risk. Some driver does HW configuration via
> >>>> configure() and combination of start/stop, setup_queue and configure can
> >>>> be complex.
> >>>>
> >>>> I am for generating error for this case.
> >>>>
> >>>> Generating error also can be good motivation for PMDs to adapt new
> >>>> method.
> >>>
> >>> Adding Ananyev suggestion from other thread:
> >>> For tx_prepare() work, we used the following approach:
> >>> 1. submitted patch with changes in rte_ethdev and PMDs we  are familiar with (Intel ones).
> >>>     For other PMDs - patch contained just minimal changes to make it build cleanly.
> >>> 2. Asked other PMD maintainers to review rte_ethdev changes and provide a proper patch
> >>>     for the PMD they own.
> >>
> >> tx_prepare() is a little different, since it was not clear if all PMDs
> >> needs updating that is why asked to PMD owners, and the ones requires
> >> updating already has been updated with ethdev patch. Here we know all
> >> PMDs need updating, and they need proper time in advance.
> >>
> >>>
> >>> So I am OK with both suggestions. Meaning:
> >>> 1. Define the case were application use the new offloads API with PMD which supports the old one as an error.
> >>> 2. apply patches to ethdev with the above behavior.
> >>>
> >>> Just to emphasize, it means that PMDs which won't move to the new API by the end of 17.11 will not be able to run with any of the
> >> examples and application on DPDK tree (and also with other applications which moved to the new API), as I plan to submit patches which
> >> convert them all to the new API.
> >>
> >> I think it is good idea to update samples/apps to new method, but this
> >> can be short notice for PMD owners.
> >>
> >> Can we wait one more release to update samples/apps, to give time for
> >> PMDs to be updated, since old applications will work with new PMDs
> >> (thanks to your helpers), I believe this won't be a problem.
> > 
> > I am not sure what is your suggestion here?
> > Support both flavors of PMD API for 17.11? 
> 
> Support both with an exception, when application uses new method but PMD
> uses old one, throw an error (because of above discussed technical issue).
> 
> This lets existing applications run without problem, and pushes PMDs to
> adapt new method.
> 
> Your suggestion to have only new method and convert all PMDs to new
> method is good, but I am not sure if this is realistic for this release,
> since different PMDs have different pace for updates.
> 
> ethdev updates can be done in this release, with the PMDs that already
> changed to new method. The sample/app modifications Shahaf mentioned can
> be done in the beginning of the next release, and this gives time to
> remaining PMDs until end of next release. What do you think?

I think it is a reasonnable migration path.
We will need you Ferruh to make sure all drivers will be converted soon,
hopefully in 18.02.

Re: [RFC PATCH 4/4] ethdev: add helpers to move to the new offloads API

[Ananyev, Konstantin]

> -----Original Message-----
> From: Yigit, Ferruh
> Sent: Wednesday, August 30, 2017 1:43 PM
> To: Ananyev, Konstantin <konstantin.ananyev@intel.com>; Shahaf Shuler <shahafs@mellanox.com>; Thomas Monjalon
> <thomas@monjalon.net>
> Cc: dev@dpdk.org
> Subject: Re: [dpdk-dev] [RFC PATCH 4/4] ethdev: add helpers to move to the new offloads API
> 
> On 8/30/2017 11:16 AM, Ananyev, Konstantin wrote:
> > Hi Ferruh,
> >
> >> -----Original Message-----
> >> From: Yigit, Ferruh
> >> Sent: Wednesday, August 30, 2017 8:51 AM
> >> To: Shahaf Shuler <shahafs@mellanox.com>; Thomas Monjalon <thomas@monjalon.net>; Ananyev, Konstantin
> >> <konstantin.ananyev@intel.com>
> >> Cc: dev@dpdk.org
> >> Subject: Re: [dpdk-dev] [RFC PATCH 4/4] ethdev: add helpers to move to the new offloads API
> >>
> >> On 8/30/2017 7:30 AM, Shahaf Shuler wrote:
> >>> Tuesday, August 29, 2017 3:55 PM, Ferruh Yigit:
> >>>>>> Considering the re-configuration is risky, and without other ideas I will
> >>>> need to fall back to the error flow case.
> >>>>>> Are we OK with that?
> >>>>>
> >>>>> I think we can take the risk of keeping this call to
> >>>>> rte_eth_dev_configure() in the middle of rte_eth_rx_queue_setup().
> >>>>> In theory it should be acceptable.
> >>>>> If we merge it soon, it can be better tested with every drivers.
> >>>>
> >>>> I doubt about taking that risk. Some driver does HW configuration via
> >>>> configure() and combination of start/stop, setup_queue and configure can
> >>>> be complex.
> >>>>
> >>>> I am for generating error for this case.
> >>>>
> >>>> Generating error also can be good motivation for PMDs to adapt new
> >>>> method.
> >>>
> >>> Adding Ananyev suggestion from other thread:
> >>> For tx_prepare() work, we used the following approach:
> >>> 1. submitted patch with changes in rte_ethdev and PMDs we  are familiar with (Intel ones).
> >>>     For other PMDs - patch contained just minimal changes to make it build cleanly.
> >>> 2. Asked other PMD maintainers to review rte_ethdev changes and provide a proper patch
> >>>     for the PMD they own.
> >>
> >> tx_prepare() is a little different, since it was not clear if all PMDs
> >> needs updating that is why asked to PMD owners, and the ones requires
> >> updating already has been updated with ethdev patch. Here we know all
> >> PMDs need updating, and they need proper time in advance.
> >>
> >>>
> >>> So I am OK with both suggestions. Meaning:
> >>> 1. Define the case were application use the new offloads API with PMD which supports the old one as an error.
> >>> 2. apply patches to ethdev with the above behavior.
> >>>
> >>> Just to emphasize, it means that PMDs which won't move to the new API by the end of 17.11 will not be able to run with any of the
> >> examples and application on DPDK tree (and also with other applications which moved to the new API), as I plan to submit patches which
> >> convert them all to the new API.
> >>
> >> I think it is good idea to update samples/apps to new method, but this
> >> can be short notice for PMD owners.
> >>
> >> Can we wait one more release to update samples/apps, to give time for
> >> PMDs to be updated, since old applications will work with new PMDs
> >> (thanks to your helpers), I believe this won't be a problem.
> >
> > I am not sure what is your suggestion here?
> > Support both flavors of PMD API for 17.11?
> 
> Support both with an exception, when application uses new method but PMD
> uses old one, throw an error (because of above discussed technical issue).
> 
> This lets existing applications run without problem, and pushes PMDs to
> adapt new method.
> 
> Your suggestion to have only new method and convert all PMDs to new
> method is good, but I am not sure if this is realistic for this release,
> since different PMDs have different pace for updates.
> 
> ethdev updates can be done in this release, with the PMDs that already
> changed to new method. The sample/app modifications Shahaf mentioned can
> be done in the beginning of the next release, and this gives time to
> remaining PMDs until end of next release. What do you think?

If it is just a timing concern - can we probably request PMD maintainers at least to:
- review the proposed new API and object if they have any concerns
- provide an estimation - how long it would take to adopt a new one
let say in a week time?
Then we could make a final decision - do things in one go, or prolong the pain for 2 releases.
Konstantin

> 
> > Konstantin
> >
> >>
> >>>
> >>> Any objection to this approach?
> >>>
> >>>
> >

Re: [RFC PATCH 4/4] ethdev: add helpers to move to the new offloads API

[Ananyev, Konstantin]

> -----Original Message-----
> From: Shahaf Shuler [mailto:shahafs@mellanox.com]
> Sent: Wednesday, August 23, 2017 2:13 PM
> To: Ananyev, Konstantin <konstantin.ananyev@intel.com>; dev@dpdk.org
> Subject: RE: [dpdk-dev] [RFC PATCH 4/4] ethdev: add helpers to move to the new offloads API
> 
> Wednesday, August 23, 2017 3:29 PM, Ananyev, Konstantin:
> >
> >
> > > -----Original Message-----
> > > From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Shahaf Shuler
> > > Sent: Monday, August 7, 2017 1:55 PM
> > > To: dev@dpdk.org
> > > Subject: [dpdk-dev] [RFC PATCH 4/4] ethdev: add helpers to move to the
> > > new offloads API
> > >
> > > A new offloads API was introduced by commits:
> > >
> > > commit 8b07fcae6061 ("ethdev: introduce Tx queue offloads API") commit
> > > c6504557763e ("ethdev: introduce Rx queue offloads API")
> > >
> > > In order to enable PMDs to support only one of the APIs, and
> > > applications to avoid branching according to the underlying device a
> > > copy functions to/from the old/new APIs were added.
> > >
> > > Signed-off-by: Shahaf Shuler <shahafs@mellanox.com>
> > > ---
> > >  lib/librte_ether/rte_ethdev.c | 140
> > > +++++++++++++++++++++++++++++++++++++
> > >  1 file changed, 140 insertions(+)
> > >
> > > diff --git a/lib/librte_ether/rte_ethdev.c
> > > b/lib/librte_ether/rte_ethdev.c index f73307e99..2b4a28c97 100644
> > > --- a/lib/librte_ether/rte_ethdev.c
> > > +++ b/lib/librte_ether/rte_ethdev.c
> > > @@ -1003,6 +1003,63 @@ rte_eth_dev_close(uint8_t port_id)
> > >  	dev->data->tx_queues = NULL;
> > >  }
> > >
> > > +/**
> > > + * A copy function from rxmode offloads API to rte_eth_rxq_conf
> > > + * offloads API, to enable PMDs to support only one of the APIs.
> > > + */
> > > +static void
> > > +rte_eth_copy_rxmode_offloads(struct rte_eth_rxmode *rxmode,
> > > +			     struct rte_eth_rxq_conf *rxq_conf) {
> > > +	if (rxmode->header_split == 1)
> > > +		rxq_conf->offloads |= DEV_RX_OFFLOAD_HEADER_SPLIT;
> > > +	if (rxmode->hw_ip_checksum == 1)
> > > +		rxq_conf->offloads |= DEV_RX_OFFLOAD_CHECKSUM;
> > > +	if (rxmode->hw_vlan_filter == 1)
> > > +		rxq_conf->offloads |= DEV_RX_OFFLOAD_VLAN_FILTER;
> > > +	if (rxmode->hw_vlan_strip == 1)
> > > +		rxq_conf->offloads |= DEV_RX_OFFLOAD_VLAN_STRIP;
> > > +	if (rxmode->hw_vlan_extend == 1)
> > > +		rxq_conf->offloads |= DEV_RX_OFFLOAD_VLAN_EXTEND;
> > > +	if (rxmode->jumbo_frame == 1)
> > > +		rxq_conf->offloads |= DEV_RX_OFFLOAD_JUMBO_FRAME;
> > > +	if (rxmode->hw_strip_crc == 1)
> > > +		rxq_conf->offloads |= DEV_RX_OFFLOAD_CRC_STRIP;
> > > +	if (rxmode->enable_scatter == 1)
> > > +		rxq_conf->offloads |= DEV_RX_OFFLOAD_SCATTER;
> > > +	if (rxmode->enable_lro == 1)
> > > +		rxq_conf->offloads |= DEV_RX_OFFLOAD_LRO; }
> > > +
> > > +/**
> > > + * A copy function between rte_eth_rxq_conf offloads API to rxmode
> > > + * offloads API, to enable application to be agnostic to the PMD
> > > +supported
> > > + * offload API.
> > > + */
> > > +static void
> > > +rte_eth_copy_rxq_offloads(struct rte_eth_rxmode *rxmode,
> > > +			  struct rte_eth_rxq_conf *rxq_conf) {
> > > +	if (rxq_conf->offloads & DEV_RX_OFFLOAD_HEADER_SPLIT)
> > > +		rxmode->header_split = 1;
> > > +	if (rxq_conf->offloads & DEV_RX_OFFLOAD_CHECKSUM)
> > > +		rxmode->hw_ip_checksum = 1;
> > > +	if (rxq_conf->offloads & DEV_RX_OFFLOAD_VLAN_FILTER)
> > > +		rxmode->hw_vlan_filter = 1;
> > > +	if (rxq_conf->offloads & DEV_RX_OFFLOAD_VLAN_STRIP)
> > > +		rxmode->hw_vlan_strip = 1;
> > > +	if (rxq_conf->offloads & DEV_RX_OFFLOAD_VLAN_EXTEND)
> > > +		rxmode->hw_vlan_extend = 1;
> > > +	if (rxq_conf->offloads & DEV_RX_OFFLOAD_JUMBO_FRAME)
> > > +		rxmode->jumbo_frame = 1;
> > > +	if (rxq_conf->offloads & DEV_RX_OFFLOAD_CRC_STRIP)
> > > +		rxmode->hw_strip_crc = 1;
> > > +	if (rxq_conf->offloads & DEV_RX_OFFLOAD_SCATTER)
> > > +		rxmode->enable_scatter = 1;
> > > +	if (rxq_conf->offloads & DEV_RX_OFFLOAD_LRO)
> > > +		rxmode->enable_lro = 1;
> > > +}
> > > +
> > >  int
> > >  rte_eth_rx_queue_setup(uint8_t port_id, uint16_t rx_queue_id,
> > >  		       uint16_t nb_rx_desc, unsigned int socket_id, @@ -1083,6
> > > +1140,37 @@ rte_eth_rx_queue_setup(uint8_t port_id, uint16_t
> > rx_queue_id,
> > >  	if (rx_conf == NULL)
> > >  		rx_conf = &dev_info.default_rxconf;
> > >
> > > +	if ((dev->data->dev_flags & RTE_ETH_DEV_RXQ_OFFLOAD) &&
> > > +	    (dev->data->dev_conf.rxmode.ignore == 0)) {
> > > +		rte_eth_copy_rxmode_offloads(&dev->data-
> > >dev_conf.rxmode,
> > > +					     rx_conf);
> > > +	} else if ((!(dev->data->dev_flags & RTE_ETH_DEV_RXQ_OFFLOAD))
> > &&
> > > +		   (dev->data->dev_conf.rxmode.ignore == 1)) {
> > > +		int ret;
> > > +		struct rte_eth_rxmode rxmode;
> > > +
> > > +		rte_eth_copy_rxq_offloads(&rxmode, rx_conf);
> > > +		if (memcmp(&rxmode, &dev->data->dev_conf.rxmode,
> > > +			   sizeof(rxmode))) {
> > > +			/*
> > > +			 * device which work with rxmode offloads API
> > requires
> > > +			 * a re-configuration in order to apply the new
> > offloads
> > > +			 * configuration.
> > > +			 */
> > > +			dev->data->dev_conf.rxmode = rxmode;
> > > +			ret = rte_eth_dev_configure(port_id,
> > > +					dev->data->nb_rx_queues,
> > > +					dev->data->nb_tx_queues,
> > > +					&dev->data->dev_conf);
> >
> >
> > Hmm, and why we would need to reconfigure our device in the middle of rx
> > queue setup?
> 
> The reason is the old Rx offloads API is configured on device configure.
> This if section is for applications which already moved to the new offload API however the underlying PMD still uses the old one.

Ok, but as I remember, right now the initialization order is pretty strict:
rx_queue_setup() has to be always called after dev_configure().
One of the reasons for that: rx_queue_setup() might change fileds inside  dev->data->dev_private.
Second call for dev_configure() will void these changes and some of rxq config information will be lost.
So I think we should avoid calling dev_configure() inside rx_queue_setup().

My preference still would be to force all PMDs to move to the new version of rx_queue_setup() first.
I think it would be much more error prone then supporting two flavors of PMD config
and will allow us to catch errors early - in case this new scheme doesn't work by some PMD for any reason.   

Also it seems that you  forgot to:
struct rte_eth_rxmode rxmode  = dev->data->dev_conf.rxmode;

Konstantin


> 
> >
> > > +			if (ret < 0) {
> > > +				RTE_PMD_DEBUG_TRACE(
> > > +					"unable to re-configure port %d "
> > > +					"in order to apply rxq offloads "
> > > +					"configuration\n", port_id);
> > > +			}
> > > +		}
> > > +	}
> > > +
> > >  	ret = (*dev->dev_ops->rx_queue_setup)(dev, rx_queue_id,
> > nb_rx_desc,
> > >  					      socket_id, rx_conf, mp);
> >
> > BTW, I don't see changes in any PMD for new offload flags?
> > Is it because it is just a n RFC and full patch would contain such changes?
> 
> Yes this is because this is an RFC.
> 
> The full patch I intend will move all examples and testpmd to the new offloads API.
> In addition it will include the mlx5 PMD support for the new offloads API.
> 
> As I said on previous mail, I believe that the work to move the different PMDs to the new API should be done by their developers or
> maintainers.
> 
> >
> >
> > >  	if (!ret) {
> > > @@ -1094,6 +1182,51 @@ rte_eth_rx_queue_setup(uint8_t port_id,
> > uint16_t rx_queue_id,
> > >  	return ret;
> > >  }
> > >
> > > +/**
> > > + * A copy function from txq_flags to rte_eth_txq_conf offloads API,
> > > + * to enable PMDs to support only one of the APIs.
> > > + */
> > > +static void
> > > +rte_eth_copy_txq_flags(struct rte_eth_txq_conf *txq_conf) {
> > > +	uint32_t txq_flags = txq_conf->txq_flags
> > > +	uint64_t *offloads = &txq_conf->offloads;
> > > +
> > > +	if (!(txq_flags & ETH_TXQ_FLAGS_NOMULTSEGS))
> > > +		*offloads |= DEV_TX_OFFLOAD_MULTI_SEGS;
> > > +	if (!(txq_flags & ETH_TXQ_FLAGS_NOVLANOFFL))
> > > +		*offloads |= DEV_TX_OFFLOAD_VLAN_INSERT;
> > > +	if (!(txq_flags & ETH_TXQ_FLAGS_NOXSUMSCTP))
> > > +		*offloads |= DEV_TX_OFFLOAD_SCTP_CKSUM;
> > > +	if (!(txq_flags & ETH_TXQ_FLAGS_NOXSUMUDP))
> > > +		*offloads |= DEV_TX_OFFLOAD_UDP_CKSUM;
> > > +	if (!(txq_flags & ETH_TXQ_FLAGS_NOXSUMTCP))
> > > +		*offloads |= DEV_TX_OFFLOAD_TCP_CKSUM; }
> > > +
> > > +/**
> > > + * A copy function between rte_eth_txq_conf offloads API to txq_flags
> > > + * offloads API, to enable application to be agnostic to the PMD
> > > +supported
> > > + * API.
> > > + */
> > > +static void
> > > +rte_eth_copy_txq_offloads(struct rte_eth_txq_conf *txq_conf) {
> > > +	uint32_t *txq_flags = &txq_conf->txq_flags
> > > +	uint64_t offloads = txq_conf->offloads;
> > > +
> > > +	if (!(offloads & DEV_TX_OFFLOAD_MULTI_SEGS))
> > > +		*txq_flags |= ETH_TXQ_FLAGS_NOMULTSEGS;
> > > +	if (!(offloads & DEV_TX_OFFLOAD_VLAN_INSERT))
> > > +		*txq_flags |= ETH_TXQ_FLAGS_NOVLANOFFL;
> > > +	if (!(offloads & DEV_TX_OFFLOAD_SCTP_CKSUM))
> > > +		*txq_flags |= ETH_TXQ_FLAGS_NOXSUMSCTP;
> > > +	if (!(offloads & DEV_TX_OFFLOAD_UDP_CKSUM))
> > > +		*txq_flags |= ETH_TXQ_FLAGS_NOXSUMUDP;
> > > +	if (!(offloads & DEV_TX_OFFLOAD_TCP_CKSUM))
> > > +		*txq_flags |= ETH_TXQ_FLAGS_NOXSUMTCP; }
> > > +
> > >  int
> > >  rte_eth_tx_queue_setup(uint8_t port_id, uint16_t tx_queue_id,
> > >  		       uint16_t nb_tx_desc, unsigned int socket_id, @@ -1145,6
> > > +1278,13 @@ rte_eth_tx_queue_setup(uint8_t port_id, uint16_t
> > tx_queue_id,
> > >  	if (tx_conf == NULL)
> > >  		tx_conf = &dev_info.default_txconf;
> > >
> > > +	if ((dev->data->dev_flags & RTE_ETH_DEV_TXQ_OFFLOAD) &&
> > > +	    (!(tx_conf->txq_flags & ETH_TXQ_FLAGS_IGNORE)))
> > > +		rte_eth_copy_txq_flags(tx_conf);
> > > +	else if (!(dev->data->dev_flags & RTE_ETH_DEV_TXQ_OFFLOAD) &&
> > > +		   (tx_conf->txq_flags & ETH_TXQ_FLAGS_IGNORE))
> > > +		rte_eth_copy_txq_offloads(tx_conf);
> > > +
> >
> > As I said in my previous mail - I think better to always convrert from old
> > txq_flags to new TX offload flags and make each PMD to understand new
> > offload values only.
> > Konstantin
> >
> > >  	return (*dev->dev_ops->tx_queue_setup)(dev, tx_queue_id,
> > nb_tx_desc,
> > >  					       socket_id, tx_conf);
> > >  }
> > > --
> > > 2.12.0

Re: [RFC PATCH 4/4] ethdev: add helpers to move to the new offloads API

[Shahaf Shuler]

Monday, August 28, 2017 5:12 PM, Ananyev, Konstantin:
> > Wednesday, August 23, 2017 3:29 PM, Ananyev, Konstantin:
> > >
> > > Hmm, and why we would need to reconfigure our device in the middle
> > > of rx queue setup?
> >
> > The reason is the old Rx offloads API is configured on device configure.
> > This if section is for applications which already moved to the new offload
> API however the underlying PMD still uses the old one.
> 
> Ok, but as I remember, right now the initialization order is pretty strict:
> rx_queue_setup() has to be always called after dev_configure().

Is this restriction backed up in the API definition? 
>From what I saw in  dev_configure API documentation:

" 
This function must be invoked first before any other function in the Ethernet API. This function can also be re-invoked when a device is in the stopped state.
"

It does not dictate one is forbidden to do (while port is stopped):
dev_configure
rx_queue_setup(queue 1)
dev _configure (for the same configuration)
rx_queue_setup(queue 2)

didn't saw any words on in it also in rx_queue_setup API. maybe API doc should be updated.

You are correct though that all examples and apps on dpdk tree behaves in the way you described.

> One of the reasons for that: rx_queue_setup() might change fileds inside
> dev->data->dev_private.
> Second call for dev_configure() will void these changes and some of rxq
> config information will be lost.
> So I think we should avoid calling dev_configure() inside rx_queue_setup().

In continue to above comment, is this reason is because of a wrong implementation of the callbacks by the PMDs or actual limitation from ethdev?

> 
> My preference still would be to force all PMDs to move to the new version of
> rx_queue_setup() first.
> I think it would be much more error prone then supporting two flavors of
> PMD config
> and will allow us to catch errors early - in case this new scheme doesn't work
> by some PMD for any reason.

I Fully agree with you. If we can force all PMDs to move the new API it will be the best.
No need in risky re-configuration in the middle of the queue setup. 
But how can we force all to do such transition? I saw there is a discussion on this RFC on the techboard - maybe it can be decided there.

As I said before, I think I will be very hard to migrate all PMDs to the new API by myself.
It is not a simple function prototype change or some identical swap I need to do on each PMD.
Each device has its own characteristics and restriction on the offloads it can provide.
Some offloads can be enabled only on specific places in the device initialization.
Some offloads are only per port.
To do such transition properly I must fully understand the underlying device of each PMD.

I can commit on transition for the devices I familiar with: Mlx4 and mlx5. 

> 
> Also it seems that you  forgot to:
> struct rte_eth_rxmode rxmode  = dev->data->dev_conf.rxmode;

Thanks, discovered it right after I sent the RFC. Already fixed. 

> 
> Konstantin
> 
> 
> >
> > >
> > > > +			if (ret < 0) {
> > > > +				RTE_PMD_DEBUG_TRACE(
> > > > +					"unable to re-configure port %d "
> > > > +					"in order to apply rxq offloads "
> > > > +					"configuration\n", port_id);
> > > > +			}
> > > > +		}
> > > > +	}
> > > > +
> > > >  	ret = (*dev->dev_ops->rx_queue_setup)(dev, rx_queue_id,
> > > nb_rx_desc,
> > > >  					      socket_id, rx_conf, mp);
> > >
> > > BTW, I don't see changes in any PMD for new offload flags?
> > > Is it because it is just a n RFC and full patch would contain such changes?
> >
> > Yes this is because this is an RFC.
> >
> > The full patch I intend will move all examples and testpmd to the new
> offloads API.
> > In addition it will include the mlx5 PMD support for the new offloads API.
> >
> > As I said on previous mail, I believe that the work to move the
> > different PMDs to the new API should be done by their developers or
> maintainers.
> >
> > >
> > >
> > > >  	if (!ret) {
> > > > @@ -1094,6 +1182,51 @@ rte_eth_rx_queue_setup(uint8_t port_id,
> > > uint16_t rx_queue_id,
> > > >  	return ret;
> > > >  }
> > > >
> > > > +/**
> > > > + * A copy function from txq_flags to rte_eth_txq_conf offloads
> > > > +API,
> > > > + * to enable PMDs to support only one of the APIs.
> > > > + */
> > > > +static void
> > > > +rte_eth_copy_txq_flags(struct rte_eth_txq_conf *txq_conf) {
> > > > +	uint32_t txq_flags = txq_conf->txq_flags
> > > > +	uint64_t *offloads = &txq_conf->offloads;
> > > > +
> > > > +	if (!(txq_flags & ETH_TXQ_FLAGS_NOMULTSEGS))
> > > > +		*offloads |= DEV_TX_OFFLOAD_MULTI_SEGS;
> > > > +	if (!(txq_flags & ETH_TXQ_FLAGS_NOVLANOFFL))
> > > > +		*offloads |= DEV_TX_OFFLOAD_VLAN_INSERT;
> > > > +	if (!(txq_flags & ETH_TXQ_FLAGS_NOXSUMSCTP))
> > > > +		*offloads |= DEV_TX_OFFLOAD_SCTP_CKSUM;
> > > > +	if (!(txq_flags & ETH_TXQ_FLAGS_NOXSUMUDP))
> > > > +		*offloads |= DEV_TX_OFFLOAD_UDP_CKSUM;
> > > > +	if (!(txq_flags & ETH_TXQ_FLAGS_NOXSUMTCP))
> > > > +		*offloads |= DEV_TX_OFFLOAD_TCP_CKSUM; }
> > > > +
> > > > +/**
> > > > + * A copy function between rte_eth_txq_conf offloads API to
> > > > +txq_flags
> > > > + * offloads API, to enable application to be agnostic to the PMD
> > > > +supported
> > > > + * API.
> > > > + */
> > > > +static void
> > > > +rte_eth_copy_txq_offloads(struct rte_eth_txq_conf *txq_conf) {
> > > > +	uint32_t *txq_flags = &txq_conf->txq_flags
> > > > +	uint64_t offloads = txq_conf->offloads;
> > > > +
> > > > +	if (!(offloads & DEV_TX_OFFLOAD_MULTI_SEGS))
> > > > +		*txq_flags |= ETH_TXQ_FLAGS_NOMULTSEGS;
> > > > +	if (!(offloads & DEV_TX_OFFLOAD_VLAN_INSERT))
> > > > +		*txq_flags |= ETH_TXQ_FLAGS_NOVLANOFFL;
> > > > +	if (!(offloads & DEV_TX_OFFLOAD_SCTP_CKSUM))
> > > > +		*txq_flags |= ETH_TXQ_FLAGS_NOXSUMSCTP;
> > > > +	if (!(offloads & DEV_TX_OFFLOAD_UDP_CKSUM))
> > > > +		*txq_flags |= ETH_TXQ_FLAGS_NOXSUMUDP;
> > > > +	if (!(offloads & DEV_TX_OFFLOAD_TCP_CKSUM))
> > > > +		*txq_flags |= ETH_TXQ_FLAGS_NOXSUMTCP; }
> > > > +
> > > >  int
> > > >  rte_eth_tx_queue_setup(uint8_t port_id, uint16_t tx_queue_id,
> > > >  		       uint16_t nb_tx_desc, unsigned int socket_id, @@ -1145,6
> > > > +1278,13 @@ rte_eth_tx_queue_setup(uint8_t port_id, uint16_t
> > > tx_queue_id,
> > > >  	if (tx_conf == NULL)
> > > >  		tx_conf = &dev_info.default_txconf;
> > > >
> > > > +	if ((dev->data->dev_flags & RTE_ETH_DEV_TXQ_OFFLOAD) &&
> > > > +	    (!(tx_conf->txq_flags & ETH_TXQ_FLAGS_IGNORE)))
> > > > +		rte_eth_copy_txq_flags(tx_conf);
> > > > +	else if (!(dev->data->dev_flags & RTE_ETH_DEV_TXQ_OFFLOAD) &&
> > > > +		   (tx_conf->txq_flags & ETH_TXQ_FLAGS_IGNORE))
> > > > +		rte_eth_copy_txq_offloads(tx_conf);
> > > > +
> > >
> > > As I said in my previous mail - I think better to always convrert
> > > from old txq_flags to new TX offload flags and make each PMD to
> > > understand new offload values only.
> > > Konstantin
> > >
> > > >  	return (*dev->dev_ops->tx_queue_setup)(dev, tx_queue_id,
> > > nb_tx_desc,
> > > >  					       socket_id, tx_conf);
> > > >  }
> > > > --
> > > > 2.12.0

Re: [RFC PATCH 4/4] ethdev: add helpers to move to the new offloads API

[Ananyev, Konstantin]

> -----Original Message-----
> From: Shahaf Shuler [mailto:shahafs@mellanox.com]
> Sent: Tuesday, August 29, 2017 7:27 AM
> To: Ananyev, Konstantin <konstantin.ananyev@intel.com>; dev@dpdk.org
> Subject: RE: [dpdk-dev] [RFC PATCH 4/4] ethdev: add helpers to move to the new offloads API
> 
> Monday, August 28, 2017 5:12 PM, Ananyev, Konstantin:
> > > Wednesday, August 23, 2017 3:29 PM, Ananyev, Konstantin:
> > > >
> > > > Hmm, and why we would need to reconfigure our device in the middle
> > > > of rx queue setup?
> > >
> > > The reason is the old Rx offloads API is configured on device configure.
> > > This if section is for applications which already moved to the new offload
> > API however the underlying PMD still uses the old one.
> >
> > Ok, but as I remember, right now the initialization order is pretty strict:
> > rx_queue_setup() has to be always called after dev_configure().
> 
> Is this restriction backed up in the API definition?
> From what I saw in  dev_configure API documentation:
> 
> "
> This function must be invoked first before any other function in the Ethernet API. This function can also be re-invoked when a device is in
> the stopped state.
> "
> 
> It does not dictate one is forbidden to do (while port is stopped):
> dev_configure
> rx_queue_setup(queue 1)
> dev _configure (for the same configuration)
> rx_queue_setup(queue 2)
> 

It might work in some cases, but no guarantee it would work in general.
Though, as I understand, in your case for second call of dev_configure() the 
config parameters might  be different anyway.

> didn't saw any words on in it also in rx_queue_setup API. maybe API doc should be updated.

Yes, good point.

> 
> You are correct though that all examples and apps on dpdk tree behaves in the way you described.
> 
> > One of the reasons for that: rx_queue_setup() might change fileds inside
> > dev->data->dev_private.
> > Second call for dev_configure() will void these changes and some of rxq
> > config information will be lost.
> > So I think we should avoid calling dev_configure() inside rx_queue_setup().
> 
> In continue to above comment, is this reason is because of a wrong implementation of the callbacks by the PMDs or actual limitation from
> ethdev?

I'd say it is a combination of limitations of ethdev design and actual HW restrictions.
If we'll have a rx/tx function per queue some of these limitations might be removed I think.
Though for some HW - offloads can be enabled/disabled per device, not queue,
so I guess some limitations would  persist anyway.

> 
> >
> > My preference still would be to force all PMDs to move to the new version of
> > rx_queue_setup() first.
> > I think it would be much more error prone then supporting two flavors of
> > PMD config
> > and will allow us to catch errors early - in case this new scheme doesn't work
> > by some PMD for any reason.
> 
> I Fully agree with you. If we can force all PMDs to move the new API it will be the best.
> No need in risky re-configuration in the middle of the queue setup.
> But how can we force all to do such transition? I saw there is a discussion on this RFC on the techboard - maybe it can be decided there.
> 
> As I said before, I think I will be very hard to migrate all PMDs to the new API by myself.
> It is not a simple function prototype change or some identical swap I need to do on each PMD.
> Each device has its own characteristics and restriction on the offloads it can provide.
> Some offloads can be enabled only on specific places in the device initialization.
> Some offloads are only per port.
> To do such transition properly I must fully understand the underlying device of each PMD.
> 
> I can commit on transition for the devices I familiar with: Mlx4 and mlx5.

That's understandable.
For tx_prepare() work, we used the following approach:
1. submitted patch with changes in rte_ethdev and PMDs we  are familiar with (Intel ones).
    For other PMDs - patch contained just minimal changes to make it build cleanly.
2. Asked other PMD maintainers to review rte_ethdev changes and provide a proper patch
    for the PMD they own.

Probably same approach can be used here.
Konstantin

> 
> >
> > Also it seems that you  forgot to:
> > struct rte_eth_rxmode rxmode  = dev->data->dev_conf.rxmode;
> 
> Thanks, discovered it right after I sent the RFC. Already fixed.
> 
> >
> > Konstantin
> >
> >
> > >
> > > >
> > > > > +			if (ret < 0) {
> > > > > +				RTE_PMD_DEBUG_TRACE(
> > > > > +					"unable to re-configure port %d "
> > > > > +					"in order to apply rxq offloads "
> > > > > +					"configuration\n", port_id);
> > > > > +			}
> > > > > +		}
> > > > > +	}
> > > > > +
> > > > >  	ret = (*dev->dev_ops->rx_queue_setup)(dev, rx_queue_id,
> > > > nb_rx_desc,
> > > > >  					      socket_id, rx_conf, mp);
> > > >
> > > > BTW, I don't see changes in any PMD for new offload flags?
> > > > Is it because it is just a n RFC and full patch would contain such changes?
> > >
> > > Yes this is because this is an RFC.
> > >
> > > The full patch I intend will move all examples and testpmd to the new
> > offloads API.
> > > In addition it will include the mlx5 PMD support for the new offloads API.
> > >
> > > As I said on previous mail, I believe that the work to move the
> > > different PMDs to the new API should be done by their developers or
> > maintainers.
> > >
> > > >
> > > >
> > > > >  	if (!ret) {
> > > > > @@ -1094,6 +1182,51 @@ rte_eth_rx_queue_setup(uint8_t port_id,
> > > > uint16_t rx_queue_id,
> > > > >  	return ret;
> > > > >  }
> > > > >
> > > > > +/**
> > > > > + * A copy function from txq_flags to rte_eth_txq_conf offloads
> > > > > +API,
> > > > > + * to enable PMDs to support only one of the APIs.
> > > > > + */
> > > > > +static void
> > > > > +rte_eth_copy_txq_flags(struct rte_eth_txq_conf *txq_conf) {
> > > > > +	uint32_t txq_flags = txq_conf->txq_flags
> > > > > +	uint64_t *offloads = &txq_conf->offloads;
> > > > > +
> > > > > +	if (!(txq_flags & ETH_TXQ_FLAGS_NOMULTSEGS))
> > > > > +		*offloads |= DEV_TX_OFFLOAD_MULTI_SEGS;
> > > > > +	if (!(txq_flags & ETH_TXQ_FLAGS_NOVLANOFFL))
> > > > > +		*offloads |= DEV_TX_OFFLOAD_VLAN_INSERT;
> > > > > +	if (!(txq_flags & ETH_TXQ_FLAGS_NOXSUMSCTP))
> > > > > +		*offloads |= DEV_TX_OFFLOAD_SCTP_CKSUM;
> > > > > +	if (!(txq_flags & ETH_TXQ_FLAGS_NOXSUMUDP))
> > > > > +		*offloads |= DEV_TX_OFFLOAD_UDP_CKSUM;
> > > > > +	if (!(txq_flags & ETH_TXQ_FLAGS_NOXSUMTCP))
> > > > > +		*offloads |= DEV_TX_OFFLOAD_TCP_CKSUM; }
> > > > > +
> > > > > +/**
> > > > > + * A copy function between rte_eth_txq_conf offloads API to
> > > > > +txq_flags
> > > > > + * offloads API, to enable application to be agnostic to the PMD
> > > > > +supported
> > > > > + * API.
> > > > > + */
> > > > > +static void
> > > > > +rte_eth_copy_txq_offloads(struct rte_eth_txq_conf *txq_conf) {
> > > > > +	uint32_t *txq_flags = &txq_conf->txq_flags
> > > > > +	uint64_t offloads = txq_conf->offloads;
> > > > > +
> > > > > +	if (!(offloads & DEV_TX_OFFLOAD_MULTI_SEGS))
> > > > > +		*txq_flags |= ETH_TXQ_FLAGS_NOMULTSEGS;
> > > > > +	if (!(offloads & DEV_TX_OFFLOAD_VLAN_INSERT))
> > > > > +		*txq_flags |= ETH_TXQ_FLAGS_NOVLANOFFL;
> > > > > +	if (!(offloads & DEV_TX_OFFLOAD_SCTP_CKSUM))
> > > > > +		*txq_flags |= ETH_TXQ_FLAGS_NOXSUMSCTP;
> > > > > +	if (!(offloads & DEV_TX_OFFLOAD_UDP_CKSUM))
> > > > > +		*txq_flags |= ETH_TXQ_FLAGS_NOXSUMUDP;
> > > > > +	if (!(offloads & DEV_TX_OFFLOAD_TCP_CKSUM))
> > > > > +		*txq_flags |= ETH_TXQ_FLAGS_NOXSUMTCP; }
> > > > > +
> > > > >  int
> > > > >  rte_eth_tx_queue_setup(uint8_t port_id, uint16_t tx_queue_id,
> > > > >  		       uint16_t nb_tx_desc, unsigned int socket_id, @@ -1145,6
> > > > > +1278,13 @@ rte_eth_tx_queue_setup(uint8_t port_id, uint16_t
> > > > tx_queue_id,
> > > > >  	if (tx_conf == NULL)
> > > > >  		tx_conf = &dev_info.default_txconf;
> > > > >
> > > > > +	if ((dev->data->dev_flags & RTE_ETH_DEV_TXQ_OFFLOAD) &&
> > > > > +	    (!(tx_conf->txq_flags & ETH_TXQ_FLAGS_IGNORE)))
> > > > > +		rte_eth_copy_txq_flags(tx_conf);
> > > > > +	else if (!(dev->data->dev_flags & RTE_ETH_DEV_TXQ_OFFLOAD) &&
> > > > > +		   (tx_conf->txq_flags & ETH_TXQ_FLAGS_IGNORE))
> > > > > +		rte_eth_copy_txq_offloads(tx_conf);
> > > > > +
> > > >
> > > > As I said in my previous mail - I think better to always convrert
> > > > from old txq_flags to new TX offload flags and make each PMD to
> > > > understand new offload values only.
> > > > Konstantin
> > > >
> > > > >  	return (*dev->dev_ops->tx_queue_setup)(dev, tx_queue_id,
> > > > nb_tx_desc,
> > > > >  					       socket_id, tx_conf);
> > > > >  }
> > > > > --
> > > > > 2.12.0

Re: [RFC PATCH 0/4] ethdev new offloads API

[Shahaf Shuler]

Hi,

I would like to get some inputs on the below. 
This is a big (and important) work which I want to include on 17.11. I need to understand the current approach is acceptable before I continue.


Monday, August 7, 2017 1:54 PM, Shahaf Shuler:
> Tx offloads configuration is per queue. Tx offloads are enabled by default,
> and can be disabled using ETH_TXQ_FLAGS_NO* flags.
> This behaviour is not consistent with the Rx side where the Rx offloads
> configuration is per port. Rx offloads are disabled by default and enabled
> according to bit field in rte_eth_rxmode structure.
> 
> Moreover, considering more Tx and Rx offloads will be added over time, the
> cost of managing them all inside the PMD will be tremendous, as the PMD
> will need to check the matching for the entire offload set for each mbuf it
> handles.
> In addition, on the current approach each Rx offload added breaks the ABI
> compatibility as it requires to add entries to existing bit-fields.
> 
> The RFC address above issues by defining a new offloads API.
> With the new API, Tx and Rx offloads configuration is per queue.
> The offloads are disabled by default. Each offload can be enabled or disabled
> using the existing DEV_TX_OFFLOADS_* or DEV_RX_OFFLOADS_* flags.
> Such API will enable to easily add or remove offloads, without breaking the
> ABI compatibility.
> 
> The new API does not have an equivalent for the below Tx flags:
> 
> * ETH_TXQ_FLAGS_NOREFCOUNT
> * ETH_TXQ_FLAGS_NOMULTMEMP
> 
> The reason is that those flags are not to manage offloads, rather some
> guarantee from application on the way it uses mbufs, therefore could not be
> present as part of DEV_TX_OFFLOADS_*.
> Such flags are useful only for benchmarks, and therefore provide a non-
> realistic
> performance for DPDK customers using simple benchmarks for evaluation.
> Leveraging the work being done in this series to clean up those flags.
> 
> In order to provide a smooth transition between the APIs the following
> actions were taken:
> *  The old offloads API is kept for the meanwhile.
> *  New capabilities were added for PMD to advertize it has moved to the
> new
>    offloads API.
> *  Helper function which copy from old to new API and vice versa were
> added to ethdev,
>    enabling the PMD to support only one of the APIs, and the application to
> move to
>    the new API regardless the underlying device and without extra branching.
> 



> Shahaf Shuler (4):
>   ethdev: rename Rx and Tx configuration structs
>   ethdev: introduce Rx queue offloads API
>   ethdev: introduce Tx queue offloads API
>   ethdev: add helpers to move to the new offloads API
> 
>  lib/librte_ether/rte_ethdev.c | 144
> ++++++++++++++++++++++++++++++++++++-
>  lib/librte_ether/rte_ethdev.h |  72 +++++++++++++++----
>  2 files changed, 202 insertions(+), 14 deletions(-)
> 
> --
> 2.12.0

Re: [RFC PATCH 0/4] ethdev new offloads API

[Thomas Monjalon]

07/08/2017 12:54, Shahaf Shuler:
> The new API does not have an equivalent for the below Tx flags:
> 
> * ETH_TXQ_FLAGS_NOREFCOUNT
> * ETH_TXQ_FLAGS_NOMULTMEMP
> 
> The reason is that those flags are not to manage offloads, rather some
> guarantee from application on the way it uses mbufs, therefore could not be
> present as part of DEV_TX_OFFLOADS_*.
> Such flags are useful only for benchmarks, and therefore provide a non-realistic    
> performance for DPDK customers using simple benchmarks for evaluation.
> Leveraging the work being done in this series to clean up those flags.

I agree that these flags are not offload but benchmark configuration.
They should be deprecated.

Re: [RFC PATCH 0/4] ethdev new offloads API

[Jerin Jacob]

-----Original Message-----
> Date: Mon,  7 Aug 2017 13:54:27 +0300
> From: Shahaf Shuler <shahafs@mellanox.com>
> To: dev@dpdk.org
> Subject: [dpdk-dev] [RFC PATCH 0/4] ethdev new offloads API
> X-Mailer: git-send-email 2.12.0
> 
> Tx offloads configuration is per queue. Tx offloads are enabled by default, 
> and can be disabled using ETH_TXQ_FLAGS_NO* flags. 
> This behaviour is not consistent with the Rx side where the Rx offloads
> configuration is per port. Rx offloads are disabled by default and enabled 
> according to bit field in rte_eth_rxmode structure.
> 
> Moreover, considering more Tx and Rx offloads will be added 
> over time, the cost of managing them all inside the PMD will be tremendous,
> as the PMD will need to check the matching for the entire offload set 
> for each mbuf it handles.
> In addition, on the current approach each Rx offload added breaks the
> ABI compatibility as it requires to add entries to existing bit-fields.
>  
> The RFC address above issues by defining a new offloads API.
> With the new API, Tx and Rx offloads configuration is per queue.
> The offloads are disabled by default. Each offload can be enabled or
> disabled using the existing DEV_TX_OFFLOADS_* or DEV_RX_OFFLOADS_* flags.
> Such API will enable to easily add or remove offloads, without breaking the
> ABI compatibility.
> 
> The new API does not have an equivalent for the below Tx flags:
> 
> * ETH_TXQ_FLAGS_NOREFCOUNT
> * ETH_TXQ_FLAGS_NOMULTMEMP

IMO, it make sense to keep those flags as PMD optimization if an application
does not need reference count and multi mempool in the application.
As example, An non trivial application like l3fwd does not need both of them.

Re: [RFC PATCH 0/4] ethdev new offloads API

[Shahaf Shuler]

Friday, August 25, 2017 1:32 PM, Jerin Jacob:
> >
> > The new API does not have an equivalent for the below Tx flags:
> >
> > * ETH_TXQ_FLAGS_NOREFCOUNT
> > * ETH_TXQ_FLAGS_NOMULTMEMP
> 
> IMO, it make sense to keep those flags as PMD optimization if an application
> does not need reference count and multi mempool in the application.
> As example, An non trivial application like l3fwd does not need both of them.

The l3fwd application is yet another simple example from DPDK tree. Am not sure that a complete vRouter/vSwitch implementation is with the same characteristics.
Moreover, I think the fact there is an application which is able to use it is not enough.  IMO there needs to be some basic functionality always provided by the PMDs and not controlled by flags.
For example, let's say we have an application which always sends the mbufs with the same ol_flags, or even with the same length.
Will it make sense to add more flags to control it?
Will it makes sense to run RFC2544 benchmark with testpmd io forwarding with those flags? 

If the answer is yes, maybe those flags (and others to follow) belong on different location on ethdev. However for sure they are not offloads.

Re: [RFC PATCH 0/4] ethdev new offloads API

[Jerin Jacob]

-----Original Message-----
> Date: Sun, 27 Aug 2017 06:05:25 +0000
> From: Shahaf Shuler <shahafs@mellanox.com>
> To: Jerin Jacob <jerin.jacob@caviumnetworks.com>
> CC: "dev@dpdk.org" <dev@dpdk.org>
> Subject: RE: [dpdk-dev] [RFC PATCH 0/4] ethdev new offloads API
> 
> Friday, August 25, 2017 1:32 PM, Jerin Jacob:
> > >
> > > The new API does not have an equivalent for the below Tx flags:
> > >
> > > * ETH_TXQ_FLAGS_NOREFCOUNT
> > > * ETH_TXQ_FLAGS_NOMULTMEMP
> > 
> > IMO, it make sense to keep those flags as PMD optimization if an application
> > does not need reference count and multi mempool in the application.
> > As example, An non trivial application like l3fwd does not need both of them.
> 
> The l3fwd application is yet another simple example from DPDK tree. Am not sure that a complete vRouter/vSwitch implementation is with the same characteristics.

But not all dpdk applications are complete vRouter/vSwitch implementation.

> Moreover, I think the fact there is an application which is able to use it is not enough.  IMO there needs to be some basic functionality always provided by the PMDs and not controlled by flags.
> For example, let's say we have an application which always sends the mbufs with the same ol_flags, or even with the same length.

Does ETH_TXQ_FLAGS_NOREFCOUNT comes in same category like mbuf->ol_flags?

> Will it make sense to add more flags to control it?
> Will it makes sense to run RFC2544 benchmark with testpmd io forwarding with those flags? 
> 
> If the answer is yes, maybe those flags (and others to follow) belong on different location on ethdev. However for sure they are not offloads.

I am not sure about the reason for opting out mempool related flags.
In the context of HW assisted external mempool managers, Enabling reference count is an offload
from Ethernet device.
For example, with external HW assisted mempool, ethdev driver needs to
have different way of forming TXQ descriptor in case if reference count
is enabled(as, in the case of HW assisted mempool managers, bu default,
HW frees the packet on send)

I am fine with moving the flags to some where else if it is make sense to you.But
from PMD optimization perspective, I think it is important have these flags.

Re: [RFC PATCH 0/4] ethdev new offloads API

[Thomas Monjalon]

28/08/2017 07:00, Jerin Jacob:
> From: Shahaf Shuler <shahafs@mellanox.com>
> > Friday, August 25, 2017 1:32 PM, Jerin Jacob:
> > > >
> > > > The new API does not have an equivalent for the below Tx flags:
> > > >
> > > > * ETH_TXQ_FLAGS_NOREFCOUNT
> > > > * ETH_TXQ_FLAGS_NOMULTMEMP
> > > 
> > > IMO, it make sense to keep those flags as PMD optimization if an application
> > > does not need reference count and multi mempool in the application.
> > > As example, An non trivial application like l3fwd does not need both of them.
> > 
> > The l3fwd application is yet another simple example from DPDK tree. Am not sure that a complete vRouter/vSwitch implementation is with the same characteristics.
> 
> But not all dpdk applications are complete vRouter/vSwitch implementation.
> 
> > Moreover, I think the fact there is an application which is able to use it is not enough.  IMO there needs to be some basic functionality always provided by the PMDs and not controlled by flags.
> > For example, let's say we have an application which always sends the mbufs with the same ol_flags, or even with the same length.
> 
> Does ETH_TXQ_FLAGS_NOREFCOUNT comes in same category like mbuf->ol_flags?
> 
> > Will it make sense to add more flags to control it?
> > Will it makes sense to run RFC2544 benchmark with testpmd io forwarding with those flags? 
> > 
> > If the answer is yes, maybe those flags (and others to follow) belong on different location on ethdev. However for sure they are not offloads.
> 
> I am not sure about the reason for opting out mempool related flags.
> In the context of HW assisted external mempool managers, Enabling reference count is an offload
> from Ethernet device.
> For example, with external HW assisted mempool, ethdev driver needs to
> have different way of forming TXQ descriptor in case if reference count
> is enabled(as, in the case of HW assisted mempool managers, bu default,
> HW frees the packet on send)
> 
> I am fine with moving the flags to some where else if it is make sense to you.But
> from PMD optimization perspective, I think it is important have these flags.

Why not.
Please Jerin, could you work on moving these settings in a new API?
We can have a function to enable such optimizations.
However I am not sure ethdev is the right place as these hints apply
to any mbuf.
Other related question: what happens if other libraries manipulate
these mbufs with special optimization hints? Should they be aware?

Re: [RFC PATCH 0/4] ethdev new offloads API

[Jerin Jacob]

-----Original Message-----
> Date: Mon, 28 Aug 2017 12:57:13 +0200
> From: Thomas Monjalon <thomas@monjalon.net>
> To: Jerin Jacob <jerin.jacob@caviumnetworks.com>
> Cc: dev@dpdk.org, Shahaf Shuler <shahafs@mellanox.com>
> Subject: Re: [dpdk-dev] [RFC PATCH 0/4] ethdev new offloads API
> 
> 28/08/2017 07:00, Jerin Jacob:
> > From: Shahaf Shuler <shahafs@mellanox.com>
> > > Friday, August 25, 2017 1:32 PM, Jerin Jacob:
> > > > >
> > > > > The new API does not have an equivalent for the below Tx flags:
> > > > >
> > > > > * ETH_TXQ_FLAGS_NOREFCOUNT
> > > > > * ETH_TXQ_FLAGS_NOMULTMEMP
> > > > 
> > > > IMO, it make sense to keep those flags as PMD optimization if an application
> > > > does not need reference count and multi mempool in the application.
> > > > As example, An non trivial application like l3fwd does not need both of them.
> > > 
> > > The l3fwd application is yet another simple example from DPDK tree. Am not sure that a complete vRouter/vSwitch implementation is with the same characteristics.
> > 
> > But not all dpdk applications are complete vRouter/vSwitch implementation.
> > 
> > > Moreover, I think the fact there is an application which is able to use it is not enough.  IMO there needs to be some basic functionality always provided by the PMDs and not controlled by flags.
> > > For example, let's say we have an application which always sends the mbufs with the same ol_flags, or even with the same length.
> > 
> > Does ETH_TXQ_FLAGS_NOREFCOUNT comes in same category like mbuf->ol_flags?
> > 
> > > Will it make sense to add more flags to control it?
> > > Will it makes sense to run RFC2544 benchmark with testpmd io forwarding with those flags? 
> > > 
> > > If the answer is yes, maybe those flags (and others to follow) belong on different location on ethdev. However for sure they are not offloads.
> > 
> > I am not sure about the reason for opting out mempool related flags.
> > In the context of HW assisted external mempool managers, Enabling reference count is an offload
> > from Ethernet device.
> > For example, with external HW assisted mempool, ethdev driver needs to
> > have different way of forming TXQ descriptor in case if reference count
> > is enabled(as, in the case of HW assisted mempool managers, bu default,
> > HW frees the packet on send)
> > 
> > I am fine with moving the flags to some where else if it is make sense to you.But
> > from PMD optimization perspective, I think it is important have these flags.
> 
> Why not.

> We can have a function to enable such optimizations.

OK

> However I am not sure ethdev is the right place as these hints apply
> to any mbuf.

I think, We are talking about the mbuf behavior when working with ethdev TXQ
here. Right? IMO, it make senses in the ethdev layer.

We pulled in ETH_TXQ_FLAGS_NOMULTSEGS flag for the rework even though it
is related to mbuf. If you think, ETH_TXQ_FLAGS_NOREFCOUNT and
ETH_TXQ_FLAGS_NOMULTMEMP should NOT a be PER TXQ configurable flag then it can
be moved to Port level TXQ configuration at

function: rte_eth_dev_configure()
struct rte_eth_conf::struct rte_eth_txmode:

I think it is important for The ARM64 architecture to optimize for the
application which does not need reference counting and single pool
configuration like l3fwd.

Reasons:
- The NPU class ethdev hardwares are tightly coupled with external
  mempool ops with HW offload, and there is provision to utilize these features
- For the general purpose arm64 perspective, At least for the low-end systems,
  the cache hierarchy is quite different from x86. So its costly to deference
  the mbuf area(which stores the mempool handler) after the Tx free. We
  can support both use cases, just that it should configurable based on
  the flags by the application.

> Please Jerin, could you work on moving these settings in a new API?

Sure. Once the generic code is in place. We are committed to fix the
PMDs by 18.02.

Jerin