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

Message ID 20220204221334.3551574-3-gakhil@marvell.com (mailing list archive)
State Superseded, archived
Delegated to: Ferruh Yigit
Headers
Series ethdev: introduce IP reassembly offload |

Checks

Context Check Description
ci/checkpatch warning coding style issues

Commit Message

Akhil Goyal Feb. 4, 2022, 10:13 p.m. UTC
  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(+)
  

Comments

Ferruh Yigit Feb. 7, 2022, 1:58 p.m. UTC | #1
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.
  
Akhil Goyal Feb. 7, 2022, 2:20 p.m. UTC | #2
> 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?

Dynfield is added for the error/ incomplete reassembly case.
If the dynfield is not registered, the feature can work well for success scenarios.
Dynfield registration is responsibility of PMD and it is upto the driver to decide
when to set the dynfield. The registration can be done in conf_set() API.

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

Capability get is not a place where the feature is enabled.
Dynfield should be registered only in case the feature is enabled.
I will add following line in conf_set() doxygen comment.

The PMD should call 'rte_eth_ip_reass_dynfield_register()' when
the feature is enabled and return error if dynfield is not registered.
Dynfield is needed to give packets back to the application in case the
reassembly is not complete.

> 
> >   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.
@Olivier: could you please comment on this?
  
Ferruh Yigit Feb. 7, 2022, 2:56 p.m. UTC | #3
On 2/7/2022 2:20 PM, Akhil Goyal wrote:
>> 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?
> 
> Dynfield is added for the error/ incomplete reassembly case.
> If the dynfield is not registered, the feature can work well for success scenarios.
> Dynfield registration is responsibility of PMD and it is upto the driver to decide
> when to set the dynfield. The registration can be done in conf_set() API.
> 
>>
>> Can you please describe this dependency, preferable in the
>> 'rte_eth_ip_reassembly_capability_get()' doxygen comment?
> 
> Capability get is not a place where the feature is enabled.
> Dynfield should be registered only in case the feature is enabled.
> I will add following line in conf_set() doxygen comment.
> 
> The PMD should call 'rte_eth_ip_reass_dynfield_register()' when
> the feature is enabled and return error if dynfield is not registered.
> Dynfield is needed to give packets back to the application in case the
> reassembly is not complete.
> 

Can you also clarify what PMD should do related to the ip reassembly feature
when registering dynfield fails? Should it keep the feature enabled or disabled?

This will also clarify for the application, if application detects that
'RTE_MBUF_DYNFIELD_IP_REASS_NAME' is not registered how it should behave?
Ignore it? Fail? Disable ip reassembly?

>>
>>>    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.
> @Olivier: could you please comment on this?
  
Akhil Goyal Feb. 7, 2022, 4:20 p.m. UTC | #4
> On 2/7/2022 2:20 PM, Akhil Goyal wrote:
> >> 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?
> >
> > Dynfield is added for the error/ incomplete reassembly case.
> > If the dynfield is not registered, the feature can work well for success
> scenarios.
> > Dynfield registration is responsibility of PMD and it is upto the driver to decide
> > when to set the dynfield. The registration can be done in conf_set() API.
> >
> >>
> >> Can you please describe this dependency, preferable in the
> >> 'rte_eth_ip_reassembly_capability_get()' doxygen comment?
> >
> > Capability get is not a place where the feature is enabled.
> > Dynfield should be registered only in case the feature is enabled.
> > I will add following line in conf_set() doxygen comment.
> >
> > The PMD should call 'rte_eth_ip_reass_dynfield_register()' when
> > the feature is enabled and return error if dynfield is not registered.
> > Dynfield is needed to give packets back to the application in case the
> > reassembly is not complete.
> >
> 
> Can you also clarify what PMD should do related to the ip reassembly feature
> when registering dynfield fails? Should it keep the feature enabled or disabled?
> 
> This will also clarify for the application, if application detects that
> 'RTE_MBUF_DYNFIELD_IP_REASS_NAME' is not registered how it should
> behave?
> Ignore it? Fail? Disable ip reassembly?

The PMD can return error in the conf_set API, if dynfield is not successfully
registered. Or in case of inline IPsec, the PMD can return the error while
creating inline security session.

Hence the application will get to know if the dynfield is successfully configured
Or not.
If the dynfield is not configured, PMD will return configuration error
(either in conf_set or in security_session_create) and feature
will not be enabled.
  
