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. > >>> + * >>> + * 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. > >> >>> + * >>> + * 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. > >> >>> + >>> +/** >>> + * 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()? Regards, Bala > >> >> Regards, >> Bala >>> + >>> +/* >>> + * >>> * Copy >>> * ******************************************************** >>> * >>> -- >>> 2.7.4 >>>
