[API-NEXT] api: packet reference count support

Message ID	1441798909-5379-1-git-send-email-maxim.uvarov@linaro.org
State	New
Headers	show Return-Path: <patchwork-forward+bncBDX25FHYWQNBBEFWYCXQKGQEXQ2BH2Q@linaro.org> Received-SPF: pass (google.com: domain of patch+caf_=patchwork-forward=linaro.org@linaro.org designates 209.85.215.46 as permitted sender) client-ip=209.85.215.46; Received-SPF: pass (google.com: domain of lng-odp-bounces@lists.linaro.org designates 54.225.227.206 as permitted sender) client-ip=54.225.227.206; From: Maxim Uvarov <maxim.uvarov@linaro.org> To: lng-odp@lists.linaro.org Date: Wed, 9 Sep 2015 14:41:49 +0300 Message-Id: <1441798909-5379-1-git-send-email-maxim.uvarov@linaro.org> Subject: [lng-odp] [API-NEXT PATCH] api: packet reference count support Precedence: list MIME-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: base64 Errors-To: lng-odp-bounces@lists.linaro.org Sender: "lng-odp" <lng-odp-bounces@lists.linaro.org> Mailing-list: list patchwork-forward@linaro.org; contact patchwork-forward+owners@linaro.org

Maxim Uvarov Sept. 9, 2015, 11:41 a.m. UTC

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()
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.uvarov@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(-)

Ola Liljedahl Sept. 9, 2015, 2:37 p.m. UTC | #1

On 9 September 2015 at 13:41, Maxim Uvarov <maxim.uvarov@linaro.org> wrote:

> Add api for buffer reference count support. Which is useful in case:
>  - multicast support
>  - TCP retransmission
>  - traffic generator
>
I can understand that a traffic generator wants to reuse existing
initialised packets but doesn't it need to know when a packet has been
transmitted and it is safe to reuse and modify the buffer? And we currently
do not have any support for getting such notifications. One could check the
reference counter of packets previously enqueued for transmission but I
think it would be better to (conditionally) return transmitted packets to
the application. This creates a natural flow control between application
and NIC.


> odp_packet_free() function should relay on reference count and do not
>
relay -> rely?


> free packet with reference count > 1. Implementation for pktio send()
> function should be also adjusted to not free packet on sending.

pktio_send() should "free" the packet after transmission, i.e. do the
equivalent to calling odp_packet_free(). Whether the packet is actually
recycled (returned to the pool) depends on the value of the reference
counter. By "freeing" packet, we always mean that the reference counter is
decremented and the buffer is recycled only when the refcnt reaches zero.


> 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.uvarov@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);
>         }
>
> -       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)
> +               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
> https://lists.linaro.org/mailman/listinfo/lng-odp
>

Bill Fischofer Sept. 9, 2015, 7:15 p.m. UTC | #2

On Wed, Sep 9, 2015 at 6:41 AM, Maxim Uvarov <maxim.uvarov@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.uvarov@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
> https://lists.linaro.org/mailman/listinfo/lng-odp
>

Bill Fischofer Sept. 9, 2015, 7:52 p.m. UTC | #3

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.fischofer@linaro.org>
wrote:

>
>
> On Wed, Sep 9, 2015 at 6:41 AM, Maxim Uvarov <maxim.uvarov@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.uvarov@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
>> https://lists.linaro.org/mailman/listinfo/lng-odp
>>
>
>

Ivan Khoronzhuk Sept. 9, 2015, 7:58 p.m. UTC | #4

On 09.09.15 22:15, Bill Fischofer wrote:
>
>
> On Wed, Sep 9, 2015 at 6:41 AM, Maxim Uvarov <maxim.uvarov@linaro.org <mailto:maxim.uvarov@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.uvarov@linaro.org <mailto:maxim.uvarov@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.
It's also unclear when and where packet finally has to be really deleted.
What if odp_packet_refcount_dec() leads to 0, but odp_packet_free() was called sometime before?
Who will delete the packet?
Who should care about it, implementation? Then maybe there is reason to add some more
smart functions like get/put functions that use basic refcount inc/dec and packet_free....

>
>     +               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)
It's associated with decrement -1, not val...like inc +1...also

>     +{
>     +       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
>

Maxim Uvarov Sept. 10, 2015, 7:40 a.m. UTC | #5

On 09/09/15 22:15, Bill Fischofer wrote:
>
>
> On Wed, Sep 9, 2015 at 6:41 AM, Maxim Uvarov <maxim.uvarov@linaro.org 
> <mailto:maxim.uvarov@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)
>

ok. I thought what happens in case if refcount is 3 for example. Then 
used calls decrement on 5.
refcount is unsigned and in my understanding that should be error. Or it 
might be valid situation,
refcount should be 0 and packet should be freed. I think it's better to 
keep that as error. But I agree
that it's better to say free if refcount is 0.

>     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.uvarov@linaro.org
>     <mailto:maxim.uvarov@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.

only one alloc:
if (!len) { alloc} else {alloc}

>             }
>
>     -       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))
>
agree.
> 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
>
>

Maxim Uvarov Sept. 10, 2015, 8:48 a.m. UTC | #6

On 09/09/15 22:52, Bill Fischofer wrote:
> 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.
>

yes, I understand that.  odp_pktio_send() function references to the 
"pktio driver" and odp_packet_free() called inside that driver.
Reference count alloc/free are parts of packet management and put ref 
count set up to packet and free to packet i/o  for me looks not
very logical.

Might be we need one more function, like:
odp_packet_free() - will decrement count and free if 0.
odp_packet_destroy() - will free packet without accounting refcount.

> 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.
>
Sure. There should be some restrictions where and how use that refcount. 
In my understanding increasing refcount should be done somewhere near 
send(), packet should not be modified by other thread.

odp_packet_add_data() works well if refcount decremented in 
odp_packet_free(), in my understanding. New buffer allocated and 
refcount for old one decreased. That looks logical. If application needs 
copy refcounts then it can call:
refcount_set(new_pkt, refcount(pkt));
refcont_set(pkt, 0);
odp_packet_free(pkt);

But I agree there might be bunch of places where we need to account 
refcounts or disable their usage.

Thanks,
Maxim.
> On Wed, Sep 9, 2015 at 2:15 PM, Bill Fischofer 
> <bill.fischofer@linaro.org <mailto:bill.fischofer@linaro.org>> wrote:
>
>
>
>     On Wed, Sep 9, 2015 at 6:41 AM, Maxim Uvarov
>     <maxim.uvarov@linaro.org <mailto:maxim.uvarov@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)
>
ok.
>
>         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.uvarov@linaro.org
>         <mailto:maxim.uvarov@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
>
>
>

Maxim Uvarov Sept. 10, 2015, 9:09 a.m. UTC | #7

On 09/09/15 22:58, Ivan Khoronzhuk wrote:
> t's also unclear when and where packet finally has to be really deleted.
> What if odp_packet_refcount_dec() leads to 0, but odp_packet_free() 
> was called sometime before?

There should be no packets with refcount 0. As soon as it became 0 it's 
freed. It's done in odp_packet_free(pkt),
so that you can not more use pkt handler provided to free function.

The other deal if that odp_packet_free(pkt) should be not void.

Something like:
pkt = odp_packet_alloc();
odp_packet_refcount_set(pkt, 3);

queue_create();

then it' supposed that schedule will return reference to that packet 3 
times:

while(1) {
     pkt = evt = odp_schedule();
     odp_packet_free(pkt);
}

Any case if you wrongly account reference count should be application 
error and trap application
execution.
>
> Who will delete the packet?

odp_packet_free() if refcount is 0. No?

>
> Who should care about it, implementation? Then maybe there is reason 
> to add some more
> smart functions like get/put functions that use basic refcount inc/dec 
> and packet_free....
>
Yes, each implementation have it's own way to support refcounts.

