[EXT] Re: [PATCH v3 1/4] ethdev: introduce IP reassembly offload

[Ferruh Yigit]

On 2/2/2022 10:57 AM, Akhil Goyal wrote:
>>> +/* 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.
>>> + *
>>> + * If RTE_ETH_RX_OFFLOAD_IP_REASSEMBLY flag is set in offloads field,
>>> + * the PMD will attempt IP reassembly for the received packets as per
>>> + * properties defined in this structure.
>>> + */
>>> +struct rte_eth_ip_reass_params {
>>
>> As a generic comment, what do you think to use full 'reassembly' instead
>> of 'reass' short version, to clarify/simplify the meaning?
> 
> Full reassembly was used in most places. But here the struct name would be too big.
> IMO, reass is good enough here. Though, no strong opinion. Will change if you insist.
> 

Only it doesn't remind me 'reassembly' when I see 'reass', so I think not clear,
we can wait for more comments from others.

>>
>>> +	/** Maximum time in ms which PMD can wait for other fragments. */
>>> +	uint32_t reass_timeout_ms;
>>> +	/** 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
>>> @@ -1841,8 +1865,10 @@ struct rte_eth_dev_info {
>>>    	 * embedded managed interconnect/switch.
>>>    	 */
>>>    	struct rte_eth_switch_info switch_info;
>>> +	/** IP reassembly offload capabilities that a device can support. */
>>> +	struct rte_eth_ip_reass_params reass_capa;
>>>
>>
>> "struct rte_eth_dev_info" & 'rte_eth_dev_info_get()' are very common,
>> all applications that use net devices and even some internal APIs rely on
>> this struct and API.
>> It makes me uneasy to extend this struct with rarely used features,
>> worrying on loading to much (capability/status/config) on single
>> API/struct can cause an unmaintainable code by time.
>>
>> Also most of the time (if not always) offload flag is just an on/off flag
>> to the PMD, application set/unset offload flag and PMD knows what to do.
>> But for this case some capability variables, and a configuration API is
>> required/involved.
>>
>> For considering above two cases, what do you think implement this as
>> control plane APIs instead of offload flag?
>> There are already 'conf_set()' and 'conf_get()' APIs introduced in coming
>> patches, introducing an additional 'capability_get()' API removes the need
>> of change in "struct rte_eth_dev_info" and
>> 'RTE_ETH_RX_OFFLOAD_IP_REASSEMBLY'
>> can be removed.
>> Thomas, Andrew, what do you think?
> 
> We are ok to add a new dev_op for capability_get() if we agree on that.
> Thomas, Andrew, let me know if you think otherwise.
> 
>>
>>
>>> -	uint64_t reserved_64s[2]; /**< Reserved for future fields */
>>> +	uint64_t reserved_64s[1]; /**< Reserved for future fields */
>>>    	void *reserved_ptrs[2];   /**< Reserved for future fields */
>>>    };
>>>
>