| Message ID | 1439577746-21499-1-git-send-email-zoltan.kiss@linaro.org |
|---|---|
| State | New |
| Headers | show |
On 15 August 2015 at 00:12, 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 > > include/odp/api/packet.h | 30 ++++++++++++++++++++++++++++++ > 1 file changed, 30 insertions(+) > > diff --git a/include/odp/api/packet.h b/include/odp/api/packet.h > index 3a454b5..1ae24bc 100644 > --- a/include/odp/api/packet.h > +++ b/include/odp/api/packet.h > @@ -48,6 +48,11 @@ extern "C" { > * Invalid packet segment > */ > > +/** > + * @def ODP_PACKET_RSS_INVALID > + * RSS hash is not set > + */ > + > /* > * > * Alloc and free > @@ -605,6 +610,31 @@ uint32_t odp_packet_l4_offset(odp_packet_t pkt); > int odp_packet_l4_offset_set(odp_packet_t pkt, uint32_t offset); > > /** > + * RSS hash value > + * > + * Returns the RSS hash stored for the packet. > + * > + * @param pkt Packet handle > + * > + * @return Hash value > + * @retval ODP_PACKET_RSS_INVALID if RSS hash is not set. > + */ > +uint32_t odp_packet_rss_hash(odp_packet_t pkt); > + > +/** > + * Set RSS hash value > + * > + * Store the RSS hash for the packet. > + * > + * @param pkt Packet handle > + * @param rss_hash Hash value to set > + * > + * @note If the application changes the packet header, it might want to > + * recalculate this value and set it. > + */ > +void odp_packet_rss_hash_set(odp_packet_t pkt, uint32_t rss_hash); > + > +/** > * Tests if packet is segmented > * > * @param pkt Packet handle > -- Reviewed-by : Santosh Shukla <santosh.shukla@linaro.org> > 1.9.1 > > _______________________________________________ > lng-odp mailing list > lng-odp@lists.linaro.org > https://lists.linaro.org/mailman/listinfo/lng-odp
On Sunday 16 August 2015 06:14 PM, Santosh Shukla wrote: > On 15 August 2015 at 00:12, 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 >> >> include/odp/api/packet.h | 30 ++++++++++++++++++++++++++++++ >> 1 file changed, 30 insertions(+) >> >> diff --git a/include/odp/api/packet.h b/include/odp/api/packet.h >> index 3a454b5..1ae24bc 100644 >> --- a/include/odp/api/packet.h >> +++ b/include/odp/api/packet.h >> @@ -48,6 +48,11 @@ extern "C" { >> * Invalid packet segment >> */ >> >> +/** >> + * @def ODP_PACKET_RSS_INVALID >> + * RSS hash is not set >> + */ >> + >> /* >> * >> * Alloc and free >> @@ -605,6 +610,31 @@ uint32_t odp_packet_l4_offset(odp_packet_t pkt); >> int odp_packet_l4_offset_set(odp_packet_t pkt, uint32_t offset); >> >> /** >> + * RSS hash value >> + * >> + * Returns the RSS hash stored for the packet. >> + * >> + * @param pkt Packet handle >> + * >> + * @return Hash value >> + * @retval ODP_PACKET_RSS_INVALID if RSS hash is not set. >> + */ >> +uint32_t odp_packet_rss_hash(odp_packet_t pkt); >> + >> +/** >> + * Set RSS hash value >> + * >> + * Store the RSS hash for the packet. >> + * >> + * @param pkt Packet handle >> + * @param rss_hash Hash value to set >> + * >> + * @note If the application changes the packet header, it might want to >> + * recalculate this value and set it. >> + */ >> +void odp_packet_rss_hash_set(odp_packet_t pkt, uint32_t rss_hash); Can this use-case be handled by calling odp_packet_generate_rss_hash(odp_packet_t pkt) function where the rss gets generated by the implementation rather than being set from application using odp_packet_set_rss_hash() function? Regards, Bala >> + >> +/** >> * Tests if packet is segmented >> * >> * @param pkt Packet handle >> -- > Reviewed-by : Santosh Shukla <santosh.shukla@linaro.org> > >> 1.9.1 >> >> _______________________________________________ >> lng-odp mailing list >> lng-odp@lists.linaro.org >> https://lists.linaro.org/mailman/listinfo/lng-odp > _______________________________________________ > lng-odp mailing list > lng-odp@lists.linaro.org > https://lists.linaro.org/mailman/listinfo/lng-odp
On 20 August 2015 at 14:48, Balasubramanian Manoharan <bala.manoharan@linaro.org> wrote: > > > On Sunday 16 August 2015 06:14 PM, Santosh Shukla wrote: >> >> On 15 August 2015 at 00:12, 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 >>> >>> include/odp/api/packet.h | 30 ++++++++++++++++++++++++++++++ >>> 1 file changed, 30 insertions(+) >>> >>> diff --git a/include/odp/api/packet.h b/include/odp/api/packet.h >>> index 3a454b5..1ae24bc 100644 >>> --- a/include/odp/api/packet.h >>> +++ b/include/odp/api/packet.h >>> @@ -48,6 +48,11 @@ extern "C" { >>> * Invalid packet segment >>> */ >>> >>> +/** >>> + * @def ODP_PACKET_RSS_INVALID >>> + * RSS hash is not set >>> + */ >>> + >>> /* >>> * >>> * Alloc and free >>> @@ -605,6 +610,31 @@ uint32_t odp_packet_l4_offset(odp_packet_t pkt); >>> int odp_packet_l4_offset_set(odp_packet_t pkt, uint32_t offset); >>> >>> /** >>> + * RSS hash value >>> + * >>> + * Returns the RSS hash stored for the packet. >>> + * >>> + * @param pkt Packet handle >>> + * >>> + * @return Hash value >>> + * @retval ODP_PACKET_RSS_INVALID if RSS hash is not set. >>> + */ >>> +uint32_t odp_packet_rss_hash(odp_packet_t pkt); >>> + >>> +/** >>> + * Set RSS hash value >>> + * >>> + * Store the RSS hash for the packet. >>> + * >>> + * @param pkt Packet handle >>> + * @param rss_hash Hash value to set >>> + * >>> + * @note If the application changes the packet header, it might want to >>> + * recalculate this value and set it. >>> + */ >>> +void odp_packet_rss_hash_set(odp_packet_t pkt, uint32_t rss_hash); > > Can this use-case be handled by calling > odp_packet_generate_rss_hash(odp_packet_t pkt) function where the rss gets > generated by the implementation rather than being set from application using > odp_packet_set_rss_hash() function? > Bala, As we discussed and in summary for rest; Considering ovs example : ovs uses sw based rss_hash only when HW not supporting rss Or HW generated rss is zero. hash = packet_get_hash(packet); if (OVS_UNLIKELY(!hash)) { hash = miniflow_hash_5tuple(mf, 0); packet_set_hash(packet, hash); } return hash; and rss_hash generation is inside ovs dp_packet (middle layer), It is with in ovs implementation, Not exposed to application layer, so application won't need to generate rss / update, so no possibility of rss mis-match /corruption. > Regards, > Bala > >>> + >>> +/** >>> * Tests if packet is segmented >>> * >>> * @param pkt Packet handle >>> -- >> >> Reviewed-by : Santosh Shukla <santosh.shukla@linaro.org> >> >>> 1.9.1 >>> >>> _______________________________________________ >>> lng-odp mailing list >>> lng-odp@lists.linaro.org >>> https://lists.linaro.org/mailman/listinfo/lng-odp >> >> _______________________________________________ >> lng-odp mailing list >> lng-odp@lists.linaro.org >> https://lists.linaro.org/mailman/listinfo/lng-odp > >
On 20 August 2015 at 16:23, Jerin Jacob <jerin.jacob@caviumnetworks.com> wrote: > On Thu, Aug 20, 2015 at 03:30:23PM +0530, Santosh Shukla wrote: >> On 20 August 2015 at 14:48, Balasubramanian Manoharan >> <bala.manoharan@linaro.org> wrote: >> > >> > >> > On Sunday 16 August 2015 06:14 PM, Santosh Shukla wrote: >> >> >> >> On 15 August 2015 at 00:12, 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 >> >>> >> >>> include/odp/api/packet.h | 30 ++++++++++++++++++++++++++++++ >> >>> 1 file changed, 30 insertions(+) >> >>> >> >>> diff --git a/include/odp/api/packet.h b/include/odp/api/packet.h >> >>> index 3a454b5..1ae24bc 100644 >> >>> --- a/include/odp/api/packet.h >> >>> +++ b/include/odp/api/packet.h >> >>> @@ -48,6 +48,11 @@ extern "C" { >> >>> * Invalid packet segment >> >>> */ >> >>> >> >>> +/** >> >>> + * @def ODP_PACKET_RSS_INVALID >> >>> + * RSS hash is not set >> >>> + */ >> >>> + >> >>> /* >> >>> * >> >>> * Alloc and free >> >>> @@ -605,6 +610,31 @@ uint32_t odp_packet_l4_offset(odp_packet_t pkt); >> >>> int odp_packet_l4_offset_set(odp_packet_t pkt, uint32_t offset); >> >>> >> >>> /** >> >>> + * RSS hash value >> >>> + * >> >>> + * Returns the RSS hash stored for the packet. >> >>> + * >> >>> + * @param pkt Packet handle >> >>> + * >> >>> + * @return Hash value >> >>> + * @retval ODP_PACKET_RSS_INVALID if RSS hash is not set. >> >>> + */ >> >>> +uint32_t odp_packet_rss_hash(odp_packet_t pkt); >> >>> + >> >>> +/** >> >>> + * Set RSS hash value >> >>> + * >> >>> + * Store the RSS hash for the packet. >> >>> + * >> >>> + * @param pkt Packet handle >> >>> + * @param rss_hash Hash value to set >> >>> + * >> >>> + * @note If the application changes the packet header, it might want to >> >>> + * recalculate this value and set it. >> >>> + */ >> >>> +void odp_packet_rss_hash_set(odp_packet_t pkt, uint32_t rss_hash); >> > >> > Can this use-case be handled by calling >> > odp_packet_generate_rss_hash(odp_packet_t pkt) function where the rss gets >> > generated by the implementation rather than being set from application using >> > odp_packet_set_rss_hash() function? >> > >> >> Bala, As we discussed and in summary for rest; Considering ovs example >> : ovs uses sw based rss_hash only when HW not supporting rss Or HW >> generated rss is zero. >> >> hash = packet_get_hash(packet); >> if (OVS_UNLIKELY(!hash)) { >> hash = miniflow_hash_5tuple(mf, 0); >> packet_set_hash(packet, hash); >> } >> return hash; >> >> and rss_hash generation is inside ovs dp_packet (middle layer), It is > > How about following change in OVS plarform specific packet_get_hash code. > odp_packet_rss_hash_set > kind of interface wont fit correctly in octeon platform as hash > identifier used by hardware in subsequent HW based queue operations > > static inline uint32_t > packet_get_hash(struct dp_packet *p) > { > #ifdef DPDK_NETDEV > return p->mbuf.hash.rss; > #else > #ifdef ODP_NETDEV > unit32_t hash; > hash = odp_packet_rss_hash(p->odp_pkt); > if (OVS_UNLIKELY(!hash) > hash = odp_packet_generate_rss_hash(p->odp_pkt); > return hash; > #endif > return p->rss_hash; > #endif > } > Trying to understand your api definition; odp_packet_rss_hash() -> to get rss and odp_packet_generate_rss_hash() -> generate rss from where : sw or hw? In either case, your trying to generate non-zero rss (considering a valid rss (?) for your platform). - IIUC _generate_rss_hash() able to take corrective measure so you won't find need for using rss_hash_set() right? - In other word, No s/w based rss __setting__ required for octeon. did I understood all that right? > >> with in ovs implementation, Not exposed to application layer, so >> application won't need to generate rss / update, so no possibility of >> rss mis-match /corruption. >> >> > Regards, >> > Bala >> > >> >>> + >> >>> +/** >> >>> * Tests if packet is segmented >> >>> * >> >>> * @param pkt Packet handle >> >>> -- >> >> >> >> Reviewed-by : Santosh Shukla <santosh.shukla@linaro.org> >> >> >> >>> 1.9.1 >> >>> >> >>> _______________________________________________ >> >>> lng-odp mailing list >> >>> lng-odp@lists.linaro.org >> >>> https://lists.linaro.org/mailman/listinfo/lng-odp >> >> >> >> _______________________________________________ >> >> lng-odp mailing list >> >> lng-odp@lists.linaro.org >> >> https://lists.linaro.org/mailman/listinfo/lng-odp >> > >> > >> _______________________________________________ >> lng-odp mailing list >> lng-odp@lists.linaro.org >> https://lists.linaro.org/mailman/listinfo/lng-odp
The RSS is relevant to packets originating from a NIC and is independent of the CoS or other flow designators. It's there mainly because some applications (e.g., OVS) use it internally, so it's for legacy support. On Thu, Aug 20, 2015 at 6:43 AM, Jerin Jacob <jerin.jacob@caviumnetworks.com > wrote: > On Thu, Aug 20, 2015 at 04:52:29PM +0530, Santosh Shukla wrote: > > On 20 August 2015 at 16:23, Jerin Jacob <jerin.jacob@caviumnetworks.com> > wrote: > > > On Thu, Aug 20, 2015 at 03:30:23PM +0530, Santosh Shukla wrote: > > >> On 20 August 2015 at 14:48, Balasubramanian Manoharan > > >> <bala.manoharan@linaro.org> wrote: > > >> > > > >> > > > >> > On Sunday 16 August 2015 06:14 PM, Santosh Shukla wrote: > > >> >> > > >> >> On 15 August 2015 at 00:12, 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 > > >> >>> > > >> >>> include/odp/api/packet.h | 30 ++++++++++++++++++++++++++++++ > > >> >>> 1 file changed, 30 insertions(+) > > >> >>> > > >> >>> diff --git a/include/odp/api/packet.h b/include/odp/api/packet.h > > >> >>> index 3a454b5..1ae24bc 100644 > > >> >>> --- a/include/odp/api/packet.h > > >> >>> +++ b/include/odp/api/packet.h > > >> >>> @@ -48,6 +48,11 @@ extern "C" { > > >> >>> * Invalid packet segment > > >> >>> */ > > >> >>> > > >> >>> +/** > > >> >>> + * @def ODP_PACKET_RSS_INVALID > > >> >>> + * RSS hash is not set > > >> >>> + */ > > >> >>> + > > >> >>> /* > > >> >>> * > > >> >>> * Alloc and free > > >> >>> @@ -605,6 +610,31 @@ uint32_t odp_packet_l4_offset(odp_packet_t > pkt); > > >> >>> int odp_packet_l4_offset_set(odp_packet_t pkt, uint32_t offset); > > >> >>> > > >> >>> /** > > >> >>> + * RSS hash value > > >> >>> + * > > >> >>> + * Returns the RSS hash stored for the packet. > > >> >>> + * > > >> >>> + * @param pkt Packet handle > > >> >>> + * > > >> >>> + * @return Hash value > > >> >>> + * @retval ODP_PACKET_RSS_INVALID if RSS hash is not set. > > >> >>> + */ > > >> >>> +uint32_t odp_packet_rss_hash(odp_packet_t pkt); > > >> >>> + > > >> >>> +/** > > >> >>> + * Set RSS hash value > > >> >>> + * > > >> >>> + * Store the RSS hash for the packet. > > >> >>> + * > > >> >>> + * @param pkt Packet handle > > >> >>> + * @param rss_hash Hash value to set > > >> >>> + * > > >> >>> + * @note If the application changes the packet header, it might > want to > > >> >>> + * recalculate this value and set it. > > >> >>> + */ > > >> >>> +void odp_packet_rss_hash_set(odp_packet_t pkt, uint32_t > rss_hash); > > >> > > > >> > Can this use-case be handled by calling > > >> > odp_packet_generate_rss_hash(odp_packet_t pkt) function where the > rss gets > > >> > generated by the implementation rather than being set from > application using > > >> > odp_packet_set_rss_hash() function? > > >> > > > >> > > >> Bala, As we discussed and in summary for rest; Considering ovs example > > >> : ovs uses sw based rss_hash only when HW not supporting rss Or HW > > >> generated rss is zero. > > >> > > >> hash = packet_get_hash(packet); > > >> if (OVS_UNLIKELY(!hash)) { > > >> hash = miniflow_hash_5tuple(mf, 0); > > >> packet_set_hash(packet, hash); > > >> } > > >> return hash; > > >> > > >> and rss_hash generation is inside ovs dp_packet (middle layer), It is > > > > > > How about following change in OVS plarform specific packet_get_hash > code. > > > odp_packet_rss_hash_set > > > kind of interface wont fit correctly in octeon platform as hash > > > identifier used by hardware in subsequent HW based queue operations > > > > > > static inline uint32_t > > > packet_get_hash(struct dp_packet *p) > > > { > > > #ifdef DPDK_NETDEV > > > return p->mbuf.hash.rss; > > > #else > > > #ifdef ODP_NETDEV > > > unit32_t hash; > > > hash = odp_packet_rss_hash(p->odp_pkt); > > > if (OVS_UNLIKELY(!hash) > > > hash = odp_packet_generate_rss_hash(p->odp_pkt); > > > return hash; > > > #endif > > > return p->rss_hash; > > > #endif > > > } > > > > > > > Trying to understand your api definition; > > odp_packet_rss_hash() -> to get rss > > and > > odp_packet_generate_rss_hash() -> generate rss from where : sw or hw? > > NOP if HW is capable, for software generated packet/ non-capable HW it > will be in SW > > > In either case, your trying to generate non-zero rss (considering a > > valid rss (?) for your platform). > > IMO, The term RSS may not be correct in API definition. May be something > like flow id, make sense across all platform. > > Jerin > > > > > - IIUC _generate_rss_hash() able to take corrective measure so you > > won't find need for using rss_hash_set() right? > > - In other word, No s/w based rss __setting__ required for octeon. > > > > did I understood all that right? > > > > > > > >> with in ovs implementation, Not exposed to application layer, so > > >> application won't need to generate rss / update, so no possibility of > > >> rss mis-match /corruption. > > >> > > >> > Regards, > > >> > Bala > > >> > > > >> >>> + > > >> >>> +/** > > >> >>> * Tests if packet is segmented > > >> >>> * > > >> >>> * @param pkt Packet handle > > >> >>> -- > > >> >> > > >> >> Reviewed-by : Santosh Shukla <santosh.shukla@linaro.org> > > >> >> > > >> >>> 1.9.1 > > >> >>> > > >> >>> _______________________________________________ > > >> >>> lng-odp mailing list > > >> >>> lng-odp@lists.linaro.org > > >> >>> https://lists.linaro.org/mailman/listinfo/lng-odp > > >> >> > > >> >> _______________________________________________ > > >> >> lng-odp mailing list > > >> >> lng-odp@lists.linaro.org > > >> >> https://lists.linaro.org/mailman/listinfo/lng-odp > > >> > > > >> > > > >> _______________________________________________ > > >> lng-odp mailing list > > >> lng-odp@lists.linaro.org > > >> https://lists.linaro.org/mailman/listinfo/lng-odp > _______________________________________________ > lng-odp mailing list > lng-odp@lists.linaro.org > https://lists.linaro.org/mailman/listinfo/lng-odp >
Hi, On 20 August 2015 at 17:32, Bill Fischofer <bill.fischofer@linaro.org> wrote: > The RSS is relevant to packets originating from a NIC and is independent > of the CoS or other flow designators. It's there mainly because some > applications (e.g., OVS) use it internally, so it's for legacy support. > This API might be used for legacy applications but if the HW generates RSS by default then that value can be used by the application, somehow this API is seen as a requirement only for OVS but getting the hash value of the packet will be used by other applications also. So we should solve the generic use-case rather than focussing for OVS alone. The issue here is to avoid the application to configure the hash value since it is possible that the application configures a hash value different from what the implementation would have generated for that particular packet flow and this would cause an issue in some platforms. This was the basis on which the removal of set_rss_hash() function was suggested. Regards, Bala > > On Thu, Aug 20, 2015 at 6:43 AM, Jerin Jacob < > jerin.jacob@caviumnetworks.com> wrote: > >> On Thu, Aug 20, 2015 at 04:52:29PM +0530, Santosh Shukla wrote: >> > On 20 August 2015 at 16:23, Jerin Jacob <jerin.jacob@caviumnetworks.com> >> wrote: >> > > On Thu, Aug 20, 2015 at 03:30:23PM +0530, Santosh Shukla wrote: >> > >> On 20 August 2015 at 14:48, Balasubramanian Manoharan >> > >> <bala.manoharan@linaro.org> wrote: >> > >> > >> > >> > >> > >> > On Sunday 16 August 2015 06:14 PM, Santosh Shukla wrote: >> > >> >> >> > >> >> On 15 August 2015 at 00:12, 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 >> > >> >>> >> > >> >>> include/odp/api/packet.h | 30 ++++++++++++++++++++++++++++++ >> > >> >>> 1 file changed, 30 insertions(+) >> > >> >>> >> > >> >>> diff --git a/include/odp/api/packet.h b/include/odp/api/packet.h >> > >> >>> index 3a454b5..1ae24bc 100644 >> > >> >>> --- a/include/odp/api/packet.h >> > >> >>> +++ b/include/odp/api/packet.h >> > >> >>> @@ -48,6 +48,11 @@ extern "C" { >> > >> >>> * Invalid packet segment >> > >> >>> */ >> > >> >>> >> > >> >>> +/** >> > >> >>> + * @def ODP_PACKET_RSS_INVALID >> > >> >>> + * RSS hash is not set >> > >> >>> + */ >> > >> >>> + >> > >> >>> /* >> > >> >>> * >> > >> >>> * Alloc and free >> > >> >>> @@ -605,6 +610,31 @@ uint32_t odp_packet_l4_offset(odp_packet_t >> pkt); >> > >> >>> int odp_packet_l4_offset_set(odp_packet_t pkt, uint32_t >> offset); >> > >> >>> >> > >> >>> /** >> > >> >>> + * RSS hash value >> > >> >>> + * >> > >> >>> + * Returns the RSS hash stored for the packet. >> > >> >>> + * >> > >> >>> + * @param pkt Packet handle >> > >> >>> + * >> > >> >>> + * @return Hash value >> > >> >>> + * @retval ODP_PACKET_RSS_INVALID if RSS hash is not set. >> > >> >>> + */ >> > >> >>> +uint32_t odp_packet_rss_hash(odp_packet_t pkt); >> > >> >>> + >> > >> >>> +/** >> > >> >>> + * Set RSS hash value >> > >> >>> + * >> > >> >>> + * Store the RSS hash for the packet. >> > >> >>> + * >> > >> >>> + * @param pkt Packet handle >> > >> >>> + * @param rss_hash Hash value to set >> > >> >>> + * >> > >> >>> + * @note If the application changes the packet header, it might >> want to >> > >> >>> + * recalculate this value and set it. >> > >> >>> + */ >> > >> >>> +void odp_packet_rss_hash_set(odp_packet_t pkt, uint32_t >> rss_hash); >> > >> > >> > >> > Can this use-case be handled by calling >> > >> > odp_packet_generate_rss_hash(odp_packet_t pkt) function where the >> rss gets >> > >> > generated by the implementation rather than being set from >> application using >> > >> > odp_packet_set_rss_hash() function? >> > >> > >> > >> >> > >> Bala, As we discussed and in summary for rest; Considering ovs >> example >> > >> : ovs uses sw based rss_hash only when HW not supporting rss Or HW >> > >> generated rss is zero. >> > >> >> > >> hash = packet_get_hash(packet); >> > >> if (OVS_UNLIKELY(!hash)) { >> > >> hash = miniflow_hash_5tuple(mf, 0); >> > >> packet_set_hash(packet, hash); >> > >> } >> > >> return hash; >> > >> >> > >> and rss_hash generation is inside ovs dp_packet (middle layer), It >> is >> > > >> > > How about following change in OVS plarform specific packet_get_hash >> code. >> > > odp_packet_rss_hash_set >> > > kind of interface wont fit correctly in octeon platform as hash >> > > identifier used by hardware in subsequent HW based queue operations >> > > >> > > static inline uint32_t >> > > packet_get_hash(struct dp_packet *p) >> > > { >> > > #ifdef DPDK_NETDEV >> > > return p->mbuf.hash.rss; >> > > #else >> > > #ifdef ODP_NETDEV >> > > unit32_t hash; >> > > hash = odp_packet_rss_hash(p->odp_pkt); >> > > if (OVS_UNLIKELY(!hash) >> > > hash = odp_packet_generate_rss_hash(p->odp_pkt); >> > > return hash; >> > > #endif >> > > return p->rss_hash; >> > > #endif >> > > } >> > > >> > >> > Trying to understand your api definition; >> > odp_packet_rss_hash() -> to get rss >> > and >> > odp_packet_generate_rss_hash() -> generate rss from where : sw or hw? >> >> NOP if HW is capable, for software generated packet/ non-capable HW it >> will be in SW >> >> > In either case, your trying to generate non-zero rss (considering a >> > valid rss (?) for your platform). >> >> IMO, The term RSS may not be correct in API definition. May be something >> like flow id, make sense across all platform. >> >> Jerin >> >> > >> > - IIUC _generate_rss_hash() able to take corrective measure so you >> > won't find need for using rss_hash_set() right? >> > - In other word, No s/w based rss __setting__ required for octeon. >> > >> > did I understood all that right? >> > >> > > >> > >> with in ovs implementation, Not exposed to application layer, so >> > >> application won't need to generate rss / update, so no possibility of >> > >> rss mis-match /corruption. >> > >> >> > >> > Regards, >> > >> > Bala >> > >> > >> > >> >>> + >> > >> >>> +/** >> > >> >>> * Tests if packet is segmented >> > >> >>> * >> > >> >>> * @param pkt Packet handle >> > >> >>> -- >> > >> >> >> > >> >> Reviewed-by : Santosh Shukla <santosh.shukla@linaro.org> >> > >> >> >> > >> >>> 1.9.1 >> > >> >>> >> > >> >>> _______________________________________________ >> > >> >>> lng-odp mailing list >> > >> >>> lng-odp@lists.linaro.org >> > >> >>> https://lists.linaro.org/mailman/listinfo/lng-odp >> > >> >> >> > >> >> _______________________________________________ >> > >> >> lng-odp mailing list >> > >> >> lng-odp@lists.linaro.org >> > >> >> https://lists.linaro.org/mailman/listinfo/lng-odp >> > >> > >> > >> > >> > >> _______________________________________________ >> > >> lng-odp mailing list >> > >> lng-odp@lists.linaro.org >> > >> https://lists.linaro.org/mailman/listinfo/lng-odp >> _______________________________________________ >> lng-odp mailing list >> lng-odp@lists.linaro.org >> https://lists.linaro.org/mailman/listinfo/lng-odp >> > > > _______________________________________________ > lng-odp mailing list > lng-odp@lists.linaro.org > https://lists.linaro.org/mailman/listinfo/lng-odp > >
Hi, On 20/08/15 11:53, Jerin Jacob wrote: > How about following change in OVS plarform specific packet_get_hash code. > odp_packet_rss_hash_set > kind of interface wont fit correctly in octeon platform as hash > identifier used by hardware in subsequent HW based queue operations > > static inline uint32_t > packet_get_hash(struct dp_packet *p) > { > #ifdef DPDK_NETDEV > return p->mbuf.hash.rss; > #else > #ifdef ODP_NETDEV > unit32_t hash; > hash = odp_packet_rss_hash(p->odp_pkt); > if (OVS_UNLIKELY(!hash) > hash = odp_packet_generate_rss_hash(p->odp_pkt); > return hash; > #endif > return p->rss_hash; > #endif > } I was thinking about this too, but I see two problems: 1. OVS changes the hash if the packet is currently recirculated: https://github.com/openvswitch/ovs/blob/master/lib/dpif-netdev.c#L3125 And as far as I can see, it doesn't recalculate the hash when the recirculation is finished. So the final hash of the packet at the end won't be what the platform would generate. OVS doesn't seem to care about it, I think the assumption is that the platform won't use this hash anymore, and it's role is finished after matching in EMC. My first idea to sort this out is to check for recirculation depth in netdev_send(), before the send of course. If it's true, then we can regenerate the hash using odp_packet_generate_rss_hash() 2. Minor thing, but if the platform is only able to do this from software (like DPDK), it should better be as fast as OVS's hashing at least. OVS uses CRC32 on SSE4 x64 CPUs, and Murmurhash on others. I don't know how do they compare e.g. with Toeplitz, but I guess they choose them because they are more suitable for the purpose.
Remember that the purpose of RSS is simply to help spread arriving packets to different receiving CPUs. Also, the RSS has itself is not static. It takes as input the RSS key, which defines the packet fields over which the hash is calculated, and the low order bits of it are used as an index into the RSS indirection table. Both the keys and the indirection table are dynamically updated by SW, so the interpretation of a given RSS hash value will vary. So I can't look at a packet and tell you the RSS hash without knowing what key to use, and the interpretation of the hash is dependent on the indirection table (also not part of the hash). Once the packet has been delivered in accordance with the information in the indirection table the "meaning" of the RSS hash is no longer current. At that point it's just a pseudo-random set of bits that is indirectly associated with what were the packet fields of interest at the time the packet was originally received. On Thu, Aug 20, 2015 at 9:20 AM, Zoltan Kiss <zoltan.kiss@linaro.org> wrote: > Hi, > > On 20/08/15 11:53, Jerin Jacob wrote: > >> How about following change in OVS plarform specific packet_get_hash code. >> odp_packet_rss_hash_set >> kind of interface wont fit correctly in octeon platform as hash >> identifier used by hardware in subsequent HW based queue operations >> >> static inline uint32_t >> packet_get_hash(struct dp_packet *p) >> { >> #ifdef DPDK_NETDEV >> return p->mbuf.hash.rss; >> #else >> #ifdef ODP_NETDEV >> unit32_t hash; >> hash = odp_packet_rss_hash(p->odp_pkt); >> if (OVS_UNLIKELY(!hash) >> hash = odp_packet_generate_rss_hash(p->odp_pkt); >> return hash; >> #endif >> return p->rss_hash; >> #endif >> } >> > > I was thinking about this too, but I see two problems: > > 1. OVS changes the hash if the packet is currently recirculated: > > https://github.com/openvswitch/ovs/blob/master/lib/dpif-netdev.c#L3125 > > And as far as I can see, it doesn't recalculate the hash when the > recirculation is finished. So the final hash of the packet at the end won't > be what the platform would generate. OVS doesn't seem to care about it, I > think the assumption is that the platform won't use this hash anymore, and > it's role is finished after matching in EMC. > My first idea to sort this out is to check for recirculation depth in > netdev_send(), before the send of course. If it's true, then we can > regenerate the hash using odp_packet_generate_rss_hash() > > 2. Minor thing, but if the platform is only able to do this from software > (like DPDK), it should better be as fast as OVS's hashing at least. OVS > uses CRC32 on SSE4 x64 CPUs, and Murmurhash on others. I don't know how do > they compare e.g. with Toeplitz, but I guess they choose them because they > are more suitable for the purpose. > > _______________________________________________ > lng-odp mailing list > lng-odp@lists.linaro.org > https://lists.linaro.org/mailman/listinfo/lng-odp >
On 20/08/15 17:55, Jerin Jacob wrote: > On Thu, Aug 20, 2015 at 03:20:46PM +0100, Zoltan Kiss wrote: >> Hi, >> >> On 20/08/15 11:53, Jerin Jacob wrote: >>> How about following change in OVS plarform specific packet_get_hash code. >>> odp_packet_rss_hash_set >>> kind of interface wont fit correctly in octeon platform as hash >>> identifier used by hardware in subsequent HW based queue operations >>> >>> static inline uint32_t >>> packet_get_hash(struct dp_packet *p) >>> { >>> #ifdef DPDK_NETDEV >>> return p->mbuf.hash.rss; >>> #else >>> #ifdef ODP_NETDEV >>> unit32_t hash; >>> hash = odp_packet_rss_hash(p->odp_pkt); >>> if (OVS_UNLIKELY(!hash) >>> hash = odp_packet_generate_rss_hash(p->odp_pkt); >>> return hash; >>> #endif >>> return p->rss_hash; >>> #endif >>> } >> >> I was thinking about this too, but I see two problems: >> >> 1. OVS changes the hash if the packet is currently recirculated: >> >> https://github.com/openvswitch/ovs/blob/master/lib/dpif-netdev.c#L3125 >> >> And as far as I can see, it doesn't recalculate the hash when the >> recirculation is finished. So the final hash of the packet at the end won't >> be what the platform would generate. OVS doesn't seem to care about it, I >> think the assumption is that the platform won't use this hash anymore, and >> it's role is finished after matching in EMC. >> My first idea to sort this out is to check for recirculation depth in >> netdev_send(), before the send of course. If it's true, then we can >> regenerate the hash using odp_packet_generate_rss_hash() > > if its not true then other option could be to hold additional data in > ODP meta data. I mean we only need to call odp_packet_generate_rss_hash if recirculation depth is non-zero. Otherwise OVS won't touch the hash if the platform makes sure there is a hash > >> >> 2. Minor thing, but if the platform is only able to do this from software >> (like DPDK), it should better be as fast as OVS's hashing at least. OVS uses >> CRC32 on SSE4 x64 CPUs, and Murmurhash on others. I don't know how do they > > DPDK do have low level HW accelerated API's for crc 4 and 8 bytes. > Implementing 5 tuple based on CRC on ODP-DPDK port will NOT be be any issue. > And armv8 also has advanced SIMD instruction for CRC 8,4,2 and 1 bytes Yes, but DPDK would need to generate Toeplitz, not CRC, if the NIC is using that. And it might be that it's fast when the NIC does it, but not when the CPU. I'm also considering to let the application know whether the platform requires this hash to set before handed back to the platform (e.g. going out). Platforms like DPDK doesn't care if the application modifies the hash, it won't use it again. So even if it can calculate it fast, it would be a waste of cycles. In that case the application wouldn't need to worry when changing the hash. > >> compare e.g. with Toeplitz, but I guess they choose them because they are >> more suitable for the purpose. > > I guess RSS hash algorithm used Intel nic is Toeplitz to get good > packet distribution using RSS(not for lookup) > > > >
On 20/08/15 17:43, Bill Fischofer wrote: > Remember that the purpose of RSS is simply to help spread arriving > packets to different receiving CPUs. Also, the RSS has itself is not > static. It takes as input the RSS key, which defines the packet fields > over which the hash is calculated, and the low order bits of it are used > as an index into the RSS indirection table. Both the keys and the > indirection table are dynamically updated by SW, so the interpretation > of a given RSS hash value will vary. So I can't look at a packet and > tell you the RSS hash without knowing what key to use, and the > interpretation of the hash is dependent on the indirection table (also > not part of the hash). Once the packet has been delivered in accordance > with the information in the indirection table the "meaning" of the RSS > hash is no longer current. At that point it's just a pseudo-random set > of bits that is indirectly associated with what were the packet fields > of interest at the time the packet was originally received. Currently the purpose is just to have a hash of certain headers, which defines a flow. Later on we can add APIs about how to define that set of headers, right now these bits are the important bits for OVS. Also, the key an the indirection table is not relevant for OVS's use case. > > On Thu, Aug 20, 2015 at 9:20 AM, Zoltan Kiss <zoltan.kiss@linaro.org > <mailto:zoltan.kiss@linaro.org>> wrote: > > Hi, > > On 20/08/15 11:53, Jerin Jacob wrote: > > How about following change in OVS plarform specific > packet_get_hash code. > odp_packet_rss_hash_set > kind of interface wont fit correctly in octeon platform as hash > identifier used by hardware in subsequent HW based queue operations > > static inline uint32_t > packet_get_hash(struct dp_packet *p) > { > #ifdef DPDK_NETDEV > return p->mbuf.hash.rss; > #else > #ifdef ODP_NETDEV > unit32_t hash; > hash = odp_packet_rss_hash(p->odp_pkt); > if (OVS_UNLIKELY(!hash) > hash = odp_packet_generate_rss_hash(p->odp_pkt); > return hash; > #endif > return p->rss_hash; > #endif > } > > > I was thinking about this too, but I see two problems: > > 1. OVS changes the hash if the packet is currently recirculated: > > https://github.com/openvswitch/ovs/blob/master/lib/dpif-netdev.c#L3125 > > And as far as I can see, it doesn't recalculate the hash when the > recirculation is finished. So the final hash of the packet at the > end won't be what the platform would generate. OVS doesn't seem to > care about it, I think the assumption is that the platform won't use > this hash anymore, and it's role is finished after matching in EMC. > My first idea to sort this out is to check for recirculation depth > in netdev_send(), before the send of course. If it's true, then we > can regenerate the hash using odp_packet_generate_rss_hash() > > 2. Minor thing, but if the platform is only able to do this from > software (like DPDK), it should better be as fast as OVS's hashing > at least. OVS uses CRC32 on SSE4 x64 CPUs, and Murmurhash on others. > I don't know how do they compare e.g. with Toeplitz, but I guess > they choose them because they are more suitable for the purpose. > > _______________________________________________ > lng-odp mailing list > lng-odp@lists.linaro.org <mailto:lng-odp@lists.linaro.org> > https://lists.linaro.org/mailman/listinfo/lng-odp > >
On 20/08/15 12:43, Jerin Jacob wrote: > IMO, The term RSS may not be correct in API definition. May be something > like flow id, make sense across all platform. Good idea. I'll replace "rss" with "flow" instead.
An RSS hash is not a flow identifier. NICs produce RSS hashes so it makes sense to be honest about that. On Thursday, August 20, 2015, Zoltan Kiss <zoltan.kiss@linaro.org> wrote: > > > On 20/08/15 12:43, Jerin Jacob wrote: > >> IMO, The term RSS may not be correct in API definition. May be something >> like flow id, make sense across all platform. >> > > Good idea. I'll replace "rss" with "flow" instead. > _______________________________________________ > lng-odp mailing list > lng-odp@lists.linaro.org > https://lists.linaro.org/mailman/listinfo/lng-odp >
On 20/08/15 19:13, Bill Fischofer wrote: > An RSS hash is not a flow identifier. It depends on the actual secret hash as well, that's true. But otherwise it satisfies the requirement that it depends on the selected fields of the header (aka flow) > NICs produce RSS hashes so it > makes sense to be honest about that. I had the impression that on Octeon it's called something else, and I guess it's not exactly like RSS, isn't that true? > > On Thursday, August 20, 2015, Zoltan Kiss <zoltan.kiss@linaro.org > <mailto:zoltan.kiss@linaro.org>> wrote: > > > > On 20/08/15 12:43, Jerin Jacob wrote: > > IMO, The term RSS may not be correct in API definition. May be > something > like flow id, make sense across all platform. > > > Good idea. I'll replace "rss" with "flow" instead. > _______________________________________________ > lng-odp mailing list > 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..1ae24bc 100644 --- a/include/odp/api/packet.h +++ b/include/odp/api/packet.h @@ -48,6 +48,11 @@ extern "C" { * Invalid packet segment */ +/** + * @def ODP_PACKET_RSS_INVALID + * RSS hash is not set + */ + /* * * Alloc and free @@ -605,6 +610,31 @@ uint32_t odp_packet_l4_offset(odp_packet_t pkt); int odp_packet_l4_offset_set(odp_packet_t pkt, uint32_t offset); /** + * RSS hash value + * + * Returns the RSS hash stored for the packet. + * + * @param pkt Packet handle + * + * @return Hash value + * @retval ODP_PACKET_RSS_INVALID if RSS hash is not set. + */ +uint32_t odp_packet_rss_hash(odp_packet_t pkt); + +/** + * Set RSS hash value + * + * Store the RSS hash for the packet. + * + * @param pkt Packet handle + * @param rss_hash Hash value to set + * + * @note If the application changes the packet header, it might want to + * recalculate this value and set it. + */ +void odp_packet_rss_hash_set(odp_packet_t pkt, uint32_t rss_hash); + +/** * Tests if packet is segmented * * @param pkt Packet handle
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 include/odp/api/packet.h | 30 ++++++++++++++++++++++++++++++ 1 file changed, 30 insertions(+)