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
