| Message ID | 1440434918-20477-1-git-send-email-zoltan.kiss@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
Hi Zoltan, Few comments inline... On 24 August 2015 at 22:18, Zoltan Kiss <zoltan.kiss@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.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: 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
Hi, On 25 August 2015 at 16:09, Savolainen, Petri (Nokia - FI/Espoo) <petri.savolainen@nokia.com> wrote: > > >> -----Original Message----- >> From: lng-odp [mailto:lng-odp-bounces@lists.linaro.org] On Behalf Of >> ext Bala Manoharan >> Sent: Tuesday, August 25, 2015 1:14 PM >> To: Zoltan Kiss >> Cc: LNG ODP Mailman List >> Subject: Re: [lng-odp] [API-NEXT PATCH v6] api: packet: allow access to >> packet flow hash values >> >> Hi Zoltan, >> >> Few comments inline... >> >> On 24 August 2015 at 22:18, Zoltan Kiss <zoltan.kiss@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.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: 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. > > A valid hash can be anything, also zero. Application cannot tell from hash value if the hash was actually generated and is valid. As you mention sometimes hash cannot be generated and it varies between platforms when that's the case. In general, application needs to check if hash is there before using it. > > Implementation can utilize its knowledge about the algorithm, packet input processing pipeline and packet metadata. For example, if packet is coming from packet input (application hash not stored/clr hash) and it's an IP packet "has hash" is true, but for non-IP packets it's false... The concern with adding additional has_hash_flag was that there will an "if check" in the system for every read hash value and since this comes in the fast path it might get costly if application reads it in several places. Anyhow that can be optimized by the implementation. Let me summarize my understanding, 1. Incoming packet the implementation has generated a hash value HASH1 2. Implementation sets the has_hash flag to true. 3. Application calls set_hash() function and sets a hash value HASH2 4. when application reads the packet_hash_value() implementation returns the value HASH2 5. If application calls clear_hash_flag() then has_hash_flag() function will return false and implementation can return any value if packet_hash_value() function if called by application by mistake. Basically this means that once hash value is set by the application it replaces any system generated hash value. Regards, Bala > > -Petri > >> >> > + >> > +/** >> > * 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
On 25/08/15 12:24, Bala Manoharan wrote: > Hi, > > On 25 August 2015 at 16:09, Savolainen, Petri (Nokia - FI/Espoo) > <petri.savolainen@nokia.com> wrote: >> >> >>> -----Original Message----- >>> From: lng-odp [mailto:lng-odp-bounces@lists.linaro.org] On Behalf Of >>> ext Bala Manoharan >>> Sent: Tuesday, August 25, 2015 1:14 PM >>> To: Zoltan Kiss >>> Cc: LNG ODP Mailman List >>> Subject: Re: [lng-odp] [API-NEXT PATCH v6] api: packet: allow access to >>> packet flow hash values >>> >>> Hi Zoltan, >>> >>> Few comments inline... >>> >>> On 24 August 2015 at 22:18, Zoltan Kiss <zoltan.kiss@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.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: 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. >> >> A valid hash can be anything, also zero. Application cannot tell from hash value if the hash was actually generated and is valid. As you mention sometimes hash cannot be generated and it varies between platforms when that's the case. In general, application needs to check if hash is there before using it. >> >> Implementation can utilize its knowledge about the algorithm, packet input processing pipeline and packet metadata. For example, if packet is coming from packet input (application hash not stored/clr hash) and it's an IP packet "has hash" is true, but for non-IP packets it's false... > > The concern with adding additional has_hash_flag was that there will > an "if check" in the system for every read hash value and since this > comes in the fast path it might get costly if application reads it in > several places. Anyhow that can be optimized by the implementation. > > Let me summarize my understanding, > > 1. Incoming packet the implementation has generated a hash value HASH1 > 2. Implementation sets the has_hash flag to true. Note, there might be implementations which doesn't provide any hash. In that case they return false, and getting the hash value produces an undefined result. Even in this case the application has to store HASH2 somewhere at step #3, and be able to set the flag. > 3. Application calls set_hash() function and sets a hash value HASH2 > 4. when application reads the packet_hash_value() implementation > returns the value HASH2 > 5. If application calls clear_hash_flag() then has_hash_flag() > function will return false and implementation can return any value if > packet_hash_value() function if called by application by mistake. Yes, that's an undefined behaviour. > > Basically this means that once hash value is set by the application it > replaces any system generated hash value. > > Regards, > Bala > >> >> -Petri >> >>> >>>> + >>>> +/** >>>> * 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
On 25/08/15 11:14, Bala Manoharan wrote: > Hi Zoltan, > > Few comments inline... > > On 24 August 2015 at 22:18, Zoltan Kiss <zoltan.kiss@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.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: 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. I thought the first note implicitly suggest that, I'll add there this sentence: "Overwriting the platform provided value doesn't change how the platform handles this flow after it." > > 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. I'll add two note to odp_packet_flow_hash() to spell out: - the implementation can choose not to generate any hash at all - if set_hash were not called on this packet, it returns what the platform originally generated (if any) > >> +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 >
Hi, On 25/08/15 12:24, Bala Manoharan wrote: > The concern with adding additional has_hash_flag was that there will > an "if check" in the system for every read hash value and since this > comes in the fast path it might get costly if application reads it in > several places. Anyhow that can be optimized by the implementation. If the implementation can guarantee that it generates some hash for every packet, it can just return non-zero here, so the compiler can optimize it out. DPDK can be configured at pktio creation time to set up what kind of packets should be handled by RSS, therefore what headers are in the hash. But I guess we should expect that for non-supported protocols the hash value can end up undefined, so we should check the flag for sure. I think branch predictors can eliminate most of the performance impact. Zoli
I don't see what problems would arise by saying that if the has is not present we return zeros for the hash value. Hashes serve a statistical function. The 1-in-4-billion chance that an actual hash value would be misinterpreted as unset simply means that the application will take some default action for those packets. Hardly cause for worry, especially for systems that actually produce hashes for each packet. Compare that to the benefit of eliminating all of the extraneous "if hash set" logic, it would seem to be a reasonable tradeoff. On Tue, Aug 25, 2015 at 6:52 AM, Zoltan Kiss <zoltan.kiss@linaro.org> wrote: > Hi, > > On 25/08/15 12:24, Bala Manoharan wrote: > >> The concern with adding additional has_hash_flag was that there will >> an "if check" in the system for every read hash value and since this >> comes in the fast path it might get costly if application reads it in >> several places. Anyhow that can be optimized by the implementation. >> > > If the implementation can guarantee that it generates some hash for every > packet, it can just return non-zero here, so the compiler can optimize it > out. > DPDK can be configured at pktio creation time to set up what kind of > packets should be handled by RSS, therefore what headers are in the hash. > But I guess we should expect that for non-supported protocols the hash > value can end up undefined, so we should check the flag for sure. I think > branch predictors can eliminate most of the performance impact. > > Zoli > > _______________________________________________ > lng-odp mailing list > lng-odp@lists.linaro.org > https://lists.linaro.org/mailman/listinfo/lng-odp >
On 25/08/15 14:29, Savolainen, Petri (Nokia - FI/Espoo) wrote: > A specific hash value in the API smells like a DoS attack (generate > lot’s of packet that produce hash == 0). RSS is protected against it, the hash function relies on a secret key to avoid attackers to figure out what different flows are mapping to the same hash value > If application needs a valid > hash for a next step, it would need to calculate one or otherwise try to > figure out if the zero hash really is a valid hash value or not. > > -Petri > > *From:*ext Bill Fischofer [mailto:bill.fischofer@linaro.org] > *Sent:* Tuesday, August 25, 2015 3:02 PM > *To:* Zoltan Kiss > *Cc:* Bala Manoharan; Savolainen, Petri (Nokia - FI/Espoo); LNG ODP > Mailman List > *Subject:* Re: [lng-odp] [API-NEXT PATCH v6] api: packet: allow access > to packet flow hash values > > I don't see what problems would arise by saying that if the has is not > present we return zeros for the hash value. Hashes serve a statistical > function. The 1-in-4-billion chance that an actual hash value would be > misinterpreted as unset simply means that the application will take some > default action for those packets. Hardly cause for worry, especially > for systems that actually produce hashes for each packet. Compare that > to the benefit of eliminating all of the extraneous "if hash set" logic, > it would seem to be a reasonable tradeoff. > > On Tue, Aug 25, 2015 at 6:52 AM, Zoltan Kiss <zoltan.kiss@linaro.org > <mailto:zoltan.kiss@linaro.org>> wrote: > > Hi, > > On 25/08/15 12:24, Bala Manoharan wrote: > > The concern with adding additional has_hash_flag was that there will > an "if check" in the system for every read hash value and since this > comes in the fast path it might get costly if application reads it in > several places. Anyhow that can be optimized by the implementation. > > > If the implementation can guarantee that it generates some hash for > every packet, it can just return non-zero here, so the compiler can > optimize it out. > DPDK can be configured at pktio creation time to set up what kind of > packets should be handled by RSS, therefore what headers are in the > hash. But I guess we should expect that for non-supported protocols the > hash value can end up undefined, so we should check the flag for sure. I > think branch predictors can eliminate most of the performance impact. > > Zoli > > > _______________________________________________ > lng-odp mailing list > lng-odp@lists.linaro.org <mailto:lng-odp@lists.linaro.org> > https://lists.linaro.org/mailman/listinfo/lng-odp >
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. + */ +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); + +/** * 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); + +/** * @} */
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.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: 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(+)