On Tue, Dec 20, 2016 at 5:59 AM, Bala Manoharan <bala.manoha...@linaro.org> wrote: > Regards, > Bala > > > On 19 December 2016 at 20:23, Bill Fischofer <bill.fischo...@linaro.org> > wrote: >> On Mon, Dec 19, 2016 at 4:06 AM, Bala Manoharan >> <bala.manoha...@linaro.org> wrote: >>> Comments inline.... >>> >>> >>> On 15 November 2016 at 20:14, Bill Fischofer <bill.fischo...@linaro.org> >>> wrote: >>>> Introduce three new APIs that support efficient sharing of portions of >>>> packets. >>>> >>>> odp_packet_ref_static() creates an alias for a base packet >>>> >>>> odp_packet_ref() creates a reference to a base packet >>>> >>>> odp_packet_ref_pkt() creates a reference to a base packet from a supplied >>>> header packet >>>> >>>> In addition, three other APIs simplify working with references >>>> >>>> odp_packet_is_ref() says whether a packet is a reference >>>> >>>> odp_packet_has_ref() says whether a packet has had a reference made to it >>>> >>>> odp_packet_unshared_len() gives the length of the prefix bytes that are >>>> unique to this reference. These are the only bytes of the packet that may >>>> be modified safely. >>>> >>>> Signed-off-by: Bill Fischofer <bill.fischo...@linaro.org> >>>> --- >>>> include/odp/api/spec/packet.h | 168 >>>> ++++++++++++++++++++++++++++++++++++++++++ >>>> 1 file changed, 168 insertions(+) >>>> >>>> diff --git a/include/odp/api/spec/packet.h b/include/odp/api/spec/packet.h >>>> index faf62e2..137024f 100644 >>>> --- a/include/odp/api/spec/packet.h >>>> +++ b/include/odp/api/spec/packet.h >>>> @@ -256,6 +256,23 @@ uint32_t odp_packet_seg_len(odp_packet_t pkt); >>>> uint32_t odp_packet_len(odp_packet_t pkt); >>>> >>>> /** >>>> + * Packet unshared data len >>>> + * >>>> + * Returns the sum of data lengths over all unshared packet segments. >>>> These >>>> + * are the only bytes that should be modified by the caller. The rest of >>>> the >>>> + * packet should be treated as read-only. >>>> + * >>>> + * This routine will return 0 if odp_packet_has_ref() != 0 and will be the >>>> + * same as odp_packet_len() if odp_packet_has_ref() == 0 and >>>> + * odp_packet_is_ref() == 0. >>>> + * >>>> + * @param pkt Packet handle >>>> + * >>>> + * @return Packet unshared data length >>>> + */ >>>> +uint32_t odp_packet_unshared_len(odp_packet_t pkt); >>>> + >>>> +/** >>>> * Packet headroom length >>>> * >>>> * Returns the current packet level headroom length. >>>> @@ -847,6 +864,157 @@ int odp_packet_split(odp_packet_t *pkt, uint32_t >>>> len, odp_packet_t *tail); >>>> >>>> /* >>>> * >>>> + * References >>>> + * ******************************************************** >>>> + * >>>> + */ >>>> + >>>> +/** >>>> + * Create a static reference to a packet >>>> + * >>>> + * A static reference is used to obtain an additional handle for >>>> referring to >>>> + * a packet so that the storage behind it is not freed until all >>>> references to >>>> + * the packet are freed. This can be used, for example, to support >>>> efficient >>>> + * retransmission processing. >>>> + * >>>> + * The intent of a static reference is that both the base packet and the >>>> + * returned reference will be treated as read-only after this call. >>>> Results >>>> + * are unpredictable if this restriction is not observed. >>>> + * >>>> + * Static references have restrictions but may have performance >>>> advantages on >>>> + * some platforms if the caller does not intend to modify the reference >>>> + * packet. If modification is needed (e.g., to prefix a unique header >>>> onto the >>>> + * packet) then odp_packet_ref() or odp_packet_ref_pkt() should be used. >>>> + * >>>> + * @param pkt Handle of the base packet for which a static reference is >>>> + * to be created. >>>> + * >>>> + * @return Handle of the static reference packet >>>> + * @retval ODP_PACKET_INVALID Operation failed. Base packet remains >>>> unchanged. >>>> + */ >>>> +odp_packet_t odp_packet_ref_static(odp_packet_t pkt); >>> >>> It is better to change the API signature to return the updated handle >>> of the base packet also to the application. >>> Similar to the API for odp_packet_extend_head(). >> >> I think returning the packet ref directly rather than indirectly makes >> for easier coding on the part of applications. Failure is indicated by >> returning ODP_PACKET_INVALID. So in this sense we're like >> odp_packet_alloc(). The alternative: >> >> int odp_packet_ref_static(odp_packet_t pkt, odp_packet_t *refpkt); >> >> seems cumbersome since if RC != 0 then the refpkt return would be >> meaningless. >> >>> >>>> + >>>> +/** >>>> + * Create a reference to a packet >>>> + * >>>> + * Create a dynamic reference to a base packet starting at a specified >>>> byte >>>> + * offset. This reference may be used as an argument to header >>>> manipulation >>>> + * APIs to prefix a unique header onto the shared base. The storage >>>> associated >>>> + * with the base packet is not freed until all references to it are freed, >>>> + * which permits easy multicasting or retransmission processing to be >>>> + * performed. Following a successful call, the base packet should be >>>> treated >>>> + * as read-only. Results are unpredictable if this restriction is not >>>> + * observed. >>>> + * >>>> + * This operation prepends a zero-length header onto the base packet >>>> beginning >>>> + * at the specified offset. This header is always drawn from the same >>>> pool as >>>> + * the base packet. >>>> + * >>>> + * Because references are unique packets, 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 the base packet or by any other >>>> + * reference to the same base packet. >>>> + * >>>> + * The base packet used as input to this routine may itself by a >>>> reference to >>>> + * some other base packet. Implementations MAY restrict the ability to >>>> create >>>> + * such compound references. Attempts to exceed any implementation limits >>>> in >>>> + * this regard will result in this call failing and returning >>>> + * ODP_PACKET_INVALID. >>>> + * >>>> + * If the caller does not indend to push a header onto the returned >>>> reference, >>>> + * the odp_packet_ref_static() API may be used. This may be a more >>>> efficient >>>> + * means of obtaining another reference to a base packet that will be >>>> treated >>>> + * as read-only. >>>> + * >>>> + * @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 the shared >>>> reference >>>> + * is to begin. Must be in the range >>>> 0..odp_packet_len(pkt)-1. >>>> + * >>>> + * @return Handle of the reference packet >>>> + * @retval ODP_PACKET_INVALID Operation failed. Base packet remains >>>> unchanged. >>>> + * >>>> + >>>> + */ >>>> +odp_packet_t odp_packet_ref(odp_packet_t pkt, uint32_t offset); >>>> + >>>> +/** >>>> + * Create a reference to another packet from a header packet >>>> + * >>>> + * Create a dynamic reference to a base packet starting at a specified >>>> byte >>>> + * offset by prepending a supplied header packet. The resulting reference >>>> + * consists of the unshared header followed by the shared base packet >>>> starting >>>> + * at the specified offset. This shared portion should be regarded as >>>> + * read-only. The storage associated with the shared portion of the >>>> reference >>>> + * is not freed until all references to it are freed, which permits easy >>>> + * multicasting or retransmission processing to be performed. >>> >>> My concerns with this API is what happens when application creates >>> multiple references from multiple offsets of the base packet. In that >>> scenario the read-only status of the shared portion could not be >>> maintained since if different references gives different offset there >>> will be more than one shared portion. >>> >> >> Why would this be a problem? We're relying on an "honor system" here >> since there is no defined enforcement mechanism. The rule is that you >> should only modify the unshared portion of any packet and results are >> undefined if this rule is ignored. That's why we have the >> odp_packet_unshared_len() as well as the odp_packet_has_ref() APIs to >> help applications know whether they should or should not attempt to >> modify a packet given it's handle. > > Let us consider an example, > > pkt_a = odp_packet_ref(pkt_base, 200); > ----- > ----- > ----- > pkt_b = odp_packet_ref(pkt_base, 100); > > In the above case, the read-only portion of pkt_base was from 200 to > end-of-packet during creation of first reference and it is moved to > 100 to end-of-packet during creation of second reference so all the > segment handle of pkt_base previously owned by the application becomes > invalid. >
No, in the above examples the entirety of pkt_base should be treated as read-only from the moment odp_packet_has_ref(pkt_base) > 0 until odp_packet_has_ref(pkt_base) == 0. That's the simplest, cleanest, and most portable rule. During this time odp_packet_unshared_len(pkt_base) will return 0. I don't see a use case for any other interpretation, and any other interpretation is going to encounter a lot of portability issues. Again, this is an "honor system". It's up to the application to observe this convention since we don't want to specify that the implementation has to somehow make pkt_base read only. All ODP says is results are undefined if applications do not observe this rule. Note that for both pkt_a and pkt_b odp_packet_has_ref() is 0 while odp_packet_is_ref() will be 1. That means that pkt_a and pkt_b can be modified. However since after the odp_packet_ref() call odp_packet_unshared_len() is 0 for both, that means that the only way they should be modified would be to call odp_packet_push_head() or odp_packet_extend_head() to put a unique prefix on the shared payload. So if we call odp_packet_push_head(pkt_a, 64);, then odp_packet_unshare_len(pkt_a) == 64, etc. >> >>>> + * >>>> + * Either packet input to this routine may itself be a reference, however >>>> + * individual implementations MAY impose limits on how deeply splices may >>>> be >>>> + * nested and fail the call if those limits are exceeded. >>>> + * >>>> + * The packets used as input to this routine may reside in different >>>> pools, >>>> + * however individual implementations MAY require that both reside in the >>>> same >>>> + * pool and fail the call if this restriction is not observed. >>> >>> Not sure if there is a requirement to support reference with packets >>> from multiple pools. >> >> That's why this is a MAY. We could expose this as a capability or >> simply state that this is not supported, however some implementations >> may be able to support this (e.g., odp-linux has no need to make this >> restriction) and I could see how it could be useful to have header >> templates in their own pool for storage management purposes. > > odp-linux is a special case since it does not use any HW pool manager, > most platforms using HW pool manager might not be able to create a > packet with segments from multiple pools since it will be difficult to > return the segments to respective pools during odp_packet_free(). > packet pool is a limited resource in a system and it might not be > advisable to use a separate pool for header template. That's why this is a MAY. Implementation that don't support this would fail the odp_packet_ref_pkt() call by returning ODP_PACKET_INVALID if the header and packet to be referenced reside in different pools. This is no different than an odp_packet_alloc() call failing because the pool is out of memory, etc. Applications are expected to be able to deal with these sort of situations. Again, if we want we can simply say that both packets must come from the same pool and leave it at that. I have no problem changing that if people feel that a more uniform interpretation is desirable. Note however that many of the other packet routines (odp_packet_copy(), odp_packet_concat()) support multi-pool operation. > >> >>> >>>> + * >>>> + * odp_packet_pool() on the returned reference returns the pool id of the >>>> + * header packet. >>>> + * >>>> + * @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 the shared >>>> reference >>>> + * to begin. Must be in the range 0..odp_packet_len(pkt)-1. >>>> + * >>>> + * @param hdr Handle of the header packet to be prefixed onto the base >>>> + * packet to create the reference. If this is specified as >>>> + * ODP_PACKET_INVALID, this call is equivalent to >>>> + * odp_packet_ref(). The supplied hdr must be distinct from >>>> + * the base pkt and results are undefined if an attempt is >>>> made >>>> + * to create circular references. >>>> + * >>>> + * @return Handle of the reference packet. This may or may not be >>>> the >>>> + * same as the input hdr packet. The caller should only use >>>> this >>>> + * handle since the original hdr packet no longer has any >>>> + * independent existence. >>>> + * >>>> + * @retval ODP_PACKET_INVALID If the reference failed. In this case both >>>> pkt >>>> + * and hdr are unchanged. >>>> + */ >>>> +odp_packet_t odp_packet_ref_pkt(odp_packet_t pkt, uint32_t offset, >>>> + odp_packet_t hdr); >>>> + >>>> +/** >>>> + * Test if a packet is a reference >>>> + * >>>> + * A packet is a reference if it was created by odp_packet_ref() or >>>> + * odp_packet_ref_pkt(). Note that a reference created from another >>>> + * reference is considered a compound reference. Calls on such packets >>>> will >>>> + * result in return values greater than 1. >>>> + * >>>> + * @param pkt Packet handle >>>> + * >>>> + * @retval 0 Packet is not a reference >>>> + * @retval >0 Packet is a reference consisting of N individual packets. >>>> + */ >>>> +int odp_packet_is_ref(odp_packet_t pkt); >>> >>> It is better to keep the return value as 0 or 1. Is it expected to >>> return the number of references? >>> The total number of references created is not interesting and also it >>> is not so easy since references are created at segment level as per >>> this proposal. >>> Application will have to call odp_packet_free() for all the packet >>> handle it is holding. >> >> Since any implementation supporting references needs to have some >> internal notion of a reference count this is just attempting to expose >> that in an implementation-independent manner. This is also why there >> is both the odp_packet_is_ref() and odp_packet_has_ref() APIs defined. > > Again this depends on the implementation, based on the proposal here > the reference count should be maintained per segment since the > reference is being created using an offset, different segments of the > packets could have different references. > My concern is this count is not useful since application already has > packet handles per reference and it has to free all the handles which > was populated using reference APIs. No, a reference count is logically on a per-packet basis because as noted earlier the entire packet should be treated as read only if any reference to it exists. If implementations want to use per-segment reference counts for internal convenience that's fine, but the API is referring to per-packet references. Remember that applications need to be aware of the existence of segments but ODP APIs are designed to manipulate packets, not segments, since it's assumed that the existence of segments is for the implementation's convenience, not the application's. Yes, we do have various odp_packet_seg_xxx() APIs but they're a bit of a sidecar to the other packet APIs and it's unclear how applications would use these in any generally portable manner. > >> >>> >>>> + >>>> +/** >>>> + * Test if a packet has a reference >>>> + * >>>> + * A packet has a reference if a reference was created on it by >>>> + * odp_packet_ref_static(), odp_packet_ref(), or odp_packet_ref_pkt(). >>>> + * >>>> + * @param pkt Packet handle >>>> + * >>>> + * @retval 0 Packet does not have any references >>>> + * @retval >0 Packet has N references based on it >>>> + */ >>>> +int odp_packet_has_ref(odp_packet_t pkt); >>> >>> What is the difference between odp_packet_has_ref() and >>> odp_packet_is_ref()? IMO Once a reference is created there is no >>> difference between the base packet and reference. >> >> Not true. This is discussed (with diagrams) in the User Guide updates >> associated with this patch series. >> >> Consider the call: >> >> pkt_a = odp_packet_ref(pkt_b, 0); >> >> After this call the following conditions hold: >> >> odp_packet_is_ref(pkt_a) == 1 >> odp_packet_is_ref(pkt_b) == 0 >> odp_packet_has_ref(pkt_a) == 0 >> odp_packet_has_ref(pkt_b) == 1 >> >> Now consider the more complex case: >> >> pkt_a = odp_packet_ref(pkt_b, offset1); >> pkt_c = odp_packet_ref(pkt_b, offset2); >> pkt_d = odp_packet_ref(pkt_a, offset3); >> >> Now we have: >> >> odp_packet_is_ref(pkt_a) == 1 >> odp_packet_is_ref(pkt_b) == 0 >> odp_packet_is_ref(pkt_c) == 1 >> odp_packet_is_ref(pkt_d) == 2 >> odp_packet_has_ref(pkt_a) == 1 >> odp_packet_has_ref(pkt_b) == 3 >> odp_packet_has_ref(pkt_c) == 0 >> odp_packet_has_ref(pkt_d) == 0 >> >> Essentially, odp_packet_is_ref() answers the question "How many other >> packets are referenced via this handle"? While odp_packet_has_ref() >> answers the question "How many other handles can be used to reference >> this packet"? > > This information requires lots of unnecessary computation at the > implementation level, references are created by linking multiple > segments together from application point of view there is no > difference between a base packet and a reference both have few shared > segments and have few unique segments. I understand the definition of > this API, my query is the use-case for returning this number of > has_ref and is_ref()? No that's not true since each packet (whether it's a reference or a base packet) has its own metadata (including headroom, user area, etc.). One of the reasons we introduced the odp_packet_ref_static() API is to allow implementations to further optimize this and create the type of references you refer to here when applications assert that they're going to treat the reference as entirely read only (and hence can share metadata with the base packet). You cannot just link segments together and have a valid packet reference since that doesn't cover the unique metadata like headroom, etc. Given the requirement for unique metadata on a per-reference basis, I don't see the implementation of these APIs as problematic. The odp-linux implementation shows just how simple they really are. > > Regards, > Bala Thank you! This is a great dialog. >> >>> >>> Regards, >>> Bala >>>> + >>>> +/* >>>> + * >>>> * Copy >>>> * ******************************************************** >>>> * >>>> -- >>>> 2.7.4 >>>>
