[v4,1/3] ethdev: introduce IP reassembly offload

  IP Reassembly is a costly operation if it is done in software.
The operation becomes even more costlier if IP fragments are encrypted.
However, if it is offloaded to HW, it can considerably save application
cycles.

Hence, a new offload feature is exposed in eth_dev ops for devices which can
attempt IP reassembly of packets in hardware.
- rte_eth_ip_reassembly_capability_get() - to get the maximum values
  of reassembly configuration which can be set.
- rte_eth_ip_reassembly_conf_set() - to set IP reassembly configuration
  and to enable the feature in the PMD (to be called before rte_eth_dev_start()).
- rte_eth_ip_reassembly_conf_get() - to get the current configuration
  set in PMD.

Now when the offload is enabled using rte_eth_ip_reassembly_conf_set(),
the resulting reassembled IP packet would be a typical segmented mbuf in
case of success.

And if reassembly of IP fragments is failed or is incomplete (if fragments do
not come before the reass_timeout, overlap, etc), the mbuf dynamic flags can be
updated by the PMD. This is updated in a subsequent patch.

Signed-off-by: Akhil Goyal <gakhil@marvell.com>
Change-Id: Ic20bb3af1ed599e8f2f3665d2d6c47b2e420e509
---
 doc/guides/nics/features.rst | 13 +++++
 lib/ethdev/ethdev_driver.h   | 55 +++++++++++++++++++++
 lib/ethdev/rte_ethdev.c      | 93 ++++++++++++++++++++++++++++++++++++
 lib/ethdev/rte_ethdev.h      | 91 +++++++++++++++++++++++++++++++++++
 lib/ethdev/version.map       |  5 ++
 5 files changed, 257 insertions(+)
  

> Subject: [PATCH v4 1/3] ethdev: introduce IP reassembly offload
> 
> IP Reassembly is a costly operation if it is done in software.
> The operation becomes even more costlier if IP fragments are encrypted.
> However, if it is offloaded to HW, it can considerably save application
> cycles.
> 
> Hence, a new offload feature is exposed in eth_dev ops for devices which can
> attempt IP reassembly of packets in hardware.
> - rte_eth_ip_reassembly_capability_get() - to get the maximum values
>   of reassembly configuration which can be set.
> - rte_eth_ip_reassembly_conf_set() - to set IP reassembly configuration
>   and to enable the feature in the PMD (to be called before rte_eth_dev_start()).
> - rte_eth_ip_reassembly_conf_get() - to get the current configuration
>   set in PMD.
> 
> Now when the offload is enabled using rte_eth_ip_reassembly_conf_set(),
> the resulting reassembled IP packet would be a typical segmented mbuf in
> case of success.
> 
> And if reassembly of IP fragments is failed or is incomplete (if fragments do
> not come before the reass_timeout, overlap, etc), the mbuf dynamic flags can
> be
> updated by the PMD. This is updated in a subsequent patch.
> 
> Signed-off-by: Akhil Goyal <gakhil@marvell.com>
> Change-Id: Ic20bb3af1ed599e8f2f3665d2d6c47b2e420e509
Please ignore the change-Id, will remove it in next version, or can be removed while
applying if no further comments.
  

On 2/4/2022 10:13 PM, Akhil Goyal wrote:
> IP Reassembly is a costly operation if it is done in software.
> The operation becomes even more costlier if IP fragments are encrypted.
> However, if it is offloaded to HW, it can considerably save application
> cycles.
> 
> Hence, a new offload feature is exposed in eth_dev ops for devices which can
> attempt IP reassembly of packets in hardware.
> - rte_eth_ip_reassembly_capability_get() - to get the maximum values
>    of reassembly configuration which can be set.
> - rte_eth_ip_reassembly_conf_set() - to set IP reassembly configuration
>    and to enable the feature in the PMD (to be called before rte_eth_dev_start()).
> - rte_eth_ip_reassembly_conf_get() - to get the current configuration
>    set in PMD.
> 
> Now when the offload is enabled using rte_eth_ip_reassembly_conf_set(),
> the resulting reassembled IP packet would be a typical segmented mbuf in
> case of success.
> 
> And if reassembly of IP fragments is failed or is incomplete (if fragments do
> not come before the reass_timeout, overlap, etc), the mbuf dynamic flags can be
> updated by the PMD. This is updated in a subsequent patch.
> 
> Signed-off-by: Akhil Goyal <gakhil@marvell.com>
> Change-Id: Ic20bb3af1ed599e8f2f3665d2d6c47b2e420e509
> ---
>   doc/guides/nics/features.rst | 13 +++++
>   lib/ethdev/ethdev_driver.h   | 55 +++++++++++++++++++++
>   lib/ethdev/rte_ethdev.c      | 93 ++++++++++++++++++++++++++++++++++++
>   lib/ethdev/rte_ethdev.h      | 91 +++++++++++++++++++++++++++++++++++
>   lib/ethdev/version.map       |  5 ++
>   5 files changed, 257 insertions(+)
> 
> diff --git a/doc/guides/nics/features.rst b/doc/guides/nics/features.rst
> index 27be2d2576..e6e0bbe9d8 100644
> --- a/doc/guides/nics/features.rst
> +++ b/doc/guides/nics/features.rst
> @@ -602,6 +602,19 @@ Supports inner packet L4 checksum.
>     ``tx_offload_capa,tx_queue_offload_capa:RTE_ETH_TX_OFFLOAD_OUTER_UDP_CKSUM``.
>   
>   
> +.. _nic_features_ip_reassembly:
> +
> +IP reassembly
> +-------------
> +
> +Supports IP reassembly in hardware.
> +
> +* **[provides] eth_dev_ops**: ``ip_reassemly_capability_get``,
> +  ``ip_reassembly_conf_get``, ``ip_reassembly_conf_set``.
> +* **[related]    API**: ``rte_eth_ip_reassembly_capability_get()``,
> +  ``rte_eth_ip_reassembly_conf_get()``, ``rte_eth_ip_reassembly_conf_set()``.
> +
> +

