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

[Ori Kam <orika@nvidia.com>]

> -----Original Message-----
> From: Ferruh Yigit <ferruh.yigit@amd.com>
> Sent: Monday, February 12, 2024 10:10 PM
> 
> 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.)
Good idea will add
Best,
Ori