| Message ID | 1439317883-9334-2-git-send-email-zoltan.kiss@linaro.org |
|---|---|
| State | New |
| Headers | show |
Why return a pointer to the hash value rather than the hash value itself? Returning a pointer requires that the implementation needs to allocate space to store this value, which it may not have if it doesn't use hashes. Returning the value seems simpler and we can simply say that a hash value of 0 means no hash is available. Also not clear why odp_pktio_hash_len() is needed. What's the intended use for this API? On Tue, Aug 11, 2015 at 1:31 PM, Zoltan Kiss <zoltan.kiss@linaro.org> wrote: > Applications can access the computed hash (if any) through these functions, > and query the length the platform supports to store. > > Signed-off-by: Zoltan Kiss <zoltan.kiss@linaro.org> > --- > include/odp/api/packet.h | 16 ++++++++++++++++ > include/odp/api/packet_io.h | 14 ++++++++++++++ > 2 files changed, 30 insertions(+) > > diff --git a/include/odp/api/packet.h b/include/odp/api/packet.h > index 3a454b5..66fa2a9 100644 > --- a/include/odp/api/packet.h > +++ b/include/odp/api/packet.h > @@ -615,6 +615,22 @@ int odp_packet_l4_offset_set(odp_packet_t pkt, > uint32_t offset); > int odp_packet_is_segmented(odp_packet_t pkt); > > /** > + * Hash pointer > + * > + * Returns pointer to the hash of the packet. Optionally returns the > length of > + * the hash as well. > + * > + * @param pkt Packet handle > + * @param[out] len Length of hash area (output). > + * Ignored when NULL. > + * > + * @return Hash pointer > + * > + * @see odp_pktio_hash_len() > + */ > +void *odp_packet_hash_ptr(odp_packet_t pkt, uint32_t *len); > + > +/** > * Number of segments > * > * Returns number of segments in the packet. A packet has always at least > one > diff --git a/include/odp/api/packet_io.h b/include/odp/api/packet_io.h > index e00d011..a065567 100644 > --- a/include/odp/api/packet_io.h > +++ b/include/odp/api/packet_io.h > @@ -321,6 +321,20 @@ int odp_pktio_skip_set(odp_pktio_t pktio, uint32_t > offset); > int odp_pktio_headroom_set(odp_pktio_t pktio, uint32_t headroom); > > /** > + * Hash length > + * > + * Returns length of hash space for this packet IO. > + * > + * @param pkt Packet handle > + * Ignored when NULL. > + * > + * @return Hash length > + * > + * @see odp_packet_hash_ptr() > + */ > +uint32_t odp_pktio_hash_len(odp_pktio_t pktio); > + > +/** > * Get printable value for an odp_pktio_t > * > * @param pktio odp_pktio_t handle to be printed > -- > 1.9.1 > > _______________________________________________ > lng-odp mailing list > lng-odp@lists.linaro.org > https://lists.linaro.org/mailman/listinfo/lng-odp >
On 11/08/15 20:42, Bill Fischofer wrote: > Why return a pointer to the hash value rather than the hash value > itself? Returning a pointer requires that the implementation needs to > allocate space to store this value, which it may not have if it doesn't > use hashes. Returning the value seems simpler and we can simply say > that a hash value of 0 means no hash is available. The main reason is that I want write access to the hash itself, so if the implementation doesn't calculate the hash, the application still has a place to store the hash calculated by itself. Or if it changes the packet while processing, it has the ability to change the hash accordingly Also returning something like an uint32_t would restrict how big hash we can use. E.g. FDIR stores 8 bytes there: http://dpdk.org/browse/dpdk/tree/lib/librte_mbuf/rte_mbuf.h#n812 > > Also not clear why odp_pktio_hash_len() is needed. What's the intended > use for this API? > > On Tue, Aug 11, 2015 at 1:31 PM, Zoltan Kiss <zoltan.kiss@linaro.org > <mailto:zoltan.kiss@linaro.org>> wrote: > > Applications can access the computed hash (if any) through these > functions, > and query the length the platform supports to store. > > Signed-off-by: Zoltan Kiss <zoltan.kiss@linaro.org > <mailto:zoltan.kiss@linaro.org>> > --- > include/odp/api/packet.h | 16 ++++++++++++++++ > include/odp/api/packet_io.h | 14 ++++++++++++++ > 2 files changed, 30 insertions(+) > > diff --git a/include/odp/api/packet.h b/include/odp/api/packet.h > index 3a454b5..66fa2a9 100644 > --- a/include/odp/api/packet.h > +++ b/include/odp/api/packet.h > @@ -615,6 +615,22 @@ int odp_packet_l4_offset_set(odp_packet_t pkt, > uint32_t offset); > int odp_packet_is_segmented(odp_packet_t pkt); > > /** > + * Hash pointer > + * > + * Returns pointer to the hash of the packet. Optionally returns > the length of > + * the hash as well. > + * > + * @param pkt Packet handle > + * @param[out] len Length of hash area (output). > + * Ignored when NULL. > + * > + * @return Hash pointer > + * > + * @see odp_pktio_hash_len() > + */ > +void *odp_packet_hash_ptr(odp_packet_t pkt, uint32_t *len); > + > +/** > * Number of segments > * > * Returns number of segments in the packet. A packet has always > at least one > diff --git a/include/odp/api/packet_io.h b/include/odp/api/packet_io.h > index e00d011..a065567 100644 > --- a/include/odp/api/packet_io.h > +++ b/include/odp/api/packet_io.h > @@ -321,6 +321,20 @@ int odp_pktio_skip_set(odp_pktio_t pktio, > uint32_t offset); > int odp_pktio_headroom_set(odp_pktio_t pktio, uint32_t headroom); > > /** > + * Hash length > + * > + * Returns length of hash space for this packet IO. > + * > + * @param pkt Packet handle > + * Ignored when NULL. > + * > + * @return Hash length > + * > + * @see odp_packet_hash_ptr() > + */ > +uint32_t odp_pktio_hash_len(odp_pktio_t pktio); > + > +/** > * Get printable value for an odp_pktio_t > * > * @param pktio odp_pktio_t handle to be printed > -- > 1.9.1 > > _______________________________________________ > lng-odp mailing list > lng-odp@lists.linaro.org <mailto:lng-odp@lists.linaro.org> > https://lists.linaro.org/mailman/listinfo/lng-odp > >
Then this should be done via a setter for consistency with the handling of other ODP metadata. ODP can use a uint64_t as the hash value if a uint32_t is insufficient. Or we can define the packet hash as type odp_packet_hash_t if we want to leave that as platform-dependent. Only downside of this is then you have to be able to convert to/from that abstraction, so uint64_t is probably simpler. So then we'd have: uint64_t odp_packet_hash(odp_packet_t pkt); /* 0 => No hash */ and void odp_packet_hash_set(odp_packet_t pkt, uint64_t hash); The problem with returning pointers to metadata fields is such APIs expose underlying implementations or force incompatible implementations to artificially construct temporaries that the application (falsely) assumes is "more efficient". For all you know, the hash is actually a HW Special Purpose Register (SPR) on a given platform that is a 1 cycle read (or write) and forcing the implementation to expose this as a temporary that the application can read is wasteful and is also problematic for the implementation when the application stores into such a temporary and assumes it's having some sort of effect. That's why metadata manipulation in ODP is done via getters and setters. On Wed, Aug 12, 2015 at 4:40 AM, Zoltan Kiss <zoltan.kiss@linaro.org> wrote: > > > On 11/08/15 20:42, Bill Fischofer wrote: > >> Why return a pointer to the hash value rather than the hash value >> itself? Returning a pointer requires that the implementation needs to >> allocate space to store this value, which it may not have if it doesn't >> use hashes. Returning the value seems simpler and we can simply say >> that a hash value of 0 means no hash is available. >> > > The main reason is that I want write access to the hash itself, so if the > implementation doesn't calculate the hash, the application still has a > place to store the hash calculated by itself. Or if it changes the packet > while processing, it has the ability to change the hash accordingly > > Also returning something like an uint32_t would restrict how big hash we > can use. E.g. FDIR stores 8 bytes there: > > http://dpdk.org/browse/dpdk/tree/lib/librte_mbuf/rte_mbuf.h#n812 > > > >> Also not clear why odp_pktio_hash_len() is needed. What's the intended >> use for this API? >> >> On Tue, Aug 11, 2015 at 1:31 PM, Zoltan Kiss <zoltan.kiss@linaro.org >> <mailto:zoltan.kiss@linaro.org>> wrote: >> >> Applications can access the computed hash (if any) through these >> functions, >> and query the length the platform supports to store. >> >> Signed-off-by: Zoltan Kiss <zoltan.kiss@linaro.org >> <mailto:zoltan.kiss@linaro.org>> >> >> --- >> include/odp/api/packet.h | 16 ++++++++++++++++ >> include/odp/api/packet_io.h | 14 ++++++++++++++ >> 2 files changed, 30 insertions(+) >> >> diff --git a/include/odp/api/packet.h b/include/odp/api/packet.h >> index 3a454b5..66fa2a9 100644 >> --- a/include/odp/api/packet.h >> +++ b/include/odp/api/packet.h >> @@ -615,6 +615,22 @@ int odp_packet_l4_offset_set(odp_packet_t pkt, >> uint32_t offset); >> int odp_packet_is_segmented(odp_packet_t pkt); >> >> /** >> + * Hash pointer >> + * >> + * Returns pointer to the hash of the packet. Optionally returns >> the length of >> + * the hash as well. >> + * >> + * @param pkt Packet handle >> + * @param[out] len Length of hash area (output). >> + * Ignored when NULL. >> + * >> + * @return Hash pointer >> + * >> + * @see odp_pktio_hash_len() >> + */ >> +void *odp_packet_hash_ptr(odp_packet_t pkt, uint32_t *len); >> + >> +/** >> * Number of segments >> * >> * Returns number of segments in the packet. A packet has always >> at least one >> diff --git a/include/odp/api/packet_io.h b/include/odp/api/packet_io.h >> index e00d011..a065567 100644 >> --- a/include/odp/api/packet_io.h >> +++ b/include/odp/api/packet_io.h >> @@ -321,6 +321,20 @@ int odp_pktio_skip_set(odp_pktio_t pktio, >> uint32_t offset); >> int odp_pktio_headroom_set(odp_pktio_t pktio, uint32_t headroom); >> >> /** >> + * Hash length >> + * >> + * Returns length of hash space for this packet IO. >> + * >> + * @param pkt Packet handle >> + * Ignored when NULL. >> + * >> + * @return Hash length >> + * >> + * @see odp_packet_hash_ptr() >> + */ >> +uint32_t odp_pktio_hash_len(odp_pktio_t pktio); >> + >> +/** >> * Get printable value for an odp_pktio_t >> * >> * @param pktio odp_pktio_t handle to be printed >> -- >> 1.9.1 >> >> _______________________________________________ >> 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..66fa2a9 100644 --- a/include/odp/api/packet.h +++ b/include/odp/api/packet.h @@ -615,6 +615,22 @@ int odp_packet_l4_offset_set(odp_packet_t pkt, uint32_t offset); int odp_packet_is_segmented(odp_packet_t pkt); /** + * Hash pointer + * + * Returns pointer to the hash of the packet. Optionally returns the length of + * the hash as well. + * + * @param pkt Packet handle + * @param[out] len Length of hash area (output). + * Ignored when NULL. + * + * @return Hash pointer + * + * @see odp_pktio_hash_len() + */ +void *odp_packet_hash_ptr(odp_packet_t pkt, uint32_t *len); + +/** * Number of segments * * Returns number of segments in the packet. A packet has always at least one diff --git a/include/odp/api/packet_io.h b/include/odp/api/packet_io.h index e00d011..a065567 100644 --- a/include/odp/api/packet_io.h +++ b/include/odp/api/packet_io.h @@ -321,6 +321,20 @@ int odp_pktio_skip_set(odp_pktio_t pktio, uint32_t offset); int odp_pktio_headroom_set(odp_pktio_t pktio, uint32_t headroom); /** + * Hash length + * + * Returns length of hash space for this packet IO. + * + * @param pkt Packet handle + * Ignored when NULL. + * + * @return Hash length + * + * @see odp_packet_hash_ptr() + */ +uint32_t odp_pktio_hash_len(odp_pktio_t pktio); + +/** * Get printable value for an odp_pktio_t * * @param pktio odp_pktio_t handle to be printed
Applications can access the computed hash (if any) through these functions, and query the length the platform supports to store. Signed-off-by: Zoltan Kiss <zoltan.kiss@linaro.org> --- include/odp/api/packet.h | 16 ++++++++++++++++ include/odp/api/packet_io.h | 14 ++++++++++++++ 2 files changed, 30 insertions(+)