Re: [PATCH v4 2/3] ethdev: add mbuf dynfield for incomplete IP reassembly

[Ferruh Yigit <ferruh.yigit@intel.com>]

On 2/4/2022 10:13 PM, Akhil Goyal wrote:
> Hardware IP reassembly may be incomplete for multiple reasons like
> reassembly timeout reached, duplicate fragments, etc.
> To save application cycles to process these packets again, a new
> mbuf dynflag is added to show that the mbuf received is not
> reassembled properly.
> 
> Now if this dynflag is set, application can retrieve corresponding
> chain of mbufs using mbuf dynfield set by the PMD. Now, it will be
> up to application to either drop those fragments or wait for more time.
> 
> Signed-off-by: Akhil Goyal <gakhil@marvell.com>
> Change-Id: I118847cd6269da7e6313ac4e0d970d790dfef1ff
> ---
>   lib/ethdev/ethdev_driver.h |  8 ++++++++
>   lib/ethdev/rte_ethdev.c    | 28 ++++++++++++++++++++++++++++
>   lib/ethdev/rte_ethdev.h    | 17 +++++++++++++++++
>   lib/ethdev/version.map     |  1 +
>   lib/mbuf/rte_mbuf_dyn.h    |  9 +++++++++
>   5 files changed, 63 insertions(+)
> 
> diff --git a/lib/ethdev/ethdev_driver.h b/lib/ethdev/ethdev_driver.h
> index 8fe77f283f..6cfb266f7d 100644
> --- a/lib/ethdev/ethdev_driver.h
> +++ b/lib/ethdev/ethdev_driver.h
> @@ -1707,6 +1707,14 @@ int
>   rte_eth_hairpin_queue_peer_unbind(uint16_t cur_port, uint16_t cur_queue,
>   				  uint32_t direction);
>   
> +/**
> + * @internal
> + * Register mbuf dynamic field and flag for IP reassembly incomplete case.
> + */
> +__rte_internal
> +int
> +rte_eth_ip_reass_dynfield_register(int *field_offset, int *flag);
> +
>   
>   /*
>    * Legacy ethdev API used internally by drivers.
> diff --git a/lib/ethdev/rte_ethdev.c b/lib/ethdev/rte_ethdev.c
> index 88ca4ce867..48367dbec1 100644
> --- a/lib/ethdev/rte_ethdev.c
> +++ b/lib/ethdev/rte_ethdev.c
> @@ -6567,6 +6567,34 @@ rte_eth_ip_reassembly_conf_set(uint16_t port_id,
>   		       (*dev->dev_ops->ip_reassembly_conf_set)(dev, conf));
>   }
>   
> +int
> +rte_eth_ip_reass_dynfield_register(int *field_offset, int *flag_offset)
> +{
> +	static const struct rte_mbuf_dynfield field_desc = {
> +		.name = RTE_MBUF_DYNFIELD_IP_REASS_NAME,
> +		.size = sizeof(rte_eth_ip_reass_dynfield_t),
> +		.align = __alignof__(rte_eth_ip_reass_dynfield_t),
> +	};
> +	static const struct rte_mbuf_dynflag ip_reass_dynflag = {
> +		.name = RTE_MBUF_DYNFLAG_IP_REASS_INCOMPLETE_NAME,
> +	};
> +	int offset;
> +
> +	offset = rte_mbuf_dynfield_register(&field_desc);
> +	if (offset < 0)
> +		return -1;
> +	if (field_offset != NULL)
> +		*field_offset = offset;
> +
> +	offset = rte_mbuf_dynflag_register(&ip_reass_dynflag);
> +	if (offset < 0)
> +		return -1;
> +	if (flag_offset != NULL)
> +		*flag_offset = offset;
> +
> +	return 0;
> +}
> +

How mandatory is this field for the feature?

If 'rte_eth_ip_reass_dynfield_register()' fails, what PMD should do?
Should this API called before 'rte_eth_ip_reassembly_capability_get()' and
if registering dnyfield fails should PMD return feature as not supported?

Can you please describe this dependency, preferable in the
'rte_eth_ip_reassembly_capability_get()' doxygen comment?

>   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 ecc5cd50b9..ce35023c40 100644
> --- a/lib/ethdev/rte_ethdev.h
> +++ b/lib/ethdev/rte_ethdev.h
> @@ -5292,6 +5292,23 @@ __rte_experimental
>   int rte_eth_ip_reassembly_conf_set(uint16_t port_id,
>   				   const struct rte_eth_ip_reass_params *conf);
>   
> +/**
> + * In case of IP reassembly offload failure, ol_flags in mbuf will be
> + * updated with dynamic flag and packets will be returned without alteration.
> + * The application can retrieve the attached fragments using mbuf dynamic field.
> + */
> +typedef struct {
> +	/**
> +	 * Next fragment packet. Application should fetch dynamic field of
> +	 * each fragment until a NULL is received and nb_frags is 0.
> +	 */
> +	struct rte_mbuf *next_frag;
> +	/** Time spent(in ms) by HW in waiting for further fragments. */
> +	uint16_t time_spent;
> +	/** Number of more fragments attached in mbuf dynamic fields. */
> +	uint16_t nb_frags;
> +} rte_eth_ip_reass_dynfield_t;
> +
>   
>   #include <rte_ethdev_core.h>
>   
> diff --git a/lib/ethdev/version.map b/lib/ethdev/version.map
> index e22c102818..d6de8d402e 100644
> --- a/lib/ethdev/version.map
> +++ b/lib/ethdev/version.map
> @@ -284,6 +284,7 @@ INTERNAL {
>   	rte_eth_hairpin_queue_peer_bind;
>   	rte_eth_hairpin_queue_peer_unbind;
>   	rte_eth_hairpin_queue_peer_update;
> +	rte_eth_ip_reass_dynfield_register;
>   	rte_eth_representor_id_get;
>   	rte_eth_switch_domain_alloc;
>   	rte_eth_switch_domain_free;
> diff --git a/lib/mbuf/rte_mbuf_dyn.h b/lib/mbuf/rte_mbuf_dyn.h
> index 29abe8da53..299638513b 100644
> --- a/lib/mbuf/rte_mbuf_dyn.h
> +++ b/lib/mbuf/rte_mbuf_dyn.h
> @@ -320,6 +320,15 @@ int rte_mbuf_dyn_rx_timestamp_register(int *field_offset, uint64_t *rx_flag);
>    */
>   int rte_mbuf_dyn_tx_timestamp_register(int *field_offset, uint64_t *tx_flag);
>   
> +/**
> + * For the PMDs which support IP reassembly of packets, PMD will updated the
> + * packet with RTE_MBUF_DYNFLAG_IP_REASS_INCOMPLETE_NAME to denote that
> + * IP reassembly is incomplete and application can retrieve the packets back
> + * using RTE_MBUF_DYNFIELD_IP_REASS_NAME.
> + */
> +#define RTE_MBUF_DYNFIELD_IP_REASS_NAME "rte_eth_ip_reass_dynfield"
> +#define RTE_MBUF_DYNFLAG_IP_REASS_INCOMPLETE_NAME "rte_eth_ip_reass_incomplete_dynflag"
> +

Needs Olivier's comment/ack.