-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
