Re: [PATCH v2 1/4] ethdev: introduce encap hash calculation

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

On 2/12/2024 6:44 PM, Ori Kam wrote:
> Hi Ferruh
> 
>> -----Original Message-----
>> From: Ferruh Yigit <ferruh.yigit@amd.com>
>> Sent: Monday, February 12, 2024 7:05 PM
>>
>> On 2/11/2024 7:29 AM, Ori Kam wrote:
>>> Hi Ferruh,
>>>
>>>> -----Original Message-----
>>>> From: Ferruh Yigit <ferruh.yigit@amd.com>
>>>> Sent: Thursday, February 8, 2024 7:13 PM
>>>> To: Ori Kam <orika@nvidia.com>; Dariusz Sosnowski
>>>>
>>>> On 2/8/2024 9:09 AM, Ori Kam wrote:
>>>>> During encapsulation of a packet, it is possible to change some
>>>>> outer headers to improve flow destribution.
>>>>> For example, from VXLAN RFC:
>>>>> "It is recommended that the UDP source port number
>>>>> be calculated using a hash of fields from the inner packet --
>>>>> one example being a hash of the inner Ethernet frame's headers.
>>>>> This is to enable a level of entropy for the ECMP/load-balancing"
>>>>>
>>>>> The tunnel protocol defines which outer field should hold this hash,
>>>>> but it doesn't define the hash calculation algorithm.
>>>>>
>>>>> An application that uses flow offloads gets the first few packets
>>>>> (exception path) and then decides to offload the flow.
>>>>> As a result, there are two
>>>>> different paths that a packet from a given flow may take.
>>>>> SW for the first few packets or HW for the rest.
>>>>> When the packet goes through the SW, the SW encapsulates the packet
>>>>> and must use the same hash calculation as the HW will do for
>>>>> the rest of the packets in this flow.
>>>>>
>>>>> the new function rte_flow_calc_encap_hash can query the hash value
>>>>> fromm the driver for a given packet as if the packet was passed
>>>>> through the HW.
>>>>>
>>>>> Signed-off-by: Ori Kam <orika@nvidia.com>
>>>>> Acked-by: Dariusz Sosnowski <dsosnowski@nvidia.com>
>>>>>
>>>>
>>>> <...>
>>>>
>>>>> +int
>>>>> +rte_flow_calc_encap_hash(uint16_t port_id, const struct rte_flow_item
>>>> pattern[],
>>>>> +			 enum rte_flow_encap_hash_field dest_field, uint8_t
>>>> hash_len,
>>>>> +			 uint8_t *hash, struct rte_flow_error *error)
>>>>> +{
>>>>> +	int ret;
>>>>> +	struct rte_eth_dev *dev;
>>>>> +	const struct rte_flow_ops *ops;
>>>>> +
>>>>> +	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
>>>>> +	ops = rte_flow_ops_get(port_id, error);
>>>>> +	if (!ops || !ops->flow_calc_encap_hash)
>>>>> +		return rte_flow_error_set(error, ENOTSUP,
>>>>> +
>>>> RTE_FLOW_ERROR_TYPE_UNSPECIFIED, NULL,
>>>>> +					  "calc encap hash is not supported");
>>>>> +	if ((dest_field == RTE_FLOW_ENCAP_HASH_FIELD_SRC_PORT &&
>>>> hash_len != 2) ||
>>>>> +	    (dest_field == RTE_FLOW_ENCAP_HASH_FIELD_NVGRE_FLOW_ID
>>>> && hash_len != 1))
>>>>>
>>>>
>>>> If there is a fixed mapping with the dest_field and the size, instead of
>>>> putting this information into check code, what do you think to put it
>>>> into the data structure?
>>>>
>>>> I mean instead of using enum for dest_filed, it can be a struct that is
>>>> holding enum and its expected size, this clarifies what the expected
>>>> size for that field.
>>>>
>>>
>>> From my original email I think we only need the type, we don't need the
>> size.
>>> On the RFC thread there was an objection. So I added the size,
>>> If you think it is not needed lets remove it.
>>>
>>
>> I am not saying length is not needed, but
>> API gets 'dest_field' & 'hash_len', and according checks in the API for
>> each 'dest_field' there is an exact 'hash_len' requirement, this
>> requirement is something impacts user but this information is embedded
>> in the API, my suggestion is make it more visible to user.
>>
>> My initial suggestion was put this into an object, like:
>> ```
>> struct x {
>> 	enum rte_flow_encap_hash_field dest_field;
>> 	size_t expected size;
>> } y[] = {
>> 	{ RTE_FLOW_ENCAP_HASH_FIELD_SRC_PORT, 2 },
>> 	{ RTE_FLOW_ENCAP_HASH_FIELD_NVGRE_FLOW_ID, 1 }
>> };
>> ```
>>
>> But as you mentioned this is a limited set, perhaps it is sufficient to
>> document size requirement in the "enum rte_flow_encap_hash_field" API
>> doxygen comment.
> 
> Will add it to the doxygen.
> 
>>
>>
>>
>>>>> +		return rte_flow_error_set(error, EINVAL,
>>>>> +
>>>> RTE_FLOW_ERROR_TYPE_UNSPECIFIED, NULL,
>>>>> +					  "hash len doesn't match the
>>>> requested field len");
>>>>> +	dev = &rte_eth_devices[port_id];
>>>>> +	ret = ops->flow_calc_encap_hash(dev, pattern, dest_field, hash,
>>>> error);
>>>>>
>>>>
>>>> 'hash_len' is get by API, but it is not passed to dev_ops, does this
>>>> mean this information hardcoded in the driver as well, if so why
>>>> duplicate this information in driver instead off passing hash_len to driver?
>>>
>>> Not sure I understand, like I wrote above this is pure verification from my
>> point of view.
>>> The driver knows the size based on the dest.
>>>
>>
>> My intention was similar to above comment, like dest_field type
>> RTE_FLOW_ENCAP_HASH_FIELD_SRC_PORT implies that required size should
>> be
>> 2 bytes, and it seems driver already knows about this requirement.
> 
> That is correct, that is why I don't think we need the size, add added it
> only for validation due to community request.
> 
>>
>> Instead, it can be possible to verify 'hash_len' in the API level, pass
>> this information to the driver and driver use 'hash_len' directly for
>> its size parameter, so driver will rely on API provided 'hash_len' value
>> instead of storing this information within driver.
>>
>> Lets assume 10 drivers are implementing this feature, should all of them
>> define MLX5DR_CRC_ENCAP_ENTROPY_HASH_SIZE_16 equivalent
>> enum/define
>> withing the driver?
> 
> No, the driver implements hard-coded logic, which means that it just needs to know
> the dest field, in order to know what hash to calculate
> It is possible that for each field the HW will calculate the hash using different algorithm.
> 

OK if HW already needs to know the size in advance, lets go with enum
doxygen update only.

> Also it is possible that the HW doesn't support writing to the expected field, in which case we 
> want the driver call to fail.
> 
> Field implies size.
> Size doesn't implies field.
> 
>>
>>>>
>>>>
>>>>> +	return flow_err(port_id, ret, error);
>>>>> +}
>>>>> diff --git a/lib/ethdev/rte_flow.h b/lib/ethdev/rte_flow.h
>>>>> index 1267c146e5..2bdf3a4a17 100644
>>>>> --- a/lib/ethdev/rte_flow.h
>>>>> +++ b/lib/ethdev/rte_flow.h
>>>>> @@ -6783,6 +6783,57 @@ rte_flow_calc_table_hash(uint16_t port_id,
>>>> const struct rte_flow_template_table
>>>>>  			 const struct rte_flow_item pattern[], uint8_t
>>>> pattern_template_index,
>>>>>  			 uint32_t *hash, struct rte_flow_error *error);
>>>>>
>>>>> +/**
>>>>> + * @warning
>>>>> + * @b EXPERIMENTAL: this API may change without prior notice.
>>>>> + *
>>>>> + * Destination field type for the hash calculation, when encap action is
>>>> used.
>>>>> + *
>>>>> + * @see function rte_flow_calc_encap_hash
>>>>> + */
>>>>> +enum rte_flow_encap_hash_field {
>>>>> +	/* Calculate hash placed in UDP source port field. */
>>>>>
>>
>> Just recognized that comments are not doxygen comments.
> 
> Thanks,
> Will fix.
>>
>>>>> +	RTE_FLOW_ENCAP_HASH_FIELD_SRC_PORT,
>>>>> +	/* Calculate hash placed in NVGRE flow ID field. */
>>>>> +	RTE_FLOW_ENCAP_HASH_FIELD_NVGRE_FLOW_ID,
>>>>> +};
>>>>>
>>>>
>>>> Indeed above enum represents a field in a network protocol, right?
>>>> Instead of having a 'RTE_FLOW_ENCAP_HASH_' specific one, can re-using
>>>> 'enum rte_flow_field_id' work?
>>>
>>> Since the option are really limited and defined by standard, I prefer to have
>> dedicated options.
>>>
>>
>> OK, my intention is to reduce the duplication. Just for brainstorm, what
>> is the benefit of having 'RTE_FLOW_ENCAP_HASH_' specific enums, if we
>> can present them as generic protocol fiels, like
>> 'RTE_FLOW_ENCAP_HASH_FIELD_SRC_PORT' vs
>> 'RTE_FLOW_FIELD_UDP_PORT_SRC,'?
> 
> I guess you want to go with 'RTE_FLOW_FIELD_UDP_PORT_SRC
> right?
>

I just want to discuss if redundancy can be eliminated.

> The main issue is since the options are really limited and used for a very dedicated function.
> When app developers / DPDK developers will look at it, it will be very unclear what is the use of this enum.
> We already have an enum for fields. Like you suggested we could have used it,
> but this will show much more option than there are really.
> 

OK, lets use dedicated enums to clarify to the users the specific fields
available for this set of APIs.

Btw, is boundary check like following required for the APIs:
```
if (dest_field > RTE_FLOW_ENCAP_HASH_FIELD_NVGRE_FLOW_ID)
	return -EINVAL;
```
In case user pass an invalid value as 'dest_filed'

(Note: I intentionally not used MAX enum something like
'RTE_FLOW_ENCAP_HASH_FIELD_MAX' to not need to deal with ABI issues in
the future.)