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

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

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)?