On Fri, Oct 21, 2016 at 12:07 PM, Bala Manoharan <bala.manoha...@linaro.org> wrote:
> Regards, > Bala > > > On 21 October 2016 at 19:41, Bill Fischofer <bill.fischo...@linaro.org> > wrote: > > > > > > On Fri, Oct 21, 2016 at 8:21 AM, Bala Manoharan < > bala.manoha...@linaro.org> > > wrote: > >> > >> Regards, > >> Bala > >> > >> > >> On 11 October 2016 at 09:15, Bill Fischofer <bill.fischo...@linaro.org> > >> wrote: > >> > Introduce four new APIs that support efficient sharing of portions of > >> > packets. > >> > > >> > odp_packet_splice() creates a reference to a base packet by splicing a > >> > supplied header packet onto it at a specified offset. If multiple > >> > splices > >> > are created then each shares the base packet that was spliced. > >> > > >> > odp_packet_ref() creates a reference to a base packet by splicing a > >> > zero-length header onto it at a specified offset. This allows an > >> > application to "hold onto" a pointer to a packet so that it will not > be > >> > freed until all references to it are freed. > >> > > >> > odp_packet_is_a_splice() allows an application to determine whether a > >> > packet is a splice and if so how many individual packets are contained > >> > within it. > >> > > >> > odp_packet_is_spliced() allows an application to determine whether a > >> > packet > >> > has a splice on it and if so how many splices are based on it. > >> > > >> > Signed-off-by: Bill Fischofer <bill.fischo...@linaro.org> > >> > --- > >> > include/odp/api/spec/packet.h | 103 > >> > ++++++++++++++++++++++++++++++++++++++++++ > >> > 1 file changed, 103 insertions(+) > >> > > >> > diff --git a/include/odp/api/spec/packet.h > >> > b/include/odp/api/spec/packet.h > >> > index 4a14f2d..8e147a3 100644 > >> > --- a/include/odp/api/spec/packet.h > >> > +++ b/include/odp/api/spec/packet.h > >> > @@ -844,6 +844,109 @@ int odp_packet_concat(odp_packet_t *dst, > >> > odp_packet_t src); > >> > */ > >> > int odp_packet_split(odp_packet_t *pkt, uint32_t len, odp_packet_t > >> > *tail); > >> > > >> > +/** > >> > + * Splice a packet > >> > + * > >> > + * Create a new packet by splicing a header onto an existing packet. > >> > The > >> > + * spliced packet consists of the header followed by a shared > reference > >> > to the > >> > + * base packet receiving the splice starting at a designated offset. > If > >> > + * multiple splices are created on the same base packet, it is the > >> > + * application's responsibility to coordinate any changes to the > shared > >> > + * segment(s) of the splice, otherwise results are undefined. > >> > + * > >> > + * @param hdr Handle of the header packet to be spliced onto the > >> > base packet > >> > + * > >> > + * @param pkt Handle of the base packet that is to receive the > >> > splice > >> > + * > >> > + * @param offset Byte offset within the base packet at which the > splice > >> > is > >> > + * to be made. > >> > + * > >> > + * @return Handle of the spliced packet. This may or may not be > >> > the > >> > + * same as the input hdr packet. The caller should use > >> > this > >> > + * for any future references to the splice. The > original > >> > hdr > >> > + * packet no longer has any independent existence. > >> > + * > >> > + * @retval ODP_PACKET_INVALID If the splice failed. In this case both > >> > hdr > >> > + * and pkt are unchanged. > >> > + * > >> > + * @note The base pkt remains valid after the completion of the > splice, > >> > + * however changes that affect its length may yield > unpredictable > >> > + * results when viewed through the returned splice handle. For > >> > best > >> > + * portability and predictable behavior, applications should > >> > regard the > >> > + * base packet as read only following a successful splice, with > >> > the > >> > + * exception of tailroom manipulation (which still requires > >> > application > >> > + * coordination if multiple splices exist on the same base > packet > >> > whose > >> > + * tail is being pushed or pulled). > >> > + * > >> > + * @note Either packet input to this routine may itself be a splice, > >> > however > >> > + * individual implementations may impose limits on how deeply > >> > splices > >> > + * may be nested and fail the attempted splice if those limits > >> > are > >> > + * exceeded. > >> > + * > >> > + * @note The packets used to create a splice may reside in different > >> > pools, > >> > + * however individual implementations may require that both > >> > reside in > >> > + * the same pool and fail the attempted splice if this > >> > restriction is > >> > + * not observed. Upon return odp_packet_pool() returns the pool > >> > id of > >> > + * the header packet. > >> > + * > >> > + * @note Once successfully spliced, the base packet may be freed via > >> > + * odp_packet_free(), however the storage used to represent it > >> > will not > >> > + * be released until all splices based on it have themselves > been > >> > freed. > >> > >> Do you have a reason why the 'base' packet will be freed by the > >> application after creating the slice? > > > > > > The point being made here is only that the storage behind the base packet > > will not be released until all references to it are freed. The > application > > is under no obligation to issue an odp_packet_free() at any particular > time. > > One reason why a packet (or one of its references) may be freed is if it > is > > transmitted since ODP TX processing implicitly calls odp_packet_free() > once > > the odp_packet_t has been sent. So, for example, if a reference to a > packet > > is transmitted then the base packet will still be around since only the > > reference will be freed by the transmit. Or conversely if a packet is > > transmitted but the caller still holds a reference to it then again while > > the base packet will be freed by the transmit the portion of it > contained in > > the reference would still be retained until the reference was itself > freed. > > I believe this is a general concept with reference, whenever you do > not want a packet to be freed during transmission you create a > reference for the packet and then transmit the packet so that the > implementation only decrements the reference count but does not free > the packet. > So I would prefer changing the sentence to reflect the general concept > of reference book keeping rather than explaining about storage of the > base packet, since the concept is same for 'base' packet or a whole > packet and the way it gets implemented is implementation specific. > I have no problem rewording this as the concept remains the same. We don't mention reference counts explicitly, though that's the obvious way for implementations to do the necessary bookkeeping. > > > > >> > >> > >> > + */ > >> > +odp_packet_t odp_packet_splice(odp_packet_t hdr, > >> > + odp_packet_t pkt, uint32_t offset); > >> > >> Do we have an use-case for this scenario? > >> I would prefer that the API take the entire second packet instead of > >> an 'offset' at-least in the first version and that the 'hdr' handle > >> becomes invalid once the packet has been sliced and the new handle > >> returned replaces the 'hdr' handle. > > > > > > The main use case here would be to splice on a preallocated header onto a > > portion of a base packet representing the payload. The alternative > would be > > to create a reference (e.g., splice a zero-length header onto a packet) > and > > then call odp_packet_push_head() to build a header onto the reference. > But > > being able to have these headers preallocated could be more efficient. > > > > As currently defined, the returned handle replaces the hdr packet handle > > since the hdr no longer has an independent identity after the splice. The > > base pkt, by contrast, is still valid since other references/splices can > be > > created on it to result in sharing. So the second part of your request is > > already handled. I tried to make that clear in the doxygen for @return. > > This could be achieved by first creating reference for the hdr packet > and then using the handle returned by the reference API and create a > splice. > That's effectively what happens as a reference is just a splice of a zero-length header packet. Creating a reference to the header would mean that the header would then have to be regarded as read-only, however. Splicing that onto a base would then create a compound reference (you're splicing a reference/splice onto another packet). odp-linux can do that but the question is whether every implementation can do arbitrary compound splices? That may need to be another capability indication. > > > > > Restricting the valid offset values for a splice is something that may be > > needed for some implementations, and that's why I wanted to have this > > discussion. I'd expect such limits to be communicated in an > > odp_packet_capability() API that we'd add. > > I believe offset is something which could be implemented by all HWs > using software interaction but since these APIs are executed in the > fast-path having a separate API for creating reference at 0 offset > could be beneficial as the implementation could be more efficient in > that case. > Petri originally proposed one API and I proposed to split that into two so that references didn't have to specify an explicit null header. Having a third that doesn't use the offset parameter is a possibility, but specifying an offset of 0 seems simple to optimize. But I'm certainly open to having that as a separate API if that makes life easier. > > > > > Of course, if a given implementation can only support splicing at offset > > zero then a non-zero offset splice can be affected by first calling > > odp_packet_pull_head() on the base packet before doing the splice. I > assume > > that you can support that combination of operations? > > > >> > >> > >> > + > >> > +/** > >> > + * Create a reference to a packet > >> > + * > >> > + * Create a (shared) reference to a base packet starting at a > specified > >> > + * byte offset. A reference is simply a splice with a zero-length > >> > header. > >> > + * > >> > + * @param pkt Handle of the base packet for which a reference is > to > >> > be > >> > + * created. > >> > + * @param offset Byte offset in the base packet at which shared > >> > reference is > >> > + * to begin. > >> > + * > >> > + * @return Handle of the reference packet > >> > + * @retval ODP_PACKET_INVALID Operation failed. Base packet remains > >> > unchanged. > >> > + * > >> > + * @note The zero-length header used to create a packet reference is > >> > always > >> > + * drawn from the same pool as the base packet. > >> > + * > >> > + * @note Because references are a type of splice, any bytes pushed > onto > >> > the > >> > + * head of a reference (via odp_packet_push_head() or > >> > + * odp_packet_extend_head() are unique to this reference and > are > >> > not > >> > + * seen by any other reference to the same base packet. > >> > + */ > >> > +odp_packet_t odp_packet_ref(odp_packet_t pkt, uint32_t offset); > >> > >> In the first version we can drop the offset and create a reference of > >> the entire packet. > > > > > > Since a reference is just a splice of a zero-length header any > restriction > > on supported splice offsets would propagate to references as well. Can > this > > be done via a capability() API? The advantage of keeping the offset > (even if > > 0 is the only valid value in the first release) is to preserve API/ABI > > compatibility if this restriction is relaxed in a later release. > > I believe the offset 0 will be the general use-case hence it is better > to keep a separate API for creating reference of entire packet. > Creating multiple references at multiple offset of a packet > complicates the implementation logic and since these codes execute in > the fast path the logic should be as efficient as possible. > > I would prefer adding the simple use-case in this version since I > believe it could be easily agreed upon and implemented by all HWs. > I'd like to get confirmation of that from other HW platforms. Good to have your input on this. > > > >> > >> > >> > + > >> > +/** > >> > + * Tests if a packet is a splice > >> > + * > >> > + * A packet is a splice if it was created by odp_packet_splice() or > >> > + * odp_packet_ref(). Note that a splice may be a simple splice or a > >> > + * compound splice (a splice created from another splice). > >> > + * > >> > + * @param pkt Packet Handle > >> > + * > >> > + * @retval 0 Packet is not a splice > >> > + * @retval >0 Packet is a splice containing N individual packets > >> > + */ > >> > +int odp_packet_is_a_splice(odp_packet_t pkt); > >> > + > >> > +/** > >> > + * Tests if a packet is spliced > >> > + * > >> > + * A packet is spliced if a splice was created on it by > >> > odp_packet_splice() > >> > + * or odp_packet_ref(). > >> > + * > >> > + * @param pkt Packet Handle > >> > + * > >> > + * @retval 0 Packet is not spliced > >> > + * @retval >0 Packet has N splices based on it > >> > + */ > >> > +int odp_packet_is_spliced(odp_packet_t pkt); > >> > + > >> > /* > >> > * > >> > * Copy > >> > -- > >> > 2.7.4 > >> > > > > > >
