Hi Zoltan, Few comments inline...
On 24 August 2015 at 22:18, Zoltan Kiss <zoltan.k...@linaro.org> wrote: > > Applications can read the computed hash (if any) and set it if they want > to store any extra information in it. > > Signed-off-by: Zoltan Kiss <zoltan.k...@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: I've accidentally skipped this version > > v5: > - 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 > > v6: > - remove stale function definition from the end of packet.h > - spell out in hash_set that if platform cares about the validity of this > value, > it has to maintain it internally. > - with the above change OVS doesn't need the hash generator function anymore, > so remove that too. We can introduce it later on. > > include/odp/api/packet.h | 33 +++++++++++++++++++++++++++++++++ > include/odp/api/packet_flags.h | 18 ++++++++++++++++++ > 2 files changed, 51 insertions(+) > > diff --git a/include/odp/api/packet.h b/include/odp/api/packet.h > index 3a454b5..c983332 100644 > --- a/include/odp/api/packet.h > +++ b/include/odp/api/packet.h > @@ -605,6 +605,39 @@ 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 (but does not requires!) application to reflect packet header > + * changes in the hash. > + * > + * @param pkt Packet handle > + * @param flow_hash Hash value to set > + * > + * @note If the platform needs to keep the original hash value, it has to > + * maintain it internally. > + * @note The application is not required to keep this hash valid for new or > + * modified packets. > + */ I would like to add the information that this hash being set is stored only as a meta data in the packet and this hash setting will not affect the way implementation handles the flow or that this hash value does not dictate any implementation logic. Also add the information that by default the implementation returns the original hash which was generated by the implementation lets say in the case where the odp_packet_flow_hash_set() was not called by the application. > +void odp_packet_flow_hash_set(odp_packet_t pkt, uint32_t flow_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); Not sure why we need this function odp_packet_has_flow_hash() since you have defined that zero is also a valid hash the application can ignore the value if it is zero. Moreover in most platforms the hash will always be generated for the incoming packets unless some special cases like error packets, application generated packets, etc. coz this expects the implementation to maintain a flag indicating the validity of the flow hash and every packet by default will have a hash generated by the implementation which I believe should be returned even if odp_packet_flow_hash_set() was not called by the application. > + > +/** > * 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); Same logic here. Not sure if we need to maintain the flag for hash. > + > +/** > * @} > */ > > -- > 1.9.1 > > _______________________________________________ > lng-odp mailing list > lng-odp@lists.linaro.org > https://lists.linaro.org/mailman/listinfo/lng-odp Regards, Bala _______________________________________________ lng-odp mailing list lng-odp@lists.linaro.org https://lists.linaro.org/mailman/listinfo/lng-odp