I think that inc on x and dec on y is more universal than inc on 1 and 
dec on 1.

>>
>>
>>     +
>>     +uint32_t odp_packet_refcount_dec(odp_packet_t pkt, uint32_t val)
> It's associated with decrement -1, not val...like inc +1...also

Sorry, did not understand that comment. Is that about naming? Should be 
add x, sub y? Because inc/dec supposed +-1?

Maxim.
>
>>     +{
>>     +       uint32_t tmp;

Ivan Khoronzhuk Sept. 10, 2015, 10:20 a.m. UTC | #8

Maxim,

On 10.09.15 12:09, Maxim Uvarov wrote:
> On 09/09/15 22:58, Ivan Khoronzhuk wrote:
>> t's also unclear when and where packet finally has to be really deleted.
>> What if odp_packet_refcount_dec() leads to 0, but odp_packet_free() was called sometime before?
>
> There should be no packets with refcount 0. As soon as it became 0 it's freed. It's done in odp_packet_free(pkt),
> so that you can not more use pkt handler provided to free function.
>
> The other deal if that odp_packet_free(pkt) should be not void.
>
> Something like:
> pkt = odp_packet_alloc();
> odp_packet_refcount_set(pkt, 3);
>
> queue_create();
>
> then it' supposed that schedule will return reference to that packet 3 times:
>
> while(1) {
>      pkt = evt = odp_schedule();
>      odp_packet_free(pkt);
> }
>
> Any case if you wrongly account reference count should be application error and trap application
> execution.
>>
>> Who will delete the packet?
>
> odp_packet_free() if refcount is 0. No?
I just meant that packet_free should free the packet w/o any condition.
The same, dec and inc simply dec and inc, they don't do any deletion.
And you have put and get, get() simply inc counter and put() dec counter and free packet if it's 0.
But as I see from Petri reply the task is a little differ.

>
>>
>> Who should care about it, implementation? Then maybe there is reason to add some more
>> smart functions like get/put functions that use basic refcount inc/dec and packet_free....
>>
> Yes, each implementation have it's own way to support refcounts.
>
> I think that inc on x and dec on y is more universal than inc on 1 and dec on 1.
Not exactly but close.
Maybe it's personal, but increment it's unary operation opposite to decrement. Usually it's 1.
It's not supposed to pass argument. You should always dec and inc the same value depending of
object you are working with. Like x++ is plus one for value, for pointer it's +byte size .. etc...

>
>
>>>
>>>
>>>     +
>>>     +uint32_t odp_packet_refcount_dec(odp_packet_t pkt, uint32_t val)
>> It's associated with decrement -1, not val...like inc +1...also
>
> Sorry, did not understand that comment. Is that about naming? Should be add x, sub y? Because inc/dec supposed +-1?
>
> Maxim.
>>
>>>     +{
>>>     +       uint32_t tmp;
>

Maxim Uvarov Sept. 10, 2015, 11:19 a.m. UTC | #9

On 09/10/15 12:00, Savolainen, Petri (Nokia - FI/Espoo) wrote:
>
> 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.
>

I think that uses case is not close to real life. If you do in one thread:
pkt = <get packet or alloc>;
odp_packet_refcount_set(pkt, 2);
foo(pkt);
odp_packet_free(pkt); <- ref is 1
foo(pkt);
odp_packet_free(pkt); <- ref 0, free

Then you know what pkt is on each line. Instead of having 2 reference as 
in your example my_ref and pkt, you
use just one pkt.


Now in case of several threads:

pkt = <get packet or alloc>;
odp_packet_refcount_set(pkt, 2);
<enqueue it to scheduler>

threads code:
     pkt = odp_schedule();
     foo(pkt);
     odp_packet_free(pkt);

In that case you don't need to take care about real pkt and it's ref. 
Just process packets returned by schedule() and resend them or free.



> 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 2^nd 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
>

What is use case for support segmentations refcounts? Do we have big 
overhead if we keep hole packet, not only it's segment?


Maxim.

> -Petri
>
> *From:*lng-odp [mailto:lng-odp-bounces@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.fischofer@linaro.org <mailto:bill.fischofer@linaro.org>> wrote:
>
> On Wed, Sep 9, 2015 at 6:41 AM, Maxim Uvarov <maxim.uvarov@linaro.org 
> <mailto:maxim.uvarov@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.uvarov@linaro.org
>     <mailto:maxim.uvarov@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
>

Ivan Khoronzhuk Sept. 10, 2015, 11:40 a.m. UTC | #10

On 10.09.15 14:19, Maxim Uvarov wrote:
> On 09/10/15 12:00, Savolainen, Petri (Nokia - FI/Espoo) wrote:
>>
>> 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.
>>
>
> I think that uses case is not close to real life. If you do in one thread:
> pkt = <get packet or alloc>;
> odp_packet_refcount_set(pkt, 2);
> foo(pkt);
> odp_packet_free(pkt); <- ref is 1
> foo(pkt);
> odp_packet_free(pkt); <- ref 0, free
>
> Then you know what pkt is on each line. Instead of having 2 reference as in your example my_ref and pkt, you
> use just one pkt.
>
>
> Now in case of several threads:
>
> pkt = <get packet or alloc>;
> odp_packet_refcount_set(pkt, 2);
> <enqueue it to scheduler>
>
> threads code:
>      pkt = odp_schedule();
>      foo(pkt);
>      odp_packet_free(pkt);
>
> In that case you don't need to take care about real pkt and it's ref. Just process packets returned by schedule() and resend them or free.
>

Maybe it's not from life, but why consumer and producer should know about some refcounters?
As for me it should be hidden from user. You never in start point can predict how much
ounsumers of the packet you need, so odp_packet_refcount_set() is not needed at all.
It's set to 0 at packet creation stage and increased each time +1 (by some get()),
when consumer needs to save it from deletion. When packet is not needed any more - call put.
Will it be freed or not it's not the deal of the caller.

But if packet is not copied, how do you know that this packet was not changed or it's context was
not changed by some user process? Maybe better just copy it....need more usecases..

>
>
>> 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 2^nd 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
>>
>
> What is use case for support segmentations refcounts? Do we have big overhead if we keep hole packet, not only it's segment?
>
>
> Maxim.
>
>> -Petri
>>
>> *From:*lng-odp [mailto:lng-odp-bounces@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.fischofer@linaro.org <mailto:bill.fischofer@linaro.org>> wrote:
>>
>> On Wed, Sep 9, 2015 at 6:41 AM, Maxim Uvarov <maxim.uvarov@linaro.org <mailto:maxim.uvarov@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.uvarov@linaro.org
>>     <mailto:maxim.uvarov@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

Savolainen, Petri (Nokia - FI/Espoo) Sept. 10, 2015, 12:54 p.m. UTC | #11

> -----Original Message-----

> From: EXT Maxim Uvarov [mailto:maxim.uvarov@linaro.org]

> Sent: Thursday, September 10, 2015 2:20 PM

> To: Savolainen, Petri (Nokia - FI/Espoo); EXT Bill Fischofer

> Cc: LNG ODP Mailman List

> Subject: Re: [lng-odp] [API-NEXT PATCH] api: packet reference count

> support

> 

> On 09/10/15 12:00, Savolainen, Petri (Nokia - FI/Espoo) wrote:

> >

> > 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.

> >

> 

> I think that uses case is not close to real life. If you do in one

> thread:

> pkt = <get packet or alloc>;

> odp_packet_refcount_set(pkt, 2);

> foo(pkt);

> odp_packet_free(pkt); <- ref is 1

> foo(pkt);

> odp_packet_free(pkt); <- ref 0, free

> 

> Then you know what pkt is on each line. Instead of having 2 reference

> as

> in your example my_ref and pkt, you

> use just one pkt.



The point of the example is clear ownership of a reference to the packet.

These are *bugs* currently, and API should stay clear on ownership issues also multiple references:

// double free a packet (handle or reference)
odp_packet_free(pkt);
odp_packet_free(pkt);


// double enqueue a packet (handle or reference)
ev = odp_packet_from_event(pkt);
odp_queue_enq(queue1, ev);
odp_queue_enq(queue2, ev);


With two different handles (references) it's clear how owns the handle and implementation can either return the same or different value as the handle.

my_ref = odp_packet_create_ref(pkt);

ev1 = odp_packet_from_event(pkt);
odp_queue_enq(queue1, ev1);

ev2 = odp_packet_from_event(my_ref);
odp_queue_enq(queue2, ev2);




 
> 

> 

> Now in case of several threads:

> 

> pkt = <get packet or alloc>;

> odp_packet_refcount_set(pkt, 2);

> <enqueue it to scheduler>

> 

> threads code:

>      pkt = odp_schedule();

>      foo(pkt);

>      odp_packet_free(pkt);

> 

> In that case you don't need to take care about real pkt and it's ref.

> Just process packets returned by schedule() and resend them or free.

> 


The difference is reference ownership, and usage of one vs. two (potentially different) handles. 

Thread 1:				Thread2:

// send to thread 2
queue_enq(queue, pkt);
					pkt = schedule();
// access reference
foo(pkt);				bar(pkt);



Thread 1:				Thread2:

pkt2 = create_ref(pkt1);

// send to thread 2
queue_enq(queue, pkt1);
					pkt1 = schedule();
// access reference
foo(pkt2);				bar(pkt1);


> 

> 

> > 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 2^nd 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

> >

> 

> What is use case for support segmentations refcounts? Do we have big

> overhead if we keep hole packet, not only it's segment?



For example, multicast of a packet (payload) with multiple different headers. There's a big difference if payload is copied and stored multiple times vs. only headers are multiplied.

-Petri

> 

> 

> Maxim.

> 

> > -Petri

> >

> > *From:*lng-odp [mailto:lng-odp-bounces@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.fischofer@linaro.org <mailto:bill.fischofer@linaro.org>> wrote:

> >

> > On Wed, Sep 9, 2015 at 6:41 AM, Maxim Uvarov <maxim.uvarov@linaro.org

> > <mailto:maxim.uvarov@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.uvarov@linaro.org

> >     <mailto:maxim.uvarov@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

> >

Maxim Uvarov Sept. 10, 2015, 4:10 p.m. UTC | #12

On 09/10/15 15:54, Savolainen, Petri (Nokia - FI/Espoo) wrote:
>
>> -----Original Message-----
>> From: EXT Maxim Uvarov [mailto:maxim.uvarov@linaro.org]
>> Sent: Thursday, September 10, 2015 2:20 PM
>> To: Savolainen, Petri (Nokia - FI/Espoo); EXT Bill Fischofer
>> Cc: LNG ODP Mailman List
>> Subject: Re: [lng-odp] [API-NEXT PATCH] api: packet reference count
>> support
>>
>> On 09/10/15 12:00, Savolainen, Petri (Nokia - FI/Espoo) wrote:
>>> 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.
>> I think that uses case is not close to real life. If you do in one
>> thread:
>> pkt = <get packet or alloc>;
>> odp_packet_refcount_set(pkt, 2);
>> foo(pkt);
>> odp_packet_free(pkt); <- ref is 1
>> foo(pkt);
>> odp_packet_free(pkt); <- ref 0, free
>>
>> Then you know what pkt is on each line. Instead of having 2 reference
>> as
>> in your example my_ref and pkt, you
>> use just one pkt.
>
> The point of the example is clear ownership of a reference to the packet.
>
> These are *bugs* currently, and API should stay clear on ownership issues also multiple references:
>
> // double free a packet (handle or reference)
> odp_packet_free(pkt);
> odp_packet_free(pkt);
>
>
> // double enqueue a packet (handle or reference)
> ev = odp_packet_from_event(pkt);
> odp_queue_enq(queue1, ev);
> odp_queue_enq(queue2, ev);
>
>
> With two different handles (references) it's clear how owns the handle and implementation can either return the same or different value as the handle.
>
> my_ref = odp_packet_create_ref(pkt);
>
> ev1 = odp_packet_from_event(pkt);
> odp_queue_enq(queue1, ev1);
>
> ev2 = odp_packet_from_event(my_ref);
> odp_queue_enq(queue2, ev2);
>
In my case it's easy to check,  inside odp_packet_free(pkt):
if (!odp_packet_refcount(pkt))
     ODP_ABORT("illegal free");

That will capture bugs with double enqueue when you will try to send 
that packets.


If your case we should maintain somewhere array of packet handles and 
packet headers,
with unclear size depending how match reference we can have. And handles 
should not
overlap with current packet handles. That looks more complicated, right?

>
>
>   
>>
>> Now in case of several threads:
>>
>> pkt = <get packet or alloc>;
>> odp_packet_refcount_set(pkt, 2);
>> <enqueue it to scheduler>
>>
>> threads code:
>>       pkt = odp_schedule();
>>       foo(pkt);
>>       odp_packet_free(pkt);
>>
>> In that case you don't need to take care about real pkt and it's ref.
>> Just process packets returned by schedule() and resend them or free.
>>
> The difference is reference ownership, and usage of one vs. two (potentially different) handles.
>
> Thread 1:				Thread2:
>
> // send to thread 2
> queue_enq(queue, pkt);
> 					pkt = schedule();
> // access reference
> foo(pkt);				bar(pkt);
>
>
>
> Thread 1:				Thread2:
>
> pkt2 = create_ref(pkt1);
>
> // send to thread 2
> queue_enq(queue, pkt1);
> 					pkt1 = schedule();
> // access reference
> foo(pkt2);				bar(pkt1);

That usage is clear. Not clear how to maintain pkt2 table of handlers in 
that case. And how fast
look up in that table will be and what is overhead for that lookup.

Maxim.

>
>>
>>> 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 2^nd 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
>>>
>> What is use case for support segmentations refcounts? Do we have big
>> overhead if we keep hole packet, not only it's segment?
>
> For example, multicast of a packet (payload) with multiple different headers. There's a big difference if payload is copied and stored multiple times vs. only headers are multiplied.
>
> -Petri
>
>>
>> Maxim.
>>
>>> -Petri
>>>
>>> *From:*lng-odp [mailto:lng-odp-bounces@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.fischofer@linaro.org <mailto:bill.fischofer@linaro.org>> wrote:
>>>
>>> On Wed, Sep 9, 2015 at 6:41 AM, Maxim Uvarov <maxim.uvarov@linaro.org
>>> <mailto:maxim.uvarov@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.uvarov@linaro.org
>>>      <mailto:maxim.uvarov@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
>>>

Savolainen, Petri (Nokia - FI/Espoo) Sept. 11, 2015, 7:48 a.m. UTC | #13

> -----Original Message-----

> From: EXT Maxim Uvarov [mailto:maxim.uvarov@linaro.org]

> Sent: Thursday, September 10, 2015 7:11 PM

> To: Savolainen, Petri (Nokia - FI/Espoo); EXT Bill Fischofer

> Cc: LNG ODP Mailman List

> Subject: Re: [lng-odp] [API-NEXT PATCH] api: packet reference count

> support

> 

> On 09/10/15 15:54, Savolainen, Petri (Nokia - FI/Espoo) wrote:

> >

> >> -----Original Message-----

> >> From: EXT Maxim Uvarov [mailto:maxim.uvarov@linaro.org]

> >> Sent: Thursday, September 10, 2015 2:20 PM

> >> To: Savolainen, Petri (Nokia - FI/Espoo); EXT Bill Fischofer

> >> Cc: LNG ODP Mailman List

> >> Subject: Re: [lng-odp] [API-NEXT PATCH] api: packet reference count

> >> support

> >>

> >> On 09/10/15 12:00, Savolainen, Petri (Nokia - FI/Espoo) wrote:

> >>> 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.

> >> I think that uses case is not close to real life. If you do in one

> >> thread:

> >> pkt = <get packet or alloc>;

> >> odp_packet_refcount_set(pkt, 2);

> >> foo(pkt);

> >> odp_packet_free(pkt); <- ref is 1

> >> foo(pkt);

> >> odp_packet_free(pkt); <- ref 0, free

> >>

> >> Then you know what pkt is on each line. Instead of having 2

> reference

> >> as

> >> in your example my_ref and pkt, you

> >> use just one pkt.

> >

> > The point of the example is clear ownership of a reference to the

> packet.

> >

> > These are *bugs* currently, and API should stay clear on ownership

> issues also multiple references:

> >

> > // double free a packet (handle or reference)

> > odp_packet_free(pkt);

> > odp_packet_free(pkt);

> >

> >

> > // double enqueue a packet (handle or reference)

> > ev = odp_packet_from_event(pkt);

> > odp_queue_enq(queue1, ev);

> > odp_queue_enq(queue2, ev);

> >

> >

> > With two different handles (references) it's clear how owns the

> handle and implementation can either return the same or different value

> as the handle.

> >

> > my_ref = odp_packet_create_ref(pkt);

> >

> > ev1 = odp_packet_from_event(pkt);

> > odp_queue_enq(queue1, ev1);

> >

> > ev2 = odp_packet_from_event(my_ref);

> > odp_queue_enq(queue2, ev2);

> >

> In my case it's easy to check,  inside odp_packet_free(pkt):

> if (!odp_packet_refcount(pkt))

>      ODP_ABORT("illegal free");

> 

> That will capture bugs with double enqueue when you will try to send

> that packets.

> 

> 

> If your case we should maintain somewhere array of packet handles and

> packet headers,

> with unclear size depending how match reference we can have. And

> handles

> should not

> overlap with current packet handles. That looks more complicated,

> right?

> 


Just like any other ODP API, this is a question of API definition vs. freedom of implementation. API spec defines two handles (one per reference) and thus user must act as if those two handles are unique, BUT the implementation can choose if it reuses the handle value or not. 

For example,

typedef odp_packet_t soc_packet_descriptor_t *;


odp_packet_t odp_packet_create_ref(odp_packet_t pkt)
{
	soc_packet_descriptor_t *desc = (soc_packet_descriptor_t *) pkt;

	desc->ref_count++;
	return pkt;
}

OR

odp_packet_t odp_packet_create_ref(odp_packet_t pkt)
{
	soc_packet_descriptor_t *desc = ((soc_packet_descriptor_t *) pkt) & PTR_MASK;

	desc->ref_count++;
	return (desc | THIS_IS_A_REFERENCE_TO_A_PACKET_NOT_THE_ORIGINAL_PACKET_DESC);
}



> >

> >

> >

> >>

> >> Now in case of several threads:

> >>

> >> pkt = <get packet or alloc>;

> >> odp_packet_refcount_set(pkt, 2);

> >> <enqueue it to scheduler>

> >>

> >> threads code:

> >>       pkt = odp_schedule();

> >>       foo(pkt);

> >>       odp_packet_free(pkt);

> >>

> >> In that case you don't need to take care about real pkt and it's

> ref.

> >> Just process packets returned by schedule() and resend them or free.

> >>

> > The difference is reference ownership, and usage of one vs. two

> (potentially different) handles.

> >

> > Thread 1:				Thread2:

> >

> > // send to thread 2

> > queue_enq(queue, pkt);

> > 					pkt = schedule();

> > // access reference

> > foo(pkt);				bar(pkt);

> >

> >

> >

> > Thread 1:				Thread2:

> >

> > pkt2 = create_ref(pkt1);

> >

> > // send to thread 2

> > queue_enq(queue, pkt1);

> > 					pkt1 = schedule();

> > // access reference

> > foo(pkt2);				bar(pkt1);

> 

> That usage is clear. Not clear how to maintain pkt2 table of handlers

> in

> that case. And how fast

> look up in that table will be and what is overhead for that lookup.


No need to look up. No need to explicit reference counting by application. See above.

-Petri

Maxim Uvarov Sept. 11, 2015, 8:30 a.m. UTC | #14

On 09/11/15 10:48, Savolainen, Petri (Nokia - FI/Espoo) wrote:
>
>> -----Original Message-----
>> From: EXT Maxim Uvarov [mailto:maxim.uvarov@linaro.org]
>> Sent: Thursday, September 10, 2015 7:11 PM
>> To: Savolainen, Petri (Nokia - FI/Espoo); EXT Bill Fischofer
>> Cc: LNG ODP Mailman List
>> Subject: Re: [lng-odp] [API-NEXT PATCH] api: packet reference count
>> support
>>
>> On 09/10/15 15:54, Savolainen, Petri (Nokia - FI/Espoo) wrote:
>>>> -----Original Message-----
>>>> From: EXT Maxim Uvarov [mailto:maxim.uvarov@linaro.org]
>>>> Sent: Thursday, September 10, 2015 2:20 PM
>>>> To: Savolainen, Petri (Nokia - FI/Espoo); EXT Bill Fischofer
>>>> Cc: LNG ODP Mailman List
>>>> Subject: Re: [lng-odp] [API-NEXT PATCH] api: packet reference count
>>>> support
>>>>
>>>> On 09/10/15 12:00, Savolainen, Petri (Nokia - FI/Espoo) wrote:
>>>>> 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.
>>>> I think that uses case is not close to real life. If you do in one
>>>> thread:
>>>> pkt = <get packet or alloc>;
>>>> odp_packet_refcount_set(pkt, 2);
>>>> foo(pkt);
>>>> odp_packet_free(pkt); <- ref is 1
>>>> foo(pkt);
>>>> odp_packet_free(pkt); <- ref 0, free
>>>>
>>>> Then you know what pkt is on each line. Instead of having 2
>> reference
>>>> as
>>>> in your example my_ref and pkt, you
>>>> use just one pkt.
>>> The point of the example is clear ownership of a reference to the
>> packet.
>>> These are *bugs* currently, and API should stay clear on ownership
>> issues also multiple references:
>>> // double free a packet (handle or reference)
>>> odp_packet_free(pkt);
>>> odp_packet_free(pkt);
>>>
>>>
>>> // double enqueue a packet (handle or reference)
>>> ev = odp_packet_from_event(pkt);
>>> odp_queue_enq(queue1, ev);
>>> odp_queue_enq(queue2, ev);
>>>
>>>
>>> With two different handles (references) it's clear how owns the
>> handle and implementation can either return the same or different value
>> as the handle.
>>> my_ref = odp_packet_create_ref(pkt);
>>>
>>> ev1 = odp_packet_from_event(pkt);
>>> odp_queue_enq(queue1, ev1);
>>>
>>> ev2 = odp_packet_from_event(my_ref);
>>> odp_queue_enq(queue2, ev2);
>>>
>> In my case it's easy to check,  inside odp_packet_free(pkt):
>> if (!odp_packet_refcount(pkt))
>>       ODP_ABORT("illegal free");
>>
>> That will capture bugs with double enqueue when you will try to send
>> that packets.
>>
>>
>> If your case we should maintain somewhere array of packet handles and
>> packet headers,
>> with unclear size depending how match reference we can have. And
>> handles
>> should not
>> overlap with current packet handles. That looks more complicated,
>> right?
>>
> Just like any other ODP API, this is a question of API definition vs. freedom of implementation. API spec defines two handles (one per reference) and thus user must act as if those two handles are unique, BUT the implementation can choose if it reuses the handle value or not.
>
> For example,
>
> typedef odp_packet_t soc_packet_descriptor_t *;
>
>
> odp_packet_t odp_packet_create_ref(odp_packet_t pkt)
> {
> 	soc_packet_descriptor_t *desc = (soc_packet_descriptor_t *) pkt;
>
> 	desc->ref_count++;
> 	return pkt;
> }

that variant is clear, but pkt on input and output is the same - it's 
pointer to desc somewhere.
But it's the same what I proposed.

> OR
>
> odp_packet_t odp_packet_create_ref(odp_packet_t pkt)
> {
> 	soc_packet_descriptor_t *desc = ((soc_packet_descriptor_t *) pkt) & PTR_MASK;
>
> 	desc->ref_count++;
> 	return (desc | THIS_IS_A_REFERENCE_TO_A_PACKET_NOT_THE_ORIGINAL_PACKET_DESC);
> }
That variant modifies desc pointer. And there might be possible issues:

1. it might not portable due to unable to restore original pointer to 
descr (if all bits of pointer address are used.).
2. I assume some hw schedulers can unable to work with such modified 
pointers. And it will be hard to implement in hw.
3. On every function which works with packet you need clear 
REFERENCE_BIT before accessing packet (access to *desc),
which is additional overhead.


Maxim.


>
>
>>>
>>>
>>>> Now in case of several threads:
>>>>
>>>> pkt = <get packet or alloc>;
>>>> odp_packet_refcount_set(pkt, 2);
>>>> <enqueue it to scheduler>
>>>>
>>>> threads code:
>>>>        pkt = odp_schedule();
>>>>        foo(pkt);
>>>>        odp_packet_free(pkt);
>>>>
>>>> In that case you don't need to take care about real pkt and it's
>> ref.
>>>> Just process packets returned by schedule() and resend them or free.
>>>>
>>> The difference is reference ownership, and usage of one vs. two
>> (potentially different) handles.
>>> Thread 1:				Thread2:
>>>
>>> // send to thread 2
>>> queue_enq(queue, pkt);
>>> 					pkt = schedule();
>>> // access reference
>>> foo(pkt);				bar(pkt);
>>>
>>>
>>>
>>> Thread 1:				Thread2:
>>>
>>> pkt2 = create_ref(pkt1);
>>>
>>> // send to thread 2
>>> queue_enq(queue, pkt1);
>>> 					pkt1 = schedule();
>>> // access reference
>>> foo(pkt2);				bar(pkt1);
>> That usage is clear. Not clear how to maintain pkt2 table of handlers
>> in
>> that case. And how fast
>> look up in that table will be and what is overhead for that lookup.
> No need to look up. No need to explicit reference counting by application. See above.
>
> -Petri

Savolainen, Petri (Nokia - FI/Espoo) Sept. 11, 2015, 9:32 a.m. UTC | #15

> -----Original Message-----

> From: EXT Maxim Uvarov [mailto:maxim.uvarov@linaro.org]

> Sent: Friday, September 11, 2015 11:30 AM

> To: Savolainen, Petri (Nokia - FI/Espoo); EXT Bill Fischofer

> Cc: LNG ODP Mailman List

> Subject: Re: [lng-odp] [API-NEXT PATCH] api: packet reference count

> support

> 

> On 09/11/15 10:48, Savolainen, Petri (Nokia - FI/Espoo) wrote:

> >

> >> -----Original Message-----

> >> From: EXT Maxim Uvarov [mailto:maxim.uvarov@linaro.org]

> >> Sent: Thursday, September 10, 2015 7:11 PM

> >> To: Savolainen, Petri (Nokia - FI/Espoo); EXT Bill Fischofer

> >> Cc: LNG ODP Mailman List

> >> Subject: Re: [lng-odp] [API-NEXT PATCH] api: packet reference count

> >> support

> >>

> >> On 09/10/15 15:54, Savolainen, Petri (Nokia - FI/Espoo) wrote:

> >>>> -----Original Message-----

> >>>> From: EXT Maxim Uvarov [mailto:maxim.uvarov@linaro.org]

> >>>> Sent: Thursday, September 10, 2015 2:20 PM

> >>>> To: Savolainen, Petri (Nokia - FI/Espoo); EXT Bill Fischofer

> >>>> Cc: LNG ODP Mailman List

> >>>> Subject: Re: [lng-odp] [API-NEXT PATCH] api: packet reference

> count

> >>>> support

> >>>>

> >>>> On 09/10/15 12:00, Savolainen, Petri (Nokia - FI/Espoo) wrote:

> >>>>> 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.

> >>>> I think that uses case is not close to real life. If you do in one

> >>>> thread:

> >>>> pkt = <get packet or alloc>;

> >>>> odp_packet_refcount_set(pkt, 2);

> >>>> foo(pkt);

> >>>> odp_packet_free(pkt); <- ref is 1

> >>>> foo(pkt);

> >>>> odp_packet_free(pkt); <- ref 0, free

> >>>>

> >>>> Then you know what pkt is on each line. Instead of having 2

> >> reference

> >>>> as

> >>>> in your example my_ref and pkt, you

> >>>> use just one pkt.

> >>> The point of the example is clear ownership of a reference to the

> >> packet.

> >>> These are *bugs* currently, and API should stay clear on ownership

> >> issues also multiple references:

> >>> // double free a packet (handle or reference)

> >>> odp_packet_free(pkt);

> >>> odp_packet_free(pkt);

> >>>

> >>>

> >>> // double enqueue a packet (handle or reference)

> >>> ev = odp_packet_from_event(pkt);

> >>> odp_queue_enq(queue1, ev);

> >>> odp_queue_enq(queue2, ev);

> >>>

> >>>

> >>> With two different handles (references) it's clear how owns the

> >> handle and implementation can either return the same or different

> value

> >> as the handle.

> >>> my_ref = odp_packet_create_ref(pkt);

> >>>

> >>> ev1 = odp_packet_from_event(pkt);

> >>> odp_queue_enq(queue1, ev1);

> >>>

> >>> ev2 = odp_packet_from_event(my_ref);

> >>> odp_queue_enq(queue2, ev2);

> >>>

> >> In my case it's easy to check,  inside odp_packet_free(pkt):

> >> if (!odp_packet_refcount(pkt))

> >>       ODP_ABORT("illegal free");

> >>

> >> That will capture bugs with double enqueue when you will try to send

> >> that packets.

> >>

> >>

> >> If your case we should maintain somewhere array of packet handles

> and

> >> packet headers,

> >> with unclear size depending how match reference we can have. And

> >> handles

> >> should not

> >> overlap with current packet handles. That looks more complicated,

> >> right?

> >>

> > Just like any other ODP API, this is a question of API definition vs.

> freedom of implementation. API spec defines two handles (one per

> reference) and thus user must act as if those two handles are unique,

> BUT the implementation can choose if it reuses the handle value or not.

> >

> > For example,

> >

> > typedef odp_packet_t soc_packet_descriptor_t *;

> >

> >

> > odp_packet_t odp_packet_create_ref(odp_packet_t pkt)

> > {

> > 	soc_packet_descriptor_t *desc = (soc_packet_descriptor_t *) pkt;

> >

> > 	desc->ref_count++;

> > 	return pkt;

> > }

> 

> that variant is clear, but pkt on input and output is the same - it's

> pointer to desc somewhere.

> But it's the same what I proposed.

> 

> > OR

> >

> > odp_packet_t odp_packet_create_ref(odp_packet_t pkt)

> > {

> > 	soc_packet_descriptor_t *desc = ((soc_packet_descriptor_t *) pkt)

> & PTR_MASK;

> >

> > 	desc->ref_count++;

> > 	return (desc |

> THIS_IS_A_REFERENCE_TO_A_PACKET_NOT_THE_ORIGINAL_PACKET_DESC);

> > }

> That variant modifies desc pointer. And there might be possible issues:

> 

> 1. it might not portable due to unable to restore original pointer to

> descr (if all bits of pointer address are used.).

> 2. I assume some hw schedulers can unable to work with such modified

> pointers. And it will be hard to implement in hw.

> 3. On every function which works with packet you need clear

> REFERENCE_BIT before accessing packet (access to *desc),

> which is additional overhead.



soc_packet_descriptor_t *desc = ((soc_packet_descriptor_t *) pkt) & PTR_MASK;

In this case:
- handle = pointer + flags
- pointer = handle - flags (the mask)
- implementation passes the pointer to the HW, not the handle

Handle can carry anything implementation needs, e.g. a flag which distinguishes the original packet handle from  additional references (if that's import to the implementation).


-Petri

Maxim Uvarov Sept. 11, 2015, 9:47 a.m. UTC | #16

On 09/11/15 12:32, Savolainen, Petri (Nokia - FI/Espoo) wrote:
>
>> -----Original Message-----
>> From: EXT Maxim Uvarov [mailto:maxim.uvarov@linaro.org]
>> Sent: Friday, September 11, 2015 11:30 AM
>> To: Savolainen, Petri (Nokia - FI/Espoo); EXT Bill Fischofer
>> Cc: LNG ODP Mailman List
>> Subject: Re: [lng-odp] [API-NEXT PATCH] api: packet reference count
>> support
>>
>> On 09/11/15 10:48, Savolainen, Petri (Nokia - FI/Espoo) wrote:
>>>> -----Original Message-----
>>>> From: EXT Maxim Uvarov [mailto:maxim.uvarov@linaro.org]
>>>> Sent: Thursday, September 10, 2015 7:11 PM
>>>> To: Savolainen, Petri (Nokia - FI/Espoo); EXT Bill Fischofer
>>>> Cc: LNG ODP Mailman List
>>>> Subject: Re: [lng-odp] [API-NEXT PATCH] api: packet reference count
>>>> support
>>>>
>>>> On 09/10/15 15:54, Savolainen, Petri (Nokia - FI/Espoo) wrote:
>>>>>> -----Original Message-----
>>>>>> From: EXT Maxim Uvarov [mailto:maxim.uvarov@linaro.org]
>>>>>> Sent: Thursday, September 10, 2015 2:20 PM
>>>>>> To: Savolainen, Petri (Nokia - FI/Espoo); EXT Bill Fischofer
>>>>>> Cc: LNG ODP Mailman List
>>>>>> Subject: Re: [lng-odp] [API-NEXT PATCH] api: packet reference
>> count
>>>>>> support
>>>>>>
>>>>>> On 09/10/15 12:00, Savolainen, Petri (Nokia - FI/Espoo) wrote:
>>>>>>> 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.
>>>>>> I think that uses case is not close to real life. If you do in one
>>>>>> thread:
>>>>>> pkt = <get packet or alloc>;
>>>>>> odp_packet_refcount_set(pkt, 2);
>>>>>> foo(pkt);
>>>>>> odp_packet_free(pkt); <- ref is 1
>>>>>> foo(pkt);
>>>>>> odp_packet_free(pkt); <- ref 0, free
>>>>>>
>>>>>> Then you know what pkt is on each line. Instead of having 2
>>>> reference
>>>>>> as
>>>>>> in your example my_ref and pkt, you
>>>>>> use just one pkt.
>>>>> The point of the example is clear ownership of a reference to the
>>>> packet.
>>>>> These are *bugs* currently, and API should stay clear on ownership
>>>> issues also multiple references:
>>>>> // double free a packet (handle or reference)
>>>>> odp_packet_free(pkt);
>>>>> odp_packet_free(pkt);
>>>>>
>>>>>
>>>>> // double enqueue a packet (handle or reference)
>>>>> ev = odp_packet_from_event(pkt);
>>>>> odp_queue_enq(queue1, ev);
>>>>> odp_queue_enq(queue2, ev);
>>>>>
>>>>>
>>>>> With two different handles (references) it's clear how owns the
>>>> handle and implementation can either return the same or different
>> value
>>>> as the handle.
>>>>> my_ref = odp_packet_create_ref(pkt);
>>>>>
>>>>> ev1 = odp_packet_from_event(pkt);
>>>>> odp_queue_enq(queue1, ev1);
>>>>>
>>>>> ev2 = odp_packet_from_event(my_ref);
>>>>> odp_queue_enq(queue2, ev2);
>>>>>
>>>> In my case it's easy to check,  inside odp_packet_free(pkt):
>>>> if (!odp_packet_refcount(pkt))
>>>>        ODP_ABORT("illegal free");
>>>>
>>>> That will capture bugs with double enqueue when you will try to send
>>>> that packets.
>>>>
>>>>
>>>> If your case we should maintain somewhere array of packet handles
>> and
>>>> packet headers,
>>>> with unclear size depending how match reference we can have. And
>>>> handles
>>>> should not
>>>> overlap with current packet handles. That looks more complicated,
>>>> right?
>>>>
>>> Just like any other ODP API, this is a question of API definition vs.
>> freedom of implementation. API spec defines two handles (one per
>> reference) and thus user must act as if those two handles are unique,
>> BUT the implementation can choose if it reuses the handle value or not.
>>> For example,
>>>
>>> typedef odp_packet_t soc_packet_descriptor_t *;
>>>
>>>
>>> odp_packet_t odp_packet_create_ref(odp_packet_t pkt)
>>> {
>>> 	soc_packet_descriptor_t *desc = (soc_packet_descriptor_t *) pkt;
>>>
>>> 	desc->ref_count++;
>>> 	return pkt;
>>> }
>> that variant is clear, but pkt on input and output is the same - it's
>> pointer to desc somewhere.
>> But it's the same what I proposed.
>>
>>> OR
>>>
>>> odp_packet_t odp_packet_create_ref(odp_packet_t pkt)
>>> {
>>> 	soc_packet_descriptor_t *desc = ((soc_packet_descriptor_t *) pkt)
>> & PTR_MASK;
>>> 	desc->ref_count++;
>>> 	return (desc |
>> THIS_IS_A_REFERENCE_TO_A_PACKET_NOT_THE_ORIGINAL_PACKET_DESC);
>>> }
>> That variant modifies desc pointer. And there might be possible issues:
>>
>> 1. it might not portable due to unable to restore original pointer to
>> descr (if all bits of pointer address are used.).
>> 2. I assume some hw schedulers can unable to work with such modified
>> pointers. And it will be hard to implement in hw.
>> 3. On every function which works with packet you need clear
>> REFERENCE_BIT before accessing packet (access to *desc),
>> which is additional overhead.
>
> soc_packet_descriptor_t *desc = ((soc_packet_descriptor_t *) pkt) & PTR_MASK;
>
> In this case:
> - handle = pointer + flags
> - pointer = handle - flags (the mask)
> - implementation passes the pointer to the HW, not the handle
>
> Handle can carry anything implementation needs, e.g. a flag which distinguishes the original packet handle from  additional references (if that's import to the implementation).
>
>
> -Petri
>

My point is - not everywhere you can do:

- handle = pointer + flags
- pointer = handle - flags (the mask)


Let's say pointer uses 32 bits for address (PTR_MASK is 32 bit) handle 
is also 32 bit. You just not have enough room
to save flags.

Maxim.

Savolainen, Petri (Nokia - FI/Espoo) Sept. 11, 2015, 10:07 a.m. UTC | #17

> -----Original Message-----

> From: EXT Maxim Uvarov [mailto:maxim.uvarov@linaro.org]

> Sent: Friday, September 11, 2015 12:47 PM

> To: Savolainen, Petri (Nokia - FI/Espoo); EXT Bill Fischofer

> Cc: LNG ODP Mailman List

> Subject: Re: [lng-odp] [API-NEXT PATCH] api: packet reference count

> support

> 

> On 09/11/15 12:32, Savolainen, Petri (Nokia - FI/Espoo) wrote:

> >

> >> -----Original Message-----

> >> From: EXT Maxim Uvarov [mailto:maxim.uvarov@linaro.org]

> >> Sent: Friday, September 11, 2015 11:30 AM

> >> To: Savolainen, Petri (Nokia - FI/Espoo); EXT Bill Fischofer

> >> Cc: LNG ODP Mailman List

> >> Subject: Re: [lng-odp] [API-NEXT PATCH] api: packet reference count

> >> support

> >>

> >> On 09/11/15 10:48, Savolainen, Petri (Nokia - FI/Espoo) wrote:

> >>>> -----Original Message-----

> >>>> From: EXT Maxim Uvarov [mailto:maxim.uvarov@linaro.org]

> >>>> Sent: Thursday, September 10, 2015 7:11 PM

> >>>> To: Savolainen, Petri (Nokia - FI/Espoo); EXT Bill Fischofer

> >>>> Cc: LNG ODP Mailman List

> >>>> Subject: Re: [lng-odp] [API-NEXT PATCH] api: packet reference

> count

> >>>> support

> >>>>

> >>>> On 09/10/15 15:54, Savolainen, Petri (Nokia - FI/Espoo) wrote:

> >>>>>> -----Original Message-----

> >>>>>> From: EXT Maxim Uvarov [mailto:maxim.uvarov@linaro.org]

> >>>>>> Sent: Thursday, September 10, 2015 2:20 PM

> >>>>>> To: Savolainen, Petri (Nokia - FI/Espoo); EXT Bill Fischofer

> >>>>>> Cc: LNG ODP Mailman List

> >>>>>> Subject: Re: [lng-odp] [API-NEXT PATCH] api: packet reference

> >> count

> >>>>>> support

> >>>>>>

> >>>>>> On 09/10/15 12:00, Savolainen, Petri (Nokia - FI/Espoo) wrote:

> >>>>>>> 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.

> >>>>>> I think that uses case is not close to real life. If you do in

> one

> >>>>>> thread:

> >>>>>> pkt = <get packet or alloc>;

> >>>>>> odp_packet_refcount_set(pkt, 2);

> >>>>>> foo(pkt);

> >>>>>> odp_packet_free(pkt); <- ref is 1

> >>>>>> foo(pkt);

> >>>>>> odp_packet_free(pkt); <- ref 0, free

> >>>>>>

> >>>>>> Then you know what pkt is on each line. Instead of having 2

> >>>> reference

> >>>>>> as

> >>>>>> in your example my_ref and pkt, you

> >>>>>> use just one pkt.

> >>>>> The point of the example is clear ownership of a reference to the

> >>>> packet.

> >>>>> These are *bugs* currently, and API should stay clear on

> ownership

> >>>> issues also multiple references:

> >>>>> // double free a packet (handle or reference)

> >>>>> odp_packet_free(pkt);

> >>>>> odp_packet_free(pkt);

> >>>>>

> >>>>>

> >>>>> // double enqueue a packet (handle or reference)

> >>>>> ev = odp_packet_from_event(pkt);

> >>>>> odp_queue_enq(queue1, ev);

> >>>>> odp_queue_enq(queue2, ev);

> >>>>>

> >>>>>

> >>>>> With two different handles (references) it's clear how owns the

> >>>> handle and implementation can either return the same or different

> >> value

> >>>> as the handle.

> >>>>> my_ref = odp_packet_create_ref(pkt);

> >>>>>

> >>>>> ev1 = odp_packet_from_event(pkt);

> >>>>> odp_queue_enq(queue1, ev1);

> >>>>>

> >>>>> ev2 = odp_packet_from_event(my_ref);

> >>>>> odp_queue_enq(queue2, ev2);

> >>>>>

> >>>> In my case it's easy to check,  inside odp_packet_free(pkt):

> >>>> if (!odp_packet_refcount(pkt))

> >>>>        ODP_ABORT("illegal free");

> >>>>

> >>>> That will capture bugs with double enqueue when you will try to

> send

> >>>> that packets.

> >>>>

> >>>>

> >>>> If your case we should maintain somewhere array of packet handles

> >> and

> >>>> packet headers,

> >>>> with unclear size depending how match reference we can have. And

> >>>> handles

> >>>> should not

> >>>> overlap with current packet handles. That looks more complicated,

> >>>> right?

> >>>>

> >>> Just like any other ODP API, this is a question of API definition

> vs.

> >> freedom of implementation. API spec defines two handles (one per

> >> reference) and thus user must act as if those two handles are

> unique,

> >> BUT the implementation can choose if it reuses the handle value or

> not.

> >>> For example,

> >>>

> >>> typedef odp_packet_t soc_packet_descriptor_t *;

> >>>

> >>>

> >>> odp_packet_t odp_packet_create_ref(odp_packet_t pkt)

> >>> {

> >>> 	soc_packet_descriptor_t *desc = (soc_packet_descriptor_t *) pkt;

> >>>

> >>> 	desc->ref_count++;

> >>> 	return pkt;

> >>> }

> >> that variant is clear, but pkt on input and output is the same -

> it's

> >> pointer to desc somewhere.

> >> But it's the same what I proposed.

> >>

> >>> OR

> >>>

> >>> odp_packet_t odp_packet_create_ref(odp_packet_t pkt)

> >>> {

> >>> 	soc_packet_descriptor_t *desc = ((soc_packet_descriptor_t *) pkt)

> >> & PTR_MASK;

> >>> 	desc->ref_count++;

> >>> 	return (desc |

> >> THIS_IS_A_REFERENCE_TO_A_PACKET_NOT_THE_ORIGINAL_PACKET_DESC);

> >>> }

> >> That variant modifies desc pointer. And there might be possible

> issues:

> >>

> >> 1. it might not portable due to unable to restore original pointer

> to

> >> descr (if all bits of pointer address are used.).

> >> 2. I assume some hw schedulers can unable to work with such modified

> >> pointers. And it will be hard to implement in hw.

> >> 3. On every function which works with packet you need clear

> >> REFERENCE_BIT before accessing packet (access to *desc),

> >> which is additional overhead.

> >

> > soc_packet_descriptor_t *desc = ((soc_packet_descriptor_t *) pkt) &

> PTR_MASK;

> >

> > In this case:

> > - handle = pointer + flags

> > - pointer = handle - flags (the mask)

> > - implementation passes the pointer to the HW, not the handle

> >

> > Handle can carry anything implementation needs, e.g. a flag which

> distinguishes the original packet handle from  additional references

> (if that's import to the implementation).

> >

> >

> > -Petri

> >

> 

> My point is - not everywhere you can do:

> 

> - handle = pointer + flags

> - pointer = handle - flags (the mask)

> 

> 

> Let's say pointer uses 32 bits for address (PTR_MASK is 32 bit) handle

> is also 32 bit. You just not have enough room

> to save flags.

> 

> Maxim.



It's OK since any implementation is not forced to save flags or return different handles for pkt and ref.

Also in 32 bit systems you can
- take advantage of descriptor alignment and/or memory map - and pack pointers to less than 32 bits
- use indexes instead of pointers
- define odp_packet_t as a structure of flags + pointer (e.g. sizeof struct can be 64 bits or more)

Nothing special here, just use normal programming methods to pack information into a handle ==> into a structure that you can define.

-Petri

Maxim Uvarov Sept. 11, 2015, 10:41 a.m. UTC | #18

On 09/11/15 13:07, Savolainen, Petri (Nokia - FI/Espoo) wrote:
>
>> -----Original Message-----
>> From: EXT Maxim Uvarov [mailto:maxim.uvarov@linaro.org]
>> Sent: Friday, September 11, 2015 12:47 PM
>> To: Savolainen, Petri (Nokia - FI/Espoo); EXT Bill Fischofer
>> Cc: LNG ODP Mailman List
>> Subject: Re: [lng-odp] [API-NEXT PATCH] api: packet reference count
>> support
>>
>> On 09/11/15 12:32, Savolainen, Petri (Nokia - FI/Espoo) wrote:
>>>> -----Original Message-----
>>>> From: EXT Maxim Uvarov [mailto:maxim.uvarov@linaro.org]
>>>> Sent: Friday, September 11, 2015 11:30 AM
>>>> To: Savolainen, Petri (Nokia - FI/Espoo); EXT Bill Fischofer
>>>> Cc: LNG ODP Mailman List
>>>> Subject: Re: [lng-odp] [API-NEXT PATCH] api: packet reference count
>>>> support
>>>>
>>>> On 09/11/15 10:48, Savolainen, Petri (Nokia - FI/Espoo) wrote:
>>>>>> -----Original Message-----
>>>>>> From: EXT Maxim Uvarov [mailto:maxim.uvarov@linaro.org]
>>>>>> Sent: Thursday, September 10, 2015 7:11 PM
>>>>>> To: Savolainen, Petri (Nokia - FI/Espoo); EXT Bill Fischofer
>>>>>> Cc: LNG ODP Mailman List
>>>>>> Subject: Re: [lng-odp] [API-NEXT PATCH] api: packet reference
>> count
>>>>>> support
>>>>>>
>>>>>> On 09/10/15 15:54, Savolainen, Petri (Nokia - FI/Espoo) wrote:
>>>>>>>> -----Original Message-----
>>>>>>>> From: EXT Maxim Uvarov [mailto:maxim.uvarov@linaro.org]
>>>>>>>> Sent: Thursday, September 10, 2015 2:20 PM
>>>>>>>> To: Savolainen, Petri (Nokia - FI/Espoo); EXT Bill Fischofer
>>>>>>>> Cc: LNG ODP Mailman List
>>>>>>>> Subject: Re: [lng-odp] [API-NEXT PATCH] api: packet reference
>>>> count
>>>>>>>> support
>>>>>>>>
>>>>>>>> On 09/10/15 12:00, Savolainen, Petri (Nokia - FI/Espoo) wrote:
>>>>>>>>> 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.
>>>>>>>> I think that uses case is not close to real life. If you do in
>> one
>>>>>>>> thread:
>>>>>>>> pkt = <get packet or alloc>;
>>>>>>>> odp_packet_refcount_set(pkt, 2);
>>>>>>>> foo(pkt);
>>>>>>>> odp_packet_free(pkt); <- ref is 1
>>>>>>>> foo(pkt);
>>>>>>>> odp_packet_free(pkt); <- ref 0, free
>>>>>>>>
>>>>>>>> Then you know what pkt is on each line. Instead of having 2
>>>>>> reference
>>>>>>>> as
>>>>>>>> in your example my_ref and pkt, you
>>>>>>>> use just one pkt.
>>>>>>> The point of the example is clear ownership of a reference to the
>>>>>> packet.
>>>>>>> These are *bugs* currently, and API should stay clear on
>> ownership
>>>>>> issues also multiple references:
>>>>>>> // double free a packet (handle or reference)
>>>>>>> odp_packet_free(pkt);
>>>>>>> odp_packet_free(pkt);
>>>>>>>
>>>>>>>
>>>>>>> // double enqueue a packet (handle or reference)
>>>>>>> ev = odp_packet_from_event(pkt);
>>>>>>> odp_queue_enq(queue1, ev);
>>>>>>> odp_queue_enq(queue2, ev);
>>>>>>>
>>>>>>>
>>>>>>> With two different handles (references) it's clear how owns the
>>>>>> handle and implementation can either return the same or different
>>>> value
>>>>>> as the handle.
>>>>>>> my_ref = odp_packet_create_ref(pkt);
>>>>>>>
>>>>>>> ev1 = odp_packet_from_event(pkt);
>>>>>>> odp_queue_enq(queue1, ev1);
>>>>>>>
>>>>>>> ev2 = odp_packet_from_event(my_ref);
>>>>>>> odp_queue_enq(queue2, ev2);
>>>>>>>
>>>>>> In my case it's easy to check,  inside odp_packet_free(pkt):
>>>>>> if (!odp_packet_refcount(pkt))
>>>>>>         ODP_ABORT("illegal free");
>>>>>>
>>>>>> That will capture bugs with double enqueue when you will try to
>> send
>>>>>> that packets.
>>>>>>
>>>>>>
>>>>>> If your case we should maintain somewhere array of packet handles
>>>> and
>>>>>> packet headers,
>>>>>> with unclear size depending how match reference we can have. And
>>>>>> handles
>>>>>> should not
>>>>>> overlap with current packet handles. That looks more complicated,
>>>>>> right?
>>>>>>
>>>>> Just like any other ODP API, this is a question of API definition
>> vs.
>>>> freedom of implementation. API spec defines two handles (one per
>>>> reference) and thus user must act as if those two handles are
>> unique,
>>>> BUT the implementation can choose if it reuses the handle value or
>> not.
>>>>> For example,
>>>>>
>>>>> typedef odp_packet_t soc_packet_descriptor_t *;
>>>>>
>>>>>
>>>>> odp_packet_t odp_packet_create_ref(odp_packet_t pkt)
>>>>> {
>>>>> 	soc_packet_descriptor_t *desc = (soc_packet_descriptor_t *) pkt;
>>>>>
>>>>> 	desc->ref_count++;
>>>>> 	return pkt;
>>>>> }
>>>> that variant is clear, but pkt on input and output is the same -
>> it's
>>>> pointer to desc somewhere.
>>>> But it's the same what I proposed.
>>>>
>>>>> OR
>>>>>
>>>>> odp_packet_t odp_packet_create_ref(odp_packet_t pkt)
>>>>> {
>>>>> 	soc_packet_descriptor_t *desc = ((soc_packet_descriptor_t *) pkt)
>>>> & PTR_MASK;
>>>>> 	desc->ref_count++;
>>>>> 	return (desc |
>>>> THIS_IS_A_REFERENCE_TO_A_PACKET_NOT_THE_ORIGINAL_PACKET_DESC);
>>>>> }
>>>> That variant modifies desc pointer. And there might be possible
>> issues:
>>>> 1. it might not portable due to unable to restore original pointer
>> to
>>>> descr (if all bits of pointer address are used.).
>>>> 2. I assume some hw schedulers can unable to work with such modified
>>>> pointers. And it will be hard to implement in hw.
>>>> 3. On every function which works with packet you need clear
>>>> REFERENCE_BIT before accessing packet (access to *desc),
>>>> which is additional overhead.
>>> soc_packet_descriptor_t *desc = ((soc_packet_descriptor_t *) pkt) &
>> PTR_MASK;
>>> In this case:
>>> - handle = pointer + flags
>>> - pointer = handle - flags (the mask)
>>> - implementation passes the pointer to the HW, not the handle
>>>
>>> Handle can carry anything implementation needs, e.g. a flag which
>> distinguishes the original packet handle from  additional references
>> (if that's import to the implementation).
>>>
>>> -Petri
>>>
>> My point is - not everywhere you can do:
>>
>> - handle = pointer + flags
>> - pointer = handle - flags (the mask)
>>
>>
>> Let's say pointer uses 32 bits for address (PTR_MASK is 32 bit) handle
>> is also 32 bit. You just not have enough room
>> to save flags.
>>
>> Maxim.
>
> It's OK since any implementation is not forced to save flags or return different handles for pkt and ref.
>
> Also in 32 bit systems you can
> - take advantage of descriptor alignment and/or memory map - and pack pointers to less than 32 bits
> - use indexes instead of pointers
> - define odp_packet_t as a structure of flags + pointer (e.g. sizeof struct can be 64 bits or more)
>
> Nothing special here, just use normal programming methods to pack information into a handle ==> into a structure that you can define.
>
> -Petri
>
>   

Ok, I will send v2 with that changes.

Maxim.

>
>
>

[API-NEXT] api: packet reference count support

Commit Message

Comments

Patch