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

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

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.



>>> +		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.

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?

>>
>>
>>> +	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.

>>> +	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,'?