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

[Akhil Goyal]

> -----Original Message-----
> From: Ferruh Yigit <ferruh.yigit at intel.com>
> Sent: Monday, February 7, 2022 10:11 PM
> To: Akhil Goyal <gakhil at marvell.com>; dev at dpdk.org;
> olivier.matz at 6wind.com
> Cc: Anoob Joseph <anoobj at marvell.com>; matan at nvidia.com;
> konstantin.ananyev at intel.com; thomas at monjalon.net;
> andrew.rybchenko at oktetlabs.ru; rosen.xu at intel.com;
> david.marchand at redhat.com; radu.nicolau at intel.com; Jerin Jacob
> Kollanukkaran <jerinj at marvell.com>; stephen at networkplumber.org;
> mdr at 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 at 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.