[API-NEXT,v5] api: packet: allow access to packet flow hash values

Applications can read the computed hash (if any) and set it if they
changed the packet headers or if the platform haven't calculated the hash.

Signed-off-by: Zoltan Kiss <zoltan.kiss@linaro.org>
---

v2:
- focus on RSS hash only
- use setter/getter's

v3:
- do not mention pointers
- add a note
- add new patches for implementation and test

v4:
- use separate flag get and clear, as hash can have any value (that maps to
checking ol_flags in DPDK)
- change terminology to "flow hash", it reflects better what is actually hashed
- add function to generate hash by the platform

 include/odp/api/packet.h       | 72 ++++++++++++++++++++++++++++++++++++++++++
 include/odp/api/packet_flags.h | 18 +++++++++++
 2 files changed, 90 insertions(+)

In ODP-OVS the changes should look like this:

dp_packet_get_rss_hash:
if (p->source == DPBUF_ODP)
     if (odp_packet_has_flow_hash(p->odp_pkt))
         return odp_packet_flow_hash(...);
     else
         /* Platform haven't supplied it, let OVS do what it wants. 
Note, OVS doesn't handle 0 as a hash, that bug should be fixed separately */
         return 0;

netdev_send:
/* recirc_depth tells us OVS modified the hash, even if the platform 
supplied one. odp_packet_hash_gen() let platforms opt out */
if (recirc_depth && odp_packet_flow_hash_gen(..., &hash, 1)
     odp_packet_flow_hash_set(..., hash)



On 21/08/15 19:07, Zoltan Kiss wrote:
> Applications can read the computed hash (if any) and set it if they
> changed the packet headers or if the platform haven't calculated the hash.
>
> Signed-off-by: Zoltan Kiss <zoltan.kiss@linaro.org>
> ---
>
> v2:
> - focus on RSS hash only
> - use setter/getter's
>
> v3:
> - do not mention pointers
> - add a note
> - add new patches for implementation and test
>
> v4:
> - use separate flag get and clear, as hash can have any value (that maps to
> checking ol_flags in DPDK)
> - change terminology to "flow hash", it reflects better what is actually hashed
> - add function to generate hash by the platform
>
>   include/odp/api/packet.h       | 72 ++++++++++++++++++++++++++++++++++++++++++
>   include/odp/api/packet_flags.h | 18 +++++++++++
>   2 files changed, 90 insertions(+)
>
> diff --git a/include/odp/api/packet.h b/include/odp/api/packet.h
> index 3a454b5..9a74ca4 100644
> --- a/include/odp/api/packet.h
> +++ b/include/odp/api/packet.h
> @@ -605,6 +605,78 @@ uint32_t odp_packet_l4_offset(odp_packet_t pkt);
>   int odp_packet_l4_offset_set(odp_packet_t pkt, uint32_t offset);
>
>   /**
> + * Packet flow hash value
> + *
> + * Returns the hash generated from the packet header. Use
> + * odp_packet_has_flow_hash() to check if packet contains a hash.
> + *
> + * @param      pkt      Packet handle
> + *
> + * @return  Hash value
> + *
> + * @note Zero can be a valid hash value.
> + * @note The hash algorithm and the header fields defining the flow (therefore
> + * used for hashing) is platform dependent.
> + */
> +uint32_t odp_packet_flow_hash(odp_packet_t pkt);
> +
> +/**
> + * Set packet flow hash value
> + *
> + * Store the packet flow hash for the packet and sets the flow hash flag. This
> + * enables application to recalculate hash for a modified packet.
> + *
> + * @param      pkt              Packet handle
> + * @param      flow_hash        Hash value to set
> + *
> + * @note The platform might use this hash on outgoing packets, so application
> + * should call odp_packet_flow_hash_set() with the lazy option at least before
> + * handing the packet back to the platform.
> + */
> +void odp_packet_flow_hash_set(odp_packet_t pkt, uint32_t flow_hash);
> +
> +/**
> + * Generate packet flow hash value
> + *
> + * Generates hash value from the packet headers. Hash generation is identical to
> + * packet input processing. The packet would have identical hash value
> + * when received through a packet input interface. If 'lazy' option specified,
> + * and the platform doesn't care about the value of this hash after it handed
> + * over the packet to the application, it can opt out generating one.
> + *
> + * @param      pkt      Packet handle
> + * @param      hash     Pointer for storing the hash
> + * @param      lazy     If 1, the platform can opt out
> + *
> + * @retval  0 Hash was generated
> + * @retval  <0 Hash was not generated
> + * @note If the platform wouldn't compute any hash during receive, then it
> + * won't generate any here either
> + * @note When the platform can opt out generating a hash (lazy == 1 and platform
> + * doesn't care about the hash), it doesn't mean it has to. If it can do it
> + * really fast, implementors can decide to generate it anyway.
> + */
> +int odp_packet_flow_hash_gen(odp_packet_t pkt, uint32_t *hash, int lazy);
> +
> +/**
> + * Lazy generate packet flow hash value
> + *
> + * Generates hash value from the packet headers, same as
> + * odp_packet_flow_hash_gen(), but with relaxed requirements. If the platform
> + * doesn't care about the value of this hash after it handed over the packet to
> + * the application, it can opt out generating one.
> + *
> + * @param      pkt      Packet handle
> + * @param      hash     Pointer for storing the hash
> + *
> + * @retval  0 Hash was generated
> + * @retval  <0 Hash was not generated
> + * @note If the platform wouldn't compute any hash during receive, then it
> + * won't generate any here either
> + */
> +int odp_packet_flow_hash_gen_lazy(odp_packet_t pkt, uint32_t *hash);
> +
> +/**
>    * Tests if packet is segmented
>    *
>    * @param pkt  Packet handle
> diff --git a/include/odp/api/packet_flags.h b/include/odp/api/packet_flags.h
> index bfbcc94..7c3b247 100644
> --- a/include/odp/api/packet_flags.h
> +++ b/include/odp/api/packet_flags.h
> @@ -191,6 +191,15 @@ int odp_packet_has_sctp(odp_packet_t pkt);
>   int odp_packet_has_icmp(odp_packet_t pkt);
>
>   /**
> + * Check for packet flow hash
> + *
> + * @param pkt Packet handle
> + * @retval non-zero if packet contains a hash value
> + * @retval 0 if packet does not contain a hash value
> + */
> +int odp_packet_has_flow_hash(odp_packet_t pkt);
> +
> +/**
>    * Set flag for L2 header, e.g. ethernet
>    *
>    * @param pkt Packet handle
> @@ -327,6 +336,15 @@ void odp_packet_has_sctp_set(odp_packet_t pkt, int val);
>   void odp_packet_has_icmp_set(odp_packet_t pkt, int val);
>
>   /**
> + * Clear flag for packet flow hash
> + *
> + * @param pkt Packet handle
> + *
> + * @note Set this flag is only possible through odp_packet_flow_hash_set()
> + */
> +void odp_packet_has_flow_hash_clr(odp_packet_t pkt);
> +
> +/**
>    * @}
>    */
>
>

You might want to take a look at https://wiki.freebsd.org/NetworkRSS for
some background info on RSS and the functions used with it in FreeBSD.
They define a more comprehensive set of APIs for these functions.  Not sure
if that's overkill for our purposes, but worth looking at.

On Fri, Aug 21, 2015 at 1:07 PM, Zoltan Kiss <zoltan.kiss@linaro.org> wrote:

> Applications can read the computed hash (if any) and set it if they
> changed the packet headers or if the platform haven't calculated the hash.
>
> Signed-off-by: Zoltan Kiss <zoltan.kiss@linaro.org>
> ---
>
> v2:
> - focus on RSS hash only
> - use setter/getter's
>
> v3:
> - do not mention pointers
> - add a note
> - add new patches for implementation and test
>
> v4:
> - use separate flag get and clear, as hash can have any value (that maps to
> checking ol_flags in DPDK)
> - change terminology to "flow hash", it reflects better what is actually
> hashed
> - add function to generate hash by the platform
>
>  include/odp/api/packet.h       | 72
> ++++++++++++++++++++++++++++++++++++++++++
>  include/odp/api/packet_flags.h | 18 +++++++++++
>  2 files changed, 90 insertions(+)
>
> diff --git a/include/odp/api/packet.h b/include/odp/api/packet.h
> index 3a454b5..9a74ca4 100644
> --- a/include/odp/api/packet.h
> +++ b/include/odp/api/packet.h
> @@ -605,6 +605,78 @@ uint32_t odp_packet_l4_offset(odp_packet_t pkt);
>  int odp_packet_l4_offset_set(odp_packet_t pkt, uint32_t offset);
>
>  /**
> + * Packet flow hash value
> + *
> + * Returns the hash generated from the packet header. Use
> + * odp_packet_has_flow_hash() to check if packet contains a hash.
> + *
> + * @param      pkt      Packet handle
> + *
> + * @return  Hash value
> + *
> + * @note Zero can be a valid hash value.
> + * @note The hash algorithm and the header fields defining the flow
> (therefore
> + * used for hashing) is platform dependent.
> + */
> +uint32_t odp_packet_flow_hash(odp_packet_t pkt);
> +
> +/**
> + * Set packet flow hash value
> + *
> + * Store the packet flow hash for the packet and sets the flow hash flag.
> This
> + * enables application to recalculate hash for a modified packet.
> + *
> + * @param      pkt              Packet handle
> + * @param      flow_hash        Hash value to set
> + *
> + * @note The platform might use this hash on outgoing packets, so
> application
> + * should call odp_packet_flow_hash_set() with the lazy option at least
> before
> + * handing the packet back to the platform.
> + */
> +void odp_packet_flow_hash_set(odp_packet_t pkt, uint32_t flow_hash);
> +
> +/**
> + * Generate packet flow hash value
> + *
> + * Generates hash value from the packet headers. Hash generation is
> identical to
> + * packet input processing. The packet would have identical hash value
> + * when received through a packet input interface. If 'lazy' option
> specified,
> + * and the platform doesn't care about the value of this hash after it
> handed
> + * over the packet to the application, it can opt out generating one.
> + *
> + * @param      pkt      Packet handle
> + * @param      hash     Pointer for storing the hash
> + * @param      lazy     If 1, the platform can opt out
> + *
> + * @retval  0 Hash was generated
> + * @retval  <0 Hash was not generated
> + * @note If the platform wouldn't compute any hash during receive, then it
> + * won't generate any here either
> + * @note When the platform can opt out generating a hash (lazy == 1 and
> platform
> + * doesn't care about the hash), it doesn't mean it has to. If it can do
> it
> + * really fast, implementors can decide to generate it anyway.
> + */
> +int odp_packet_flow_hash_gen(odp_packet_t pkt, uint32_t *hash, int lazy);
>

Lazy evaluation is an implementation choice and shouldn't be part of the
API.  If an application really doesn't need a hash, why is it asking for
one?


> +
> +/**
> + * Lazy generate packet flow hash value
> + *
> + * Generates hash value from the packet headers, same as
> + * odp_packet_flow_hash_gen(), but with relaxed requirements. If the
> platform
> + * doesn't care about the value of this hash after it handed over the
> packet to
> + * the application, it can opt out generating one.
> + *
> + * @param      pkt      Packet handle
> + * @param      hash     Pointer for storing the hash
> + *
> + * @retval  0 Hash was generated
> + * @retval  <0 Hash was not generated
> + * @note If the platform wouldn't compute any hash during receive, then it
> + * won't generate any here either
> + */
> +int odp_packet_flow_hash_gen_lazy(odp_packet_t pkt, uint32_t *hash);
>

It's not clear why this API is needed, especially the way you defined the
odp_packet_flow_gen() API.  Lazy evaluation is an implementation feature,
not an API feature.


> +
> +/**
>   * Tests if packet is segmented
>   *
>   * @param pkt  Packet handle
> diff --git a/include/odp/api/packet_flags.h
> b/include/odp/api/packet_flags.h
> index bfbcc94..7c3b247 100644
> --- a/include/odp/api/packet_flags.h
> +++ b/include/odp/api/packet_flags.h
> @@ -191,6 +191,15 @@ int odp_packet_has_sctp(odp_packet_t pkt);
>  int odp_packet_has_icmp(odp_packet_t pkt);
>
>  /**
> + * Check for packet flow hash
> + *
> + * @param pkt Packet handle
> + * @retval non-zero if packet contains a hash value
> + * @retval 0 if packet does not contain a hash value
> + */
> +int odp_packet_has_flow_hash(odp_packet_t pkt);
> +
> +/**
>   * Set flag for L2 header, e.g. ethernet
>   *
>   * @param pkt Packet handle
> @@ -327,6 +336,15 @@ void odp_packet_has_sctp_set(odp_packet_t pkt, int
> val);
>  void odp_packet_has_icmp_set(odp_packet_t pkt, int val);
>
>  /**
> + * Clear flag for packet flow hash
> + *
> + * @param pkt Packet handle
> + *
> + * @note Set this flag is only possible through odp_packet_flow_hash_set()
> + */
> +void odp_packet_has_flow_hash_clr(odp_packet_t pkt);
>

This is inconsistent with the other packet attribute setter functions.  If
you need this function then it should be defined as:
int odp_packet_has_flow_hash_set(odp_packet_t pkt, int val);
Where val == 0 or 1.


> +
> +/**
>   * @}
>   */
>
> --
> 1.9.1
>
> _______________________________________________
> lng-odp mailing list
> lng-odp@lists.linaro.org
> https://lists.linaro.org/mailman/listinfo/lng-odp
>

On 24/08/15 09:02, Savolainen, Petri (Nokia - FI/Espoo) wrote:
>
>
>> -----Original Message-----
>> From: lng-odp [mailto:lng-odp-bounces@lists.linaro.org] On Behalf Of
>> ext Zoltan Kiss
>> Sent: Friday, August 21, 2015 9:07 PM
>> To: lng-odp@lists.linaro.org
>> Subject: [lng-odp] [API-NEXT PATCH v5] api: packet: allow access to
>> packet flow hash values
>>
>> Applications can read the computed hash (if any) and set it if they
>> changed the packet headers or if the platform haven't calculated the
>> hash.
>>
>> Signed-off-by: Zoltan Kiss <zoltan.kiss@linaro.org>
>> ---
>>
>> v2:
>> - focus on RSS hash only
>> - use setter/getter's
>>
>> v3:
>> - do not mention pointers
>> - add a note
>> - add new patches for implementation and test
>>
>> v4:
>
> These are comments for v5?

Well, I just realized this should be v4 ... The misnumbering happened 
because I looked to the "user area init" series, which is one version 
ahead. The In-reply-to header of this mail is also wrong, it points to 
that series' v4. Sorry for the confusion, I should automate and improve 
my patch series handling.


>
>> - use separate flag get and clear, as hash can have any value (that
>> maps to
>> checking ol_flags in DPDK)
>> - change terminology to "flow hash", it reflects better what is
>> actually hashed
>> - add function to generate hash by the platform
>>
>>   include/odp/api/packet.h       | 72
>> ++++++++++++++++++++++++++++++++++++++++++
>>   include/odp/api/packet_flags.h | 18 +++++++++++
>>   2 files changed, 90 insertions(+)
>>
>> diff --git a/include/odp/api/packet.h b/include/odp/api/packet.h
>> index 3a454b5..9a74ca4 100644
>> --- a/include/odp/api/packet.h
>> +++ b/include/odp/api/packet.h
>> @@ -605,6 +605,78 @@ uint32_t odp_packet_l4_offset(odp_packet_t pkt);
>>   int odp_packet_l4_offset_set(odp_packet_t pkt, uint32_t offset);
>>
>>   /**
>> + * Packet flow hash value
>> + *
>> + * Returns the hash generated from the packet header. Use
>> + * odp_packet_has_flow_hash() to check if packet contains a hash.
>> + *
>> + * @param      pkt      Packet handle
>> + *
>> + * @return  Hash value
>> + *
>> + * @note Zero can be a valid hash value.
>> + * @note The hash algorithm and the header fields defining the flow
>> (therefore
>> + * used for hashing) is platform dependent.
>> + */
>> +uint32_t odp_packet_flow_hash(odp_packet_t pkt);
>> +
>> +/**
>> + * Set packet flow hash value
>> + *
>> + * Store the packet flow hash for the packet and sets the flow hash
>> flag. This
>> + * enables application to recalculate hash for a modified packet.
>> + *
>> + * @param      pkt              Packet handle
>> + * @param      flow_hash        Hash value to set
>> + *
>> + * @note The platform might use this hash on outgoing packets, so
>> application
>> + * should call odp_packet_flow_hash_set()
This should end "_gen"

  with the lazy option at
>> least before
>> + * handing the packet back to the platform.
>
> I think we could leave out this note and spec hash usage only for application usage.
I'm afraid we can't. AFAIK Cavium needs the hash to be correct on 
outgoing packets, so we should warn app developers that they should call 
odp_packet_flow_hash_gen() if they modified the hash AND the packet goes out

> We'd need more specific spec for the cases (ODP API calls)  which expects a valid hash value.
odp_pktio_send() for sure, I'm not sure about the queueing calls.

> Now it can be thought only as extra metadata for application usage.
>
>
>> + */
>> +void odp_packet_flow_hash_set(odp_packet_t pkt, uint32_t flow_hash);
>> +
>> +/**
>> + * Generate packet flow hash value
>> + *
>> + * Generates hash value from the packet headers. Hash generation is
>> identical to
>> + * packet input processing. The packet would have identical hash value
>> + * when received through a packet input interface. If 'lazy' option
>> specified,
>> + * and the platform doesn't care about the value of this hash after it
>> handed
>> + * over the packet to the application, it can opt out generating one.
>> + *
>> + * @param      pkt      Packet handle
>> + * @param      hash     Pointer for storing the hash
>> + * @param      lazy     If 1, the platform can opt out
>> + *
>> + * @retval  0 Hash was generated
>> + * @retval  <0 Hash was not generated
>> + * @note If the platform wouldn't compute any hash during receive,
>> then it
>> + * won't generate any here either
>> + * @note When the platform can opt out generating a hash (lazy == 1
>> and platform
>> + * doesn't care about the hash), it doesn't mean it has to. If it can
>> do it
>> + * really fast, implementors can decide to generate it anyway.
>> + */
>> +int odp_packet_flow_hash_gen(odp_packet_t pkt, uint32_t *hash, int
>> lazy);
>
>
> We have couple of use cases
> 1) application has to generate the exact input flow hash, no matter how long it takes
> 2) application needs to generate some flow hash as fast as possible (in low number of CPU cycles)
> 3) application wants to generate the exact input flow hash, but only if it's fast
>
> I think 3) can be turned into 2), since application have to be prepared to use another hash (than the input flow hash) anyway. This is why I suggested "mode" parameter like this:
>
>
> /**
>    * Generate packet flow hash value
>    *
>    * Generates flow hash value from the packet. In mode 0, hash
>    * generation is identical to packet input processing. The same
>    * packet would have identical hash value when received through
>    * a packet input interface. In mode 1, the fastest possible
>    * flow hash generation method is used.
>    *
>    * @param      pkt      Packet handle
>    * @param      hash     Pointer for storing the hash
>    * @param      mode     0: Generate packet input flow hash
>    *                      1: Generate fast flow hash
>    *
>    * @retval  0 Hash was generated
>    * @retval  <0 Hash was not generated
>    */
> int odp_packet_flow_hash_gen(odp_packet_t pkt, uint32_t *hash, int mode);
>
>
> In these two modes, the hash should be normally always generated (return 0),

