Hi Ferruh
> -----Original Message-----
> From: Ferruh Yigit <ferruh.yi...@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.yi...@amd.com>
> >> Sent: Thursday, February 8, 2024 7:13 PM
> >> To: Ori Kam <or...@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 <or...@nvidia.com>
> >>> Acked-by: Dariusz Sosnowski <dsosnow...@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.
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?
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.
Best,
Ori
