The problem to solve is: how to enable multiple references to a packet and more generally to a packet segment. It would be better to create new handles, instead of increment a 32bit count. Currently, the ownership of a handle is lost during enq/free/… operations. That semantic is worth maintaining also with multiple references.
odp_packet_t pkt = current_packet; odp_packet_t my_ref; // Create new reference to a packet my_ref = odp_packet_create_ref(pkt); if (odp_packet_ref_count(pkt) > 1) // reference count is now 2 foo(); odp_packet_free(pkt); // After free, user does not own handle ‘pkt’ anymore and cannot it. if (odp_packet_ref_count(my_ref) > 1) // reference count is now 1 foo(); odp_packet_free(my_ref); // Packet is now freed and user does not have any reference to it anymore. Multiple references to entire packet is easy, but may not be too useful without segment level references. Something like this: odp_packet_t pkt = current_packet; odp_packet_t my_ref; odp_packet_seg_t my_seg; odp_packet_seg_t seg = odp_packet_first_seg(pkt); seg = odp_packet_next_seg(pkt, seg); // New reference to the packet from 2nd segment (‘seg’) onwards. my_ref = odp_packet_seg_create_ref(pkt, seg); // Create new first segment my_seg = odp_packet_create_seg(my_ref, 0 /* index of the newly added seg */ ); // Now my_ref refers to the packet that has my_seg as the first segment and //rest of the segments are shared with other references to the packet -Petri From: lng-odp [mailto:lng-odp-boun...@lists.linaro.org] On Behalf Of EXT Bill Fischofer Sent: Wednesday, September 09, 2015 10:53 PM To: Maxim Uvarov Cc: LNG ODP Mailman List Subject: Re: [lng-odp] [API-NEXT PATCH] api: packet reference count support Actually on reflection I don't like adding reference count decrement functionality to odp_packet_free(). The problem is that this is a void function so there's no way to tell after calling it whether the packet was actually freed or not. If it was, the pkt handle is no longer valid and cannot be referenced after the call. So this seems error-prone. If reference counts are being used then they should be decremented beforehand and odp_packet_free() called only when the refcount hits 0. So, for example, this would be an enhancement to odp_pktio_send() which should behave this way. We need to be careful about how refcounts are intended to be used. For example, a number of APIs (odp_packet_add_data(), etc.) are defined such that they MAY return a different pkt (and dispose of their input packet) as part of their operation. What happens if the input pkt had a refcount > 1? If may or may not be OK to just copy over the refcount to the replacement packet. But if the reason for the refcount being > 1 is that the handle is being shared then the sharers might not know about the substitution. This isn't just for SW as it's understood that certain HW functions (e.g., crypto) may give back a different packet than the one passed to it. Need to think through these cases carefully and be sure semantics are agreeable to implementers when refcounts are introduced. On Wed, Sep 9, 2015 at 2:15 PM, Bill Fischofer <bill.fischo...@linaro.org<mailto:bill.fischo...@linaro.org>> wrote: On Wed, Sep 9, 2015 at 6:41 AM, Maxim Uvarov <maxim.uva...@linaro.org<mailto:maxim.uva...@linaro.org>> wrote: Add api for buffer reference count support. Which is useful in case: - multicast support - TCP retransmission - traffic generator odp_packet_free() function should relay on reference count and do not free packet with reference count > 1. Implementation for pktio send() Frees should happen when the reference count hits 0. >1 is the incorrect test (see below) function should be also adjusted to not free packet on sending. If platform can not directly support reference count for packet it should clone packet before send inside it's implementation. Signed-off-by: Maxim Uvarov <maxim.uva...@linaro.org<mailto:maxim.uva...@linaro.org>> --- include/odp/api/packet.h | 45 ++++++++++++++- .../linux-generic/include/odp_buffer_inlines.h | 26 --------- platform/linux-generic/odp_packet.c | 64 +++++++++++++++++++--- 3 files changed, 98 insertions(+), 37 deletions(-) diff --git a/include/odp/api/packet.h b/include/odp/api/packet.h index 5d46b7b..bf0da17 100644 --- a/include/odp/api/packet.h +++ b/include/odp/api/packet.h @@ -62,7 +62,8 @@ extern "C" { * Pool must have been created with ODP_POOL_PACKET type. The * packet is initialized with data pointers and lengths set according to the * specified len, and the default headroom and tailroom length settings. All - * other packet metadata are set to their default values. + * other packet metadata are set to their default values, packet reference count + * set to 1. * * @param pool Pool handle * @param len Packet data length @@ -79,7 +80,8 @@ odp_packet_t odp_packet_alloc(odp_pool_t pool, uint32_t len); /** * Free packet * - * Frees the packet into the buffer pool it was allocated from. + * Decrement packet reference count and free the packet back into the buffer + * pool it was allocated from if reference count reaches 0. * * @param pkt Packet handle */ @@ -125,6 +127,45 @@ odp_packet_t odp_packet_from_event(odp_event_t ev); */ odp_event_t odp_packet_to_event(odp_packet_t pkt); +/** + * Get packet reference count + * + * @param buf Packet handle + * @return Value of packet reference count + */ +uint32_t odp_packet_refcount(odp_packet_t pkt); + +/** + * Set packet reference count + * + * @param pkt Packet handle + * @param val New value of packet reference count + * + * @retval 0 on success + * @retval <0 on failure + */ +int odp_packet_refcount_set(odp_packet_t pkt, uint32_t val); + +/** + * Increment packet reference count on val + * + * @param pkt Packet handle + * @param val Value to add to current packet reference count + * + * @return New value of reference count + */ +uint32_t odp_packet_refcount_inc(odp_packet_t pkt, uint32_t val); + +/** + * Decrement event reference count on val + * + * @param pkt Packet handle + * @param val Value to subtract from current packet reference count + * + * @return New value of reference count + */ +uint32_t odp_packet_refcount_dec(odp_packet_t pkt, uint32_t val); + /* * * Pointers and lengths diff --git a/platform/linux-generic/include/odp_buffer_inlines.h b/platform/linux-generic/include/odp_buffer_inlines.h index 3f4d9fd..da5693d 100644 --- a/platform/linux-generic/include/odp_buffer_inlines.h +++ b/platform/linux-generic/include/odp_buffer_inlines.h @@ -56,32 +56,6 @@ static inline odp_buffer_hdr_t *odp_buf_to_hdr(odp_buffer_t buf) (pool->pool_mdata_addr + (index * ODP_CACHE_LINE_SIZE)); } -static inline uint32_t odp_buffer_refcount(odp_buffer_hdr_t *buf) -{ - return odp_atomic_load_u32(&buf->ref_count); -} - -static inline uint32_t odp_buffer_incr_refcount(odp_buffer_hdr_t *buf, - uint32_t val) -{ - return odp_atomic_fetch_add_u32(&buf->ref_count, val) + val; -} - -static inline uint32_t odp_buffer_decr_refcount(odp_buffer_hdr_t *buf, - uint32_t val) -{ - uint32_t tmp; - - tmp = odp_atomic_fetch_sub_u32(&buf->ref_count, val); - - if (tmp < val) { - odp_atomic_fetch_add_u32(&buf->ref_count, val - tmp); - return 0; - } else { - return tmp - val; - } -} - static inline odp_buffer_hdr_t *validate_buf(odp_buffer_t buf) { odp_buffer_bits_t handle; diff --git a/platform/linux-generic/odp_packet.c b/platform/linux-generic/odp_packet.c index 209a6e6..f86b3f3 100644 --- a/platform/linux-generic/odp_packet.c +++ b/platform/linux-generic/odp_packet.c @@ -28,27 +28,33 @@ odp_packet_t odp_packet_alloc(odp_pool_t pool_hdl, uint32_t len) { pool_entry_t *pool = odp_pool_to_entry(pool_hdl); + odp_packet_t pkt = ODP_PACKET_INVALID; if (pool->s.params.type != ODP_POOL_PACKET) - return ODP_PACKET_INVALID; + return pkt; - /* Handle special case for zero-length packets */ - if (len == 0) { - odp_packet_t pkt = - (odp_packet_t)buffer_alloc(pool_hdl, - pool->s.params.buf.size); + if (!len) { + /* Handle special case for zero-length packets */ + pkt = (odp_packet_t)buffer_alloc(pool_hdl, + pool->s.params.buf.size); if (pkt != ODP_PACKET_INVALID) pull_tail(odp_packet_hdr(pkt), pool->s.params.buf.size); - - return pkt; + } else { + pkt = (odp_packet_t)buffer_alloc(pool_hdl, len); Are you allocating pkt twice here or is this a diff artifact? The patch is unclear as posted. } - return (odp_packet_t)buffer_alloc(pool_hdl, len); + if (pkt != ODP_PACKET_INVALID) + odp_packet_refcount_set(pkt, 1); + + return pkt; } void odp_packet_free(odp_packet_t pkt) { + if (odp_packet_refcount_dec(pkt, 1) > 1) Correct test is if (odp_packet_refcount_dec(pkt, 1)) That skips if the refcount > 0, not > 1. + return; + odp_buffer_free((odp_buffer_t)pkt); } @@ -85,6 +91,46 @@ odp_event_t odp_packet_to_event(odp_packet_t pkt) return (odp_event_t)pkt; } +uint32_t odp_packet_refcount(odp_packet_t pkt) +{ + odp_buffer_t buf = _odp_packet_to_buffer(pkt); + odp_buffer_hdr_t *hdr = odp_buf_to_hdr(buf); + + return odp_atomic_load_u32(&hdr->ref_count); +} + +int odp_packet_refcount_set(odp_packet_t pkt, uint32_t val) +{ + odp_buffer_t buf = _odp_packet_to_buffer(pkt); + odp_buffer_hdr_t *hdr = odp_buf_to_hdr(buf); + + odp_atomic_store_u32(&hdr->ref_count, val); + return 0; +} + +uint32_t odp_packet_refcount_inc(odp_packet_t pkt, uint32_t val) +{ + odp_buffer_t buf = _odp_packet_to_buffer(pkt); + odp_buffer_hdr_t *hdr = odp_buf_to_hdr(buf); + + return odp_atomic_fetch_add_u32(&hdr->ref_count, val) + val; +} + +uint32_t odp_packet_refcount_dec(odp_packet_t pkt, uint32_t val) +{ + uint32_t tmp; + odp_buffer_t buf = _odp_packet_to_buffer(pkt); + odp_buffer_hdr_t *hdr = odp_buf_to_hdr(buf); + + tmp = odp_atomic_fetch_sub_u32(&hdr->ref_count, val); + if (tmp < val) { + odp_atomic_fetch_add_u32(&hdr->ref_count, val - tmp); + return 0; + } else { + return tmp - val; + } +} + /* * * Pointers and lengths -- 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
_______________________________________________ lng-odp mailing list lng-odp@lists.linaro.org https://lists.linaro.org/mailman/listinfo/lng-odp