Need to update 'default.ini' to have this new feature.

>   .. _nic_features_shared_rx_queue:
>   
>   Shared Rx queue
> diff --git a/lib/ethdev/ethdev_driver.h b/lib/ethdev/ethdev_driver.h
> index d95605a355..8fe77f283f 100644
> --- a/lib/ethdev/ethdev_driver.h
> +++ b/lib/ethdev/ethdev_driver.h
> @@ -990,6 +990,54 @@ typedef int (*eth_representor_info_get_t)(struct rte_eth_dev *dev,
>   typedef int (*eth_rx_metadata_negotiate_t)(struct rte_eth_dev *dev,
>   				       uint64_t *features);
>   
> +/**
> + * @internal
> + * Get IP reassembly offload capability of a PMD.
> + *
> + * @param dev
> + *   Port (ethdev) handle
> + *
> + * @param[out] conf
> + *   IP reassembly capability supported by the PMD
> + *
> + * @return
> + *   Negative errno value on error, zero otherwise
> + */
> +typedef int (*eth_ip_reassembly_capability_get_t)(struct rte_eth_dev *dev,
> +				struct rte_eth_ip_reass_params *capa);
> +
> +/**
> + * @internal
> + * Get IP reassembly offload configuration parameters set in PMD.
> + *
> + * @param dev
> + *   Port (ethdev) handle
> + *
> + * @param[out] conf
> + *   Configuration parameters for IP reassembly.
> + *
> + * @return
> + *   Negative errno value on error, zero otherwise
> + */
> +typedef int (*eth_ip_reassembly_conf_get_t)(struct rte_eth_dev *dev,
> +				struct rte_eth_ip_reass_params *conf);
> +
> +/**
> + * @internal
> + * Set configuration parameters for enabling IP reassembly offload in hardware.
> + *
> + * @param dev
> + *   Port (ethdev) handle
> + *
> + * @param[in] conf
> + *   Configuration parameters for IP reassembly.
> + *
> + * @return
> + *   Negative errno value on error, zero otherwise
> + */
> +typedef int (*eth_ip_reassembly_conf_set_t)(struct rte_eth_dev *dev,
> +				const struct rte_eth_ip_reass_params *conf);
> +
>   /**
>    * @internal A structure containing the functions exported by an Ethernet driver.
>    */
> @@ -1186,6 +1234,13 @@ struct eth_dev_ops {
>   	 * kinds of metadata to the PMD
>   	 */
>   	eth_rx_metadata_negotiate_t rx_metadata_negotiate;
> +
> +	/** Get IP reassembly capability */
> +	eth_ip_reassembly_capability_get_t ip_reassembly_capability_get;
> +	/** Get IP reassembly configuration */
> +	eth_ip_reassembly_conf_get_t ip_reassembly_conf_get;
> +	/** Set IP reassembly configuration */
> +	eth_ip_reassembly_conf_set_t ip_reassembly_conf_set;
>   };
>   
>   /**
> diff --git a/lib/ethdev/rte_ethdev.c b/lib/ethdev/rte_ethdev.c
> index 29e21ad580..88ca4ce867 100644
> --- a/lib/ethdev/rte_ethdev.c
> +++ b/lib/ethdev/rte_ethdev.c
> @@ -6474,6 +6474,99 @@ rte_eth_rx_metadata_negotiate(uint16_t port_id, uint64_t *features)
>   		       (*dev->dev_ops->rx_metadata_negotiate)(dev, features));
>   }
>   
> +int
> +rte_eth_ip_reassembly_capability_get(uint16_t port_id,
> +				     struct rte_eth_ip_reass_params *reass_capa)

syntax, dpdk coding converntion doesn't align the next line, mostly it has
two tabs in next line.

Same comment valid for many instances below.

> +{
> +	struct rte_eth_dev *dev;
> +
> +	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
> +	dev = &rte_eth_devices[port_id];
> +
> +	if (dev->data->dev_configured == 0) {
> +		RTE_ETHDEV_LOG(ERR,
> +			"Device with port_id=%"PRIu16" is not configured.\n",

- while printing port_id, can use %u, 'PRIu16' has no benefit as far as I can see,
and I see both are used in existing code, I prefer %u but no strong option there

- The log doesn't mention from ip reassembly capability at all, assume you see this
log, will it give enough context to understand problem is related to ip reassembly?

- Andrew has a comment before that each log message should be unique, to be able
to detect location easily, exact same log message is used a few other locations.

> +			port_id);
> +		return -EINVAL;
> +	}
> +
> +	if (reass_capa == NULL) {

I still think all 'reass' usage should be 'reassembly', but again that is me.

> +		RTE_ETHDEV_LOG(ERR, "Cannot get reassembly capability to NULL");
> +		return -EINVAL;
> +	}
> +
> +	RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->ip_reassembly_capability_get,
> +				-ENOTSUP);
> +	memset(reass_capa, 0, sizeof(struct rte_eth_ip_reass_params));
> +
> +	return eth_err(port_id, (*dev->dev_ops->ip_reassembly_capability_get)
> +					(dev, reass_capa));
> +}
> +
> +int
> +rte_eth_ip_reassembly_conf_get(uint16_t port_id,
> +			       struct rte_eth_ip_reass_params *conf)
> +{
> +	struct rte_eth_dev *dev;
> +
> +	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
> +	dev = &rte_eth_devices[port_id];
> +
> +	if (dev->data->dev_configured == 0) {
> +		RTE_ETHDEV_LOG(ERR,
> +			"Device with port_id=%"PRIu16" is not configured.\n",
> +			port_id);
> +		return -EINVAL;
> +	}
> +
> +	if (conf == NULL) {
> +		RTE_ETHDEV_LOG(ERR, "Cannot get reassembly info to NULL");
> +		return -EINVAL;
> +	}
> +
> +	RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->ip_reassembly_conf_get,
> +				-ENOTSUP);
> +	memset(conf, 0, sizeof(struct rte_eth_ip_reass_params));

If user didn't call 'rte_eth_ip_reassembly_conf_set()' prior to this call, what
user will get here? All zeros, or can PMD fill with some defaults?
Or should this API set some kind of error in that case to highlight there is no
configuration?

> +	return eth_err(port_id,
> +		       (*dev->dev_ops->ip_reassembly_conf_get)(dev, conf));
> +}
> +
> +int
> +rte_eth_ip_reassembly_conf_set(uint16_t port_id,
> +			       const struct rte_eth_ip_reass_params *conf)
> +{
> +	struct rte_eth_dev *dev;
> +
> +	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
> +	dev = &rte_eth_devices[port_id];
> +
> +	if (dev->data->dev_configured == 0) {
> +		RTE_ETHDEV_LOG(ERR,
> +			"Device with port_id=%"PRIu16" is not configured.\n",
> +			port_id);
> +		return -EINVAL;
> +	}
> +
> +	if (dev->data->dev_started != 0) {
> +		RTE_ETHDEV_LOG(ERR,
> +			"Device with port_id=%"PRIu16" started,\n"
> +			"cannot configure IP reassembly params.\n",
> +			port_id);
> +		return -EINVAL;
> +	}
> +
> +	if (conf == NULL) {
> +		RTE_ETHDEV_LOG(ERR,
> +				"Invalid IP reassembly configuration (NULL)\n");
> +		return -EINVAL;
> +	}
> +
> +	RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->ip_reassembly_conf_set,
> +				-ENOTSUP);
> +	return eth_err(port_id,
> +		       (*dev->dev_ops->ip_reassembly_conf_set)(dev, conf));
> +}
> +
>   RTE_LOG_REGISTER_DEFAULT(rte_eth_dev_logtype, INFO);
>   
>   RTE_INIT(ethdev_init_telemetry)
> diff --git a/lib/ethdev/rte_ethdev.h b/lib/ethdev/rte_ethdev.h
> index 147cc1ced3..ecc5cd50b9 100644
> --- a/lib/ethdev/rte_ethdev.h
> +++ b/lib/ethdev/rte_ethdev.h
> @@ -1794,6 +1794,29 @@ enum rte_eth_representor_type {
>   	RTE_ETH_REPRESENTOR_PF,   /**< representor of Physical Function. */
>   };
>   
> +/* Flag to offload IP reassembly for IPv4 packets. */
> +#define RTE_ETH_DEV_REASSEMBLY_F_IPV4 (RTE_BIT32(0))
> +/* Flag to offload IP reassembly for IPv6 packets. */
> +#define RTE_ETH_DEV_REASSEMBLY_F_IPV6 (RTE_BIT32(1))
> +/**
> + * A structure used to get/set IP reassembly configuration/capability.
> + *
> + * If rte_eth_ip_reassembly_capability_get() returns 0, IP reassembly can be
> + * enabled using rte_eth_ip_reassembly_conf_set() and params values lower than
> + * capability can be set in the PMD.

Can you please clarify "params values lower than capability"? Is it refering to
'max_frags'?

> + */
> +struct rte_eth_ip_reass_params {
> +	/** Maximum time in ms which PMD can wait for other fragments. */
> +	uint32_t reass_timeout_ms;

Other variables in the struct is missing 'reass_' prefix, like it is 'max_frags'
instead of 'reass_max_frags', should we do the same for this variable?

> +	/** Maximum number of fragments that can be reassembled. */
> +	uint16_t max_frags;
> +	/**
> +	 * Flags to enable reassembly of packet types -
> +	 * RTE_ETH_DEV_REASSEMBLY_F_xxx.
> +	 */
> +	uint16_t flags;
> +};
> +
>   /**
>    * A structure used to retrieve the contextual information of
>    * an Ethernet device, such as the controlling driver of the
> @@ -5202,6 +5225,74 @@ int rte_eth_representor_info_get(uint16_t port_id,
>   __rte_experimental
>   int rte_eth_rx_metadata_negotiate(uint16_t port_id, uint64_t *features);
>   
> +/**
> + * @warning
> + * @b EXPERIMENTAL: this API may change without prior notice
> + *
> + * Get IP reassembly capabilities supported by the PMD,
> + *
> + * @param port_id
> + *   The port identifier of the device.
> + * @param conf
> + *   A pointer to rte_eth_ip_reass_params structure.
> + * @return
> + *   - (-ENOTSUP) if offload configuration is not supported by device.
> + *   - (-ENODEV) if *port_id* invalid.
> + *   - (-EIO) if device is removed.
> + *   - (0) on success.
> + */
> +__rte_experimental
> +int rte_eth_ip_reassembly_capability_get(uint16_t port_id,
> +					 struct rte_eth_ip_reass_params *conf);
> +
> +/**
> + * @warning
> + * @b EXPERIMENTAL: this API may change without prior notice
> + *
> + * Get IP reassembly configuration parameters currently set in PMD,
> + * if device Rx offload flag (RTE_ETH_RX_OFFLOAD_IP_REASSEMBLY) is

This flag is no more. Needs to updated the sentences.

> + * enabled and the PMD supports IP reassembly offload.
> + *

Device needs to be configured to call this API, should it be documented here?

> + * @param port_id
> + *   The port identifier of the device.
> + * @param conf
> + *   A pointer to rte_eth_ip_reass_params structure.
> + * @return
> + *   - (-ENOTSUP) if offload configuration is not supported by device.
> + *   - (-EINVAL) if offload is not enabled in rte_eth_conf.
> + *   - (-ENODEV) if *port_id* invalid.
> + *   - (-EIO) if device is removed.
> + *   - (0) on success.
> + */
> +__rte_experimental
> +int rte_eth_ip_reassembly_conf_get(uint16_t port_id,
> +				   struct rte_eth_ip_reass_params *conf);
> +
> +/**
> + * @warning
> + * @b EXPERIMENTAL: this API may change without prior notice
> + *
> + * Set IP reassembly configuration parameters if the PMD supports IP reassembly
> + * offload. User should first call rte_eth_ip_reassembly_capability_get() to
> + * check the maximum values supported by the PMD before setting the
> + * configuration. The use of this API is mandatory to enable this feature and
> + * should be called before rte_eth_dev_start().
> + *

Not sure if above cause confusion, what above mean is this API should be called
when device is stopped. When you call it should be called before 'te_eth_dev_start()'
can it be taken as it should be called from first ever start() API?
If you think above is clear enough we can continue as it is.

> + * @param port_id
> + *   The port identifier of the device.
> + * @param conf
> + *   A pointer to rte_eth_ip_reass_params structure.
> + * @return
> + *   - (-ENOTSUP) if offload configuration is not supported by device.
> + *   - (-ENODEV) if *port_id* invalid.
> + *   - (-EIO) if device is removed.
> + *   - (0) on success.
> + */
> +__rte_experimental
> +int rte_eth_ip_reassembly_conf_set(uint16_t port_id,
> +				   const struct rte_eth_ip_reass_params *conf);
> +
> +
>   #include <rte_ethdev_core.h>
>   
>   /**
> diff --git a/lib/ethdev/version.map b/lib/ethdev/version.map
> index c2fb0669a4..e22c102818 100644
> --- a/lib/ethdev/version.map
> +++ b/lib/ethdev/version.map
> @@ -256,6 +256,11 @@ EXPERIMENTAL {
>   	rte_flow_flex_item_create;
>   	rte_flow_flex_item_release;
>   	rte_flow_pick_transfer_proxy;
> +
> +	# added in 22.03
> +	rte_eth_ip_reassembly_capability_get;
> +	rte_eth_ip_reassembly_conf_get;
> +	rte_eth_ip_reassembly_conf_set;
>   };
>   
>   INTERNAL {
  

Hi Ferruh,

Thanks for review,
Will send the next version soon.
Please see the comments inline.
> On 2/4/2022 10:13 PM, Akhil Goyal wrote:
> > IP Reassembly is a costly operation if it is done in software.
> > The operation becomes even more costlier if IP fragments are encrypted.
> > However, if it is offloaded to HW, it can considerably save application
> > cycles.
> >
> > Hence, a new offload feature is exposed in eth_dev ops for devices which
> can
> > attempt IP reassembly of packets in hardware.
> > - rte_eth_ip_reassembly_capability_get() - to get the maximum values
> >    of reassembly configuration which can be set.
> > - rte_eth_ip_reassembly_conf_set() - to set IP reassembly configuration
> >    and to enable the feature in the PMD (to be called before
> rte_eth_dev_start()).
> > - rte_eth_ip_reassembly_conf_get() - to get the current configuration
> >    set in PMD.
> >
> > Now when the offload is enabled using
> rte_eth_ip_reassembly_conf_set(),
> > the resulting reassembled IP packet would be a typical segmented mbuf in
> > case of success.
> >
> > And if reassembly of IP fragments is failed or is incomplete (if fragments do
> > not come before the reass_timeout, overlap, etc), the mbuf dynamic flags
> can be
> > updated by the PMD. This is updated in a subsequent patch.
> >
> > Signed-off-by: Akhil Goyal <gakhil@marvell.com>
> > Change-Id: Ic20bb3af1ed599e8f2f3665d2d6c47b2e420e509
> > ---
> >   doc/guides/nics/features.rst | 13 +++++
> >   lib/ethdev/ethdev_driver.h   | 55 +++++++++++++++++++++
> >   lib/ethdev/rte_ethdev.c      | 93
> ++++++++++++++++++++++++++++++++++++
> >   lib/ethdev/rte_ethdev.h      | 91
> +++++++++++++++++++++++++++++++++++
> >   lib/ethdev/version.map       |  5 ++
> >   5 files changed, 257 insertions(+)
> >
> > diff --git a/doc/guides/nics/features.rst b/doc/guides/nics/features.rst
> > index 27be2d2576..e6e0bbe9d8 100644
> > --- a/doc/guides/nics/features.rst
> > +++ b/doc/guides/nics/features.rst
> > @@ -602,6 +602,19 @@ Supports inner packet L4 checksum.
> >
> ``tx_offload_capa,tx_queue_offload_capa:RTE_ETH_TX_OFFLOAD_OUTER_
> UDP_CKSUM``.
> >
> >
> > +.. _nic_features_ip_reassembly:
> > +
> > +IP reassembly
> > +-------------
> > +
> > +Supports IP reassembly in hardware.
> > +
> > +* **[provides] eth_dev_ops**: ``ip_reassemly_capability_get``,
> > +  ``ip_reassembly_conf_get``, ``ip_reassembly_conf_set``.
> > +* **[related]    API**: ``rte_eth_ip_reassembly_capability_get()``,
> > +  ``rte_eth_ip_reassembly_conf_get()``,
> ``rte_eth_ip_reassembly_conf_set()``.
> > +
> > +
> 
> Need to update 'default.ini' to have this new feature.
Yes, It got missed.

> 
> >   .. _nic_features_shared_rx_queue:
> >
> >   Shared Rx queue
> > diff --git a/lib/ethdev/ethdev_driver.h b/lib/ethdev/ethdev_driver.h
> > index d95605a355..8fe77f283f 100644
> > --- a/lib/ethdev/ethdev_driver.h
> > +++ b/lib/ethdev/ethdev_driver.h
> > @@ -990,6 +990,54 @@ typedef int (*eth_representor_info_get_t)(struct
> rte_eth_dev *dev,
> >   typedef int (*eth_rx_metadata_negotiate_t)(struct rte_eth_dev *dev,
> >   				       uint64_t *features);
> >
> > +/**
> > + * @internal
> > + * Get IP reassembly offload capability of a PMD.
> > + *
> > + * @param dev
> > + *   Port (ethdev) handle
> > + *
> > + * @param[out] conf
> > + *   IP reassembly capability supported by the PMD
> > + *
> > + * @return
> > + *   Negative errno value on error, zero otherwise
> > + */
> > +typedef int (*eth_ip_reassembly_capability_get_t)(struct rte_eth_dev
> *dev,
> > +				struct rte_eth_ip_reass_params *capa);
> > +
> > +/**
> > + * @internal
> > + * Get IP reassembly offload configuration parameters set in PMD.
> > + *
> > + * @param dev
> > + *   Port (ethdev) handle
> > + *
> > + * @param[out] conf
> > + *   Configuration parameters for IP reassembly.
> > + *
> > + * @return
> > + *   Negative errno value on error, zero otherwise
> > + */
> > +typedef int (*eth_ip_reassembly_conf_get_t)(struct rte_eth_dev *dev,
> > +				struct rte_eth_ip_reass_params *conf);
> > +
> > +/**
> > + * @internal
> > + * Set configuration parameters for enabling IP reassembly offload in
> hardware.
> > + *
> > + * @param dev
> > + *   Port (ethdev) handle
> > + *
> > + * @param[in] conf
> > + *   Configuration parameters for IP reassembly.
> > + *
> > + * @return
> > + *   Negative errno value on error, zero otherwise
> > + */
> > +typedef int (*eth_ip_reassembly_conf_set_t)(struct rte_eth_dev *dev,
> > +				const struct rte_eth_ip_reass_params
> *conf);
> > +
> >   /**
> >    * @internal A structure containing the functions exported by an Ethernet
> driver.
> >    */
> > @@ -1186,6 +1234,13 @@ struct eth_dev_ops {
> >   	 * kinds of metadata to the PMD
> >   	 */
> >   	eth_rx_metadata_negotiate_t rx_metadata_negotiate;
> > +
> > +	/** Get IP reassembly capability */
> > +	eth_ip_reassembly_capability_get_t ip_reassembly_capability_get;
> > +	/** Get IP reassembly configuration */
> > +	eth_ip_reassembly_conf_get_t ip_reassembly_conf_get;
> > +	/** Set IP reassembly configuration */
> > +	eth_ip_reassembly_conf_set_t ip_reassembly_conf_set;
> >   };
> >
> >   /**
> > diff --git a/lib/ethdev/rte_ethdev.c b/lib/ethdev/rte_ethdev.c
> > index 29e21ad580..88ca4ce867 100644
> > --- a/lib/ethdev/rte_ethdev.c
> > +++ b/lib/ethdev/rte_ethdev.c
> > @@ -6474,6 +6474,99 @@ rte_eth_rx_metadata_negotiate(uint16_t
> port_id, uint64_t *features)
> >   		       (*dev->dev_ops->rx_metadata_negotiate)(dev,
> features));
> >   }
> >
> > +int
> > +rte_eth_ip_reassembly_capability_get(uint16_t port_id,
> > +				     struct rte_eth_ip_reass_params
> *reass_capa)
> 
> syntax, dpdk coding converntion doesn't align the next line, mostly it has
> two tabs in next line.
> 
> Same comment valid for many instances below.
> 

Ok
> > +{
> > +	struct rte_eth_dev *dev;
> > +
> > +	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
> > +	dev = &rte_eth_devices[port_id];
> > +
> > +	if (dev->data->dev_configured == 0) {
> > +		RTE_ETHDEV_LOG(ERR,
> > +			"Device with port_id=%"PRIu16" is not
> configured.\n",
> 
> - while printing port_id, can use %u, 'PRIu16' has no benefit as far as I can
> see,
> and I see both are used in existing code, I prefer %u but no strong option
> there
> 
> - The log doesn't mention from ip reassembly capability at all, assume you
> see this
> log, will it give enough context to understand problem is related to ip
> reassembly?
> 
> - Andrew has a comment before that each log message should be unique, to
> be able
> to detect location easily, exact same log message is used a few other
> locations.

Ok will update the logs.
> 
> > +			port_id);
> > +		return -EINVAL;
> > +	}
> > +
> > +	if (reass_capa == NULL) {
> 
> I still think all 'reass' usage should be 'reassembly', but again that is me.

Ok will change it.

> 
> > +		RTE_ETHDEV_LOG(ERR, "Cannot get reassembly capability to
> NULL");
> > +		return -EINVAL;
> > +	}
> > +
> > +	RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops-
> >ip_reassembly_capability_get,
> > +				-ENOTSUP);
> > +	memset(reass_capa, 0, sizeof(struct rte_eth_ip_reass_params));
> > +
> > +	return eth_err(port_id, (*dev->dev_ops-
> >ip_reassembly_capability_get)
> > +					(dev, reass_capa));
> > +}
> > +
> > +int
> > +rte_eth_ip_reassembly_conf_get(uint16_t port_id,
> > +			       struct rte_eth_ip_reass_params *conf)
> > +{
> > +	struct rte_eth_dev *dev;
> > +
> > +	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
> > +	dev = &rte_eth_devices[port_id];
> > +
> > +	if (dev->data->dev_configured == 0) {
> > +		RTE_ETHDEV_LOG(ERR,
> > +			"Device with port_id=%"PRIu16" is not
> configured.\n",
> > +			port_id);
> > +		return -EINVAL;
> > +	}
> > +
> > +	if (conf == NULL) {
> > +		RTE_ETHDEV_LOG(ERR, "Cannot get reassembly info to
> NULL");
> > +		return -EINVAL;
> > +	}
> > +
> > +	RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops-
> >ip_reassembly_conf_get,
> > +				-ENOTSUP);
> > +	memset(conf, 0, sizeof(struct rte_eth_ip_reass_params));
> 
> If user didn't call 'rte_eth_ip_reassembly_conf_set()' prior to this call, what
> user will get here? All zeros, or can PMD fill with some defaults?
> Or should this API set some kind of error in that case to highlight there is no
> configuration?

Ok I will update the doxygen comments for this. I believe it should give error.

> 
> > +	return eth_err(port_id,
> > +		       (*dev->dev_ops->ip_reassembly_conf_get)(dev, conf));
> > +}
> > +
> > +int
> > +rte_eth_ip_reassembly_conf_set(uint16_t port_id,
> > +			       const struct rte_eth_ip_reass_params *conf)
> > +{
> > +	struct rte_eth_dev *dev;
> > +
> > +	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
> > +	dev = &rte_eth_devices[port_id];
> > +
> > +	if (dev->data->dev_configured == 0) {
> > +		RTE_ETHDEV_LOG(ERR,
> > +			"Device with port_id=%"PRIu16" is not
> configured.\n",
> > +			port_id);
> > +		return -EINVAL;
> > +	}
> > +
> > +	if (dev->data->dev_started != 0) {
> > +		RTE_ETHDEV_LOG(ERR,
> > +			"Device with port_id=%"PRIu16" started,\n"
> > +			"cannot configure IP reassembly params.\n",
> > +			port_id);
> > +		return -EINVAL;
> > +	}
> > +
> > +	if (conf == NULL) {
> > +		RTE_ETHDEV_LOG(ERR,
> > +				"Invalid IP reassembly configuration
> (NULL)\n");
> > +		return -EINVAL;
> > +	}
> > +
> > +	RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops-
> >ip_reassembly_conf_set,
> > +				-ENOTSUP);
> > +	return eth_err(port_id,
> > +		       (*dev->dev_ops->ip_reassembly_conf_set)(dev, conf));
> > +}
> > +
> >   RTE_LOG_REGISTER_DEFAULT(rte_eth_dev_logtype, INFO);
> >
> >   RTE_INIT(ethdev_init_telemetry)
> > diff --git a/lib/ethdev/rte_ethdev.h b/lib/ethdev/rte_ethdev.h
> > index 147cc1ced3..ecc5cd50b9 100644
> > --- a/lib/ethdev/rte_ethdev.h
> > +++ b/lib/ethdev/rte_ethdev.h
> > @@ -1794,6 +1794,29 @@ enum rte_eth_representor_type {
> >   	RTE_ETH_REPRESENTOR_PF,   /**< representor of Physical Function.
> */
> >   };
> >
> > +/* Flag to offload IP reassembly for IPv4 packets. */
> > +#define RTE_ETH_DEV_REASSEMBLY_F_IPV4 (RTE_BIT32(0))
> > +/* Flag to offload IP reassembly for IPv6 packets. */
> > +#define RTE_ETH_DEV_REASSEMBLY_F_IPV6 (RTE_BIT32(1))
> > +/**
> > + * A structure used to get/set IP reassembly configuration/capability.
> > + *
> > + * If rte_eth_ip_reassembly_capability_get() returns 0, IP reassembly can
> be
> > + * enabled using rte_eth_ip_reassembly_conf_set() and params values
> lower than
> > + * capability can be set in the PMD.
> 
> Can you please clarify "params values lower than capability"? Is it refering to
> 'max_frags'?

It can be for all three params.
Timeout should be less than the capability of the driver.
It may be the case, app want to enable only IPv4 and not IPv6 but PMD can support both.
So conf_set() params should be lesser compared to capability.

> 
> > + */
> > +struct rte_eth_ip_reass_params {
> > +	/** Maximum time in ms which PMD can wait for other fragments.
> */
> > +	uint32_t reass_timeout_ms;
> 
> Other variables in the struct is missing 'reass_' prefix, like it is 'max_frags'
> instead of 'reass_max_frags', should we do the same for this variable?
> 

Ok I will drop reass from reass_timeout_ms. As it is part of rte_eth_ip_reass_params.
Or I should say rte_eth_ip_reassembly_params - as per your comment.

> > +	/** Maximum number of fragments that can be reassembled. */
> > +	uint16_t max_frags;
> > +	/**
> > +	 * Flags to enable reassembly of packet types -
> > +	 * RTE_ETH_DEV_REASSEMBLY_F_xxx.
> > +	 */
> > +	uint16_t flags;
> > +};
> > +
> >   /**
> >    * A structure used to retrieve the contextual information of
> >    * an Ethernet device, such as the controlling driver of the
> > @@ -5202,6 +5225,74 @@ int rte_eth_representor_info_get(uint16_t
> port_id,
> >   __rte_experimental
> >   int rte_eth_rx_metadata_negotiate(uint16_t port_id, uint64_t
> *features);
> >
> > +/**
> > + * @warning
> > + * @b EXPERIMENTAL: this API may change without prior notice
> > + *
> > + * Get IP reassembly capabilities supported by the PMD,
> > + *
> > + * @param port_id
> > + *   The port identifier of the device.
> > + * @param conf
> > + *   A pointer to rte_eth_ip_reass_params structure.
> > + * @return
> > + *   - (-ENOTSUP) if offload configuration is not supported by device.
> > + *   - (-ENODEV) if *port_id* invalid.
> > + *   - (-EIO) if device is removed.
> > + *   - (0) on success.
> > + */
> > +__rte_experimental
> > +int rte_eth_ip_reassembly_capability_get(uint16_t port_id,
> > +					 struct rte_eth_ip_reass_params
> *conf);
> > +
> > +/**
> > + * @warning
> > + * @b EXPERIMENTAL: this API may change without prior notice
> > + *
> > + * Get IP reassembly configuration parameters currently set in PMD,
> > + * if device Rx offload flag (RTE_ETH_RX_OFFLOAD_IP_REASSEMBLY) is
> 
> This flag is no more. Needs to updated the sentences.

Ahh my bad. Will correct it.

> 
> > + * enabled and the PMD supports IP reassembly offload.
> > + *
> 
> Device needs to be configured to call this API, should it be documented
> here?

Ok will add one.

> 
> > + * @param port_id
> > + *   The port identifier of the device.
> > + * @param conf
> > + *   A pointer to rte_eth_ip_reass_params structure.
> > + * @return
> > + *   - (-ENOTSUP) if offload configuration is not supported by device.
> > + *   - (-EINVAL) if offload is not enabled in rte_eth_conf.
> > + *   - (-ENODEV) if *port_id* invalid.
> > + *   - (-EIO) if device is removed.
> > + *   - (0) on success.
> > + */
> > +__rte_experimental
> > +int rte_eth_ip_reassembly_conf_get(uint16_t port_id,
> > +				   struct rte_eth_ip_reass_params *conf);
> > +
> > +/**
> > + * @warning
> > + * @b EXPERIMENTAL: this API may change without prior notice
> > + *
> > + * Set IP reassembly configuration parameters if the PMD supports IP
> reassembly
> > + * offload. User should first call rte_eth_ip_reassembly_capability_get()
> to
> > + * check the maximum values supported by the PMD before setting the
> > + * configuration. The use of this API is mandatory to enable this feature
> and
> > + * should be called before rte_eth_dev_start().
> > + *
> 
> Not sure if above cause confusion, what above mean is this API should be
> called
> when device is stopped. When you call it should be called before
> 'te_eth_dev_start()'
> can it be taken as it should be called from first ever start() API?
> If you think above is clear enough we can continue as it is.

I believe dev_start() should be called after conf_set(). So we can continue as is.
No requirement for first ever start call.

> 
> > + * @param port_id
> > + *   The port identifier of the device.
> > + * @param conf
> > + *   A pointer to rte_eth_ip_reass_params structure.
> > + * @return
> > + *   - (-ENOTSUP) if offload configuration is not supported by device.
> > + *   - (-ENODEV) if *port_id* invalid.
> > + *   - (-EIO) if device is removed.
> > + *   - (0) on success.
> > + */
> > +__rte_experimental
> > +int rte_eth_ip_reassembly_conf_set(uint16_t port_id,
> > +				   const struct rte_eth_ip_reass_params
> *conf);
> > +
> > +
> >   #include <rte_ethdev_core.h>
> >
> >   /**
> > diff --git a/lib/ethdev/version.map b/lib/ethdev/version.map
> > index c2fb0669a4..e22c102818 100644
> > --- a/lib/ethdev/version.map
> > +++ b/lib/ethdev/version.map
> > @@ -256,6 +256,11 @@ EXPERIMENTAL {
> >   	rte_flow_flex_item_create;
> >   	rte_flow_flex_item_release;
> >   	rte_flow_pick_transfer_proxy;
> > +
> > +	# added in 22.03
> > +	rte_eth_ip_reassembly_capability_get;
> > +	rte_eth_ip_reassembly_conf_get;
> > +	rte_eth_ip_reassembly_conf_set;
> >   };
> >
> >   INTERNAL {