I think it wouldn't be good for OVS: it only needs the hash if the 
platform needs it on the outgoing leg. So on DPDK mode 1 should return 
no hash, because any time spent on computing a hash noone needs is 
wasted. I think that doesn't fit very well with the "fast hash" 
definition, as it implies there should be something. At least that's how 
I would interpret it.

 > but packet errors (no L2/L3 pointer, no valid IP header, some other 
metadata missing, etc) can prevent hash generation.
>
> This defines two (potentially) different hashes: the packet input hash and the "fast" hash. I think it's not a problmen, since the flow specification (which we define later through some e.g. pktio APIs) can be the same for both hashes. So, potential difference of those two modes  would be hash quality and CPU intensively. Additional modes could be added later on if needed (e.g. update current hash for ODP API X usage).
>
>
>> +
>> +/**
>> + * Lazy generate packet flow hash value
>> + *
>> + * Generates hash value from the packet headers, same as
>> + * odp_packet_flow_hash_gen(), but with relaxed requirements. If the
>> platform
>> + * doesn't care about the value of this hash after it handed over the
>> packet to
>> + * the application, it can opt out generating one.
>> + *
>> + * @param      pkt      Packet handle
>> + * @param      hash     Pointer for storing the hash
>> + *
>> + * @retval  0 Hash was generated
>> + * @retval  <0 Hash was not generated
>> + * @note If the platform wouldn't compute any hash during receive,
>> then it
>> + * won't generate any here either
>> + */
>> +int odp_packet_flow_hash_gen_lazy(odp_packet_t pkt, uint32_t *hash);
>> +
>
> I guess this is covered by the previous function (this is unintentionally here)