Ferruh Yigit Feb. 7, 2022, 4:41 p.m. UTC | #5
On 2/7/2022 4:20 PM, Akhil Goyal wrote:
>> On 2/7/2022 2:20 PM, Akhil Goyal wrote:
>>>> 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?
>>>
>>> Dynfield is added for the error/ incomplete reassembly case.
>>> If the dynfield is not registered, the feature can work well for success
>> scenarios.
>>> Dynfield registration is responsibility of PMD and it is upto the driver to decide
>>> when to set the dynfield. The registration can be done in conf_set() API.
>>>
>>>>
>>>> Can you please describe this dependency, preferable in the
>>>> 'rte_eth_ip_reassembly_capability_get()' doxygen comment?
>>>
>>> Capability get is not a place where the feature is enabled.
>>> Dynfield should be registered only in case the feature is enabled.
>>> I will add following line in conf_set() doxygen comment.
>>>
>>> The PMD should call 'rte_eth_ip_reass_dynfield_register()' when
>>> the feature is enabled and return error if dynfield is not registered.
>>> Dynfield is needed to give packets back to the application in case the
>>> reassembly is not complete.
>>>
>>
>> Can you also clarify what PMD should do related to the ip reassembly feature
>> when registering dynfield fails? Should it keep the feature enabled or disabled?
>>
>> This will also clarify for the application, if application detects that
>> 'RTE_MBUF_DYNFIELD_IP_REASS_NAME' is not registered how it should
>> behave?
>> Ignore it? Fail? Disable ip reassembly?
> 
> The PMD can return error in the conf_set API, if dynfield is not successfully
> registered. Or in case of inline IPsec, the PMD can return the error while
> creating inline security session.
> 

I think better to handle in the conf_set, since there can be other users than
IPSec.

> Hence the application will get to know if the dynfield is successfully configured
> Or not.

Application already can know if dynfield is registered or not via
'rte_mbuf_dynfield_lookup()'.

> If the dynfield is not configured, PMD will return configuration error
> (either in conf_set or in security_session_create) and feature
> will not be enabled.
> 

ack.
What do you think to document this in the API documentation (doxygen comment)?
  
Akhil Goyal Feb. 7, 2022, 5:17 p.m. UTC | #6
> -----Original Message-----
> From: Ferruh Yigit <ferruh.yigit@intel.com>
> Sent: Monday, February 7, 2022 10:11 PM
> To: Akhil Goyal <gakhil@marvell.com>; dev@dpdk.org;
> olivier.matz@6wind.com
> Cc: Anoob Joseph <anoobj@marvell.com>; matan@nvidia.com;
> konstantin.ananyev@intel.com; thomas@monjalon.net;
> andrew.rybchenko@oktetlabs.ru; rosen.xu@intel.com;
> david.marchand@redhat.com; radu.nicolau@intel.com; Jerin Jacob
> Kollanukkaran <jerinj@marvell.com>; stephen@networkplumber.org;
> mdr@ashroe.eu
> Subject: Re: [EXT] Re: [PATCH v4 2/3] ethdev: add mbuf dynfield for
> incomplete IP reassembly
> 
> On 2/7/2022 4:20 PM, Akhil Goyal wrote:
> >> On 2/7/2022 2:20 PM, Akhil Goyal wrote:
> >>>> 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?
> >>>
> >>> Dynfield is added for the error/ incomplete reassembly case.
> >>> If the dynfield is not registered, the feature can work well for success
> >> scenarios.
> >>> Dynfield registration is responsibility of PMD and it is upto the driver to
> decide
> >>> when to set the dynfield. The registration can be done in conf_set() API.
> >>>
> >>>>
> >>>> Can you please describe this dependency, preferable in the
> >>>> 'rte_eth_ip_reassembly_capability_get()' doxygen comment?
> >>>
> >>> Capability get is not a place where the feature is enabled.
> >>> Dynfield should be registered only in case the feature is enabled.
> >>> I will add following line in conf_set() doxygen comment.
> >>>
> >>> The PMD should call 'rte_eth_ip_reass_dynfield_register()' when
> >>> the feature is enabled and return error if dynfield is not registered.
> >>> Dynfield is needed to give packets back to the application in case the
> >>> reassembly is not complete.
> >>>
> >>
> >> Can you also clarify what PMD should do related to the ip reassembly
> feature
> >> when registering dynfield fails? Should it keep the feature enabled or
> disabled?
> >>
> >> This will also clarify for the application, if application detects that
> >> 'RTE_MBUF_DYNFIELD_IP_REASS_NAME' is not registered how it should
> >> behave?
> >> Ignore it? Fail? Disable ip reassembly?
> >
> > The PMD can return error in the conf_set API, if dynfield is not successfully
> > registered. Or in case of inline IPsec, the PMD can return the error while
> > creating inline security session.
> >
> 
> I think better to handle in the conf_set, since there can be other users than
> IPSec.

