On 09/11/15 12:32, Savolainen, Petri (Nokia - FI/Espoo) wrote:
-----Original Message-----
From: EXT Maxim Uvarov [mailto:maxim.uva...@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.uva...@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.uva...@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.
_______________________________________________
lng-odp mailing list
lng-odp@lists.linaro.org
https://lists.linaro.org/mailman/listinfo/lng-odp