Yes, originally I've specified two different functions, then I forgot to 
delete this one when I rushed to send the patch before leaving office.
>
>
> -Petri
>
>
>> +/**
>>    * Tests if packet is segmented
>>    *
>>    * @param pkt  Packet handle
>> diff --git a/include/odp/api/packet_flags.h
>> b/include/odp/api/packet_flags.h
>> index bfbcc94..7c3b247 100644
>> --- a/include/odp/api/packet_flags.h
>> +++ b/include/odp/api/packet_flags.h
>> @@ -191,6 +191,15 @@ int odp_packet_has_sctp(odp_packet_t pkt);
>>   int odp_packet_has_icmp(odp_packet_t pkt);
>>
>>   /**
>> + * Check for packet flow hash
>> + *
>> + * @param pkt Packet handle
>> + * @retval non-zero if packet contains a hash value
>> + * @retval 0 if packet does not contain a hash value
>> + */
>> +int odp_packet_has_flow_hash(odp_packet_t pkt);
>> +
>> +/**
>>    * Set flag for L2 header, e.g. ethernet
>>    *
>>    * @param pkt Packet handle
>> @@ -327,6 +336,15 @@ void odp_packet_has_sctp_set(odp_packet_t pkt, int
>> val);
>>   void odp_packet_has_icmp_set(odp_packet_t pkt, int val);
>>
>>   /**
>> + * Clear flag for packet flow hash
>> + *
>> + * @param pkt Packet handle
>> + *
>> + * @note Set this flag is only possible through
>> odp_packet_flow_hash_set()
>> + */
>> +void odp_packet_has_flow_hash_clr(odp_packet_t pkt);
>> +
>> +/**
>>    * @}
>>    */
>>
>> --
>> 1.9.1
>>
>> _______________________________________________
>> lng-odp mailing list
>> lng-odp@lists.linaro.org
>> https://lists.linaro.org/mailman/listinfo/lng-odp