I think it can be left to the PMD, as there can be cases when reassembly is supported
Only with IPsec flows and not with normal IP packets.
So reassembly may get enabled for IPsec flows only when security session is created.
In that case session creation will fail.

> 
> > Hence the application will get to know if the dynfield is successfully
> configured
> > Or not.
> 
> Application already can know if dynfield is registered or not via
> 'rte_mbuf_dynfield_lookup()'.
> 
> > If the dynfield is not configured, PMD will return configuration error
> > (either in conf_set or in security_session_create) and feature
> > will not be enabled.
> >
> 
> ack.
> What do you think to document this in the API documentation (doxygen
> comment)?
Ok I will try to clarify this in doxygen comments.
  
Stephen Hemminger Feb. 7, 2022, 5:23 p.m. UTC | #7
On Sat, 5 Feb 2022 03:43:33 +0530
Akhil Goyal <gakhil@marvell.com> wrote:

> +/**
> + * @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);

Maybe use RTE_INIT() constructor for this?
  
Ferruh Yigit Feb. 7, 2022, 5:28 p.m. UTC | #8
On 2/7/2022 5:23 PM, Stephen Hemminger wrote:
> On Sat, 5 Feb 2022 03:43:33 +0530
> Akhil Goyal <gakhil@marvell.com> wrote:
> 
>> +/**
>> + * @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);
> 
> Maybe use RTE_INIT() constructor for this?

Dynfiled should be registered only when users asks for the feature.
  
Akhil Goyal Feb. 7, 2022, 5:29 p.m. UTC | #9
> On Sat, 5 Feb 2022 03:43:33 +0530
> Akhil Goyal <gakhil@marvell.com> wrote:
> 
> > +/**
> > + * @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);
> 
> Maybe use RTE_INIT() constructor for this?

The same application can be used for non-reassembly case as well.
RTE_INIT would mean, dynfield would be registered always even if it is
enabled or not.
The current implementation would register it only when the 
Reassembly option is enabled.
  
Stephen Hemminger Feb. 7, 2022, 6:01 p.m. UTC | #10
On Mon, 7 Feb 2022 17:28:26 +0000
Ferruh Yigit <ferruh.yigit@intel.com> wrote:

> On 2/7/2022 5:23 PM, Stephen Hemminger wrote:
> > On Sat, 5 Feb 2022 03:43:33 +0530
> > Akhil Goyal <gakhil@marvell.com> wrote:
> >   
> >> +/**
> >> + * @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);  
> > 
> > Maybe use RTE_INIT() constructor for this?  
> 
> Dynfiled should be registered only when users asks for the feature.

right but making the user ask can lead to errors, can it be done implicitly
on first use.
  
Akhil Goyal Feb. 7, 2022, 6:28 p.m. UTC | #11
> On Mon, 7 Feb 2022 17:28:26 +0000
> Ferruh Yigit <ferruh.yigit@intel.com> wrote:
> 
> > On 2/7/2022 5:23 PM, Stephen Hemminger wrote:
> > > On Sat, 5 Feb 2022 03:43:33 +0530
> > > Akhil Goyal <gakhil@marvell.com> wrote:
> > >
> > >> +/**
> > >> + * @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);
> > >
> > > Maybe use RTE_INIT() constructor for this?
> >
> > Dynfiled should be registered only when users asks for the feature.
> 
> right but making the user ask can lead to errors, can it be done implicitly
> on first use.

Registering dynfield is responsibility of PMD when the application asks for the feature.
So how can it lead to errors.
  
Stephen Hemminger Feb. 7, 2022, 7:08 p.m. UTC | #12
On Mon, 7 Feb 2022 18:28:03 +0000
Akhil Goyal <gakhil@marvell.com> wrote:

> > On Mon, 7 Feb 2022 17:28:26 +0000
> > Ferruh Yigit <ferruh.yigit@intel.com> wrote:
> >   
> > > On 2/7/2022 5:23 PM, Stephen Hemminger wrote:  
> > > > On Sat, 5 Feb 2022 03:43:33 +0530
> > > > Akhil Goyal <gakhil@marvell.com> wrote:
> > > >  
> > > >> +/**
> > > >> + * @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);  
> > > >
> > > > Maybe use RTE_INIT() constructor for this?  
> > >
> > > Dynfiled should be registered only when users asks for the feature.  
> > 
> > right but making the user ask can lead to errors, can it be done implicitly
> > on first use.  
> 
> Registering dynfield is responsibility of PMD when the application asks for the feature.
> So how can it lead to errors.

Sorry, forgot this is a PMD internal thing.
  

Patch

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;
+}
+
 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"
+
 #ifdef __cplusplus
 }
 #endif