> -----Original Message-----
> From: Bill Fischofer [mailto:bill.fischo...@linaro.org]
> Sent: Friday, February 10, 2017 12:46 AM
> To: Savolainen, Petri (Nokia - FI/Espoo) <petri.savolainen@nokia-bell-
> labs.com>
> Cc: lng-odp@lists.linaro.org
> Subject: Re: [lng-odp] Packet references API
>
> On Thu, Feb 9, 2017 at 9:32 AM, Savolainen, Petri (Nokia - FI/Espoo)
> <petri.savolai...@nokia-bell-labs.com> wrote:
> > Hi,
> >
> > I still think that packet ref API should be defined so that an
> implementation may fall back to doing packet copy instead of reference to
> shared data (if it simply cannot handle packets by reference). Now after
> implementing packet ref API as copy (in about 90 lines), only issues are
> with tests like this one ...
> >
> > ref = odp_packet_ref(pkt, 100);
> > odp_packet_free(ref);
> >
> > /* Verify that pkt no longer has a ref */
> > CU_ASSERT(odp_packet_has_ref(pkt) == 0);
> >
> > ... since free(ref) would need to update also 'pkt'.
The issue is that today spec is not explicit enough has_ref value, and
validation tests do this:
ref = odp_packet_ref(pkt, 100);
CU_ASSERT(odp_packet_has_ref(pkt) == 1);
CU_ASSERT(odp_packet_has_ref(ref) == 1);
odp_packet_free(ref);
/* Verify that pkt no longer has a ref */
CU_ASSERT(odp_packet_has_ref(pkt) == 0);
So, first 'pkt' has to return that it has multiple references and shared data,
and then right after the free call it must not have those any more. That means
that free(ref) must update also 'pkt'.
API needs to state explicitly how has_ref() works. There are two options how to
enable implementation by copy:
1) has_ref (and shared len) is not set for packets if implementation is copy
==> remove the first two CU_ASSERT()'s above
2) has_ref == 1 (and shared len) indicate that 'pkt' may still have other
references to it ==> remove the last CU_ASSERT() above
I think option 1) is better.
>
> There is no need for any special action here since if the
> implementation did a copy under the covers then the packet would not
> have any references. In fact, it would be a useful feature that
> odp_packet_has_ref(pkt) == 0 after a odp_packet_ref() call would be a
> positive confirmation that the "reference" was finessed with a copy
> under the covers. Note that in this case odp_packet_unshared_len(pkt)
> would be similarly unchanged.
Has_ref() spec needs update to allow that return value may be 0 or 1 after
packet_ref(), depending on implementation.
>
> A true reference means exactly that: the packet is sharing one or more
> segments with some other handle. Trying to lie about that if a copy
> has taken place is not only complicated but also potentially
> dangerously misleading.
Has_ref is easy to update during packet_ref() but hard during free(). If
has_ref is set on both but not cleared, application would need to continue
"read-only handling of packet" longer than it normally would.
Anyway, not relevant if 1) is done.
>
> The problem, of course, is that the utility of references on such
> implementations is questionable since references are intended to be a
> high-performance zero-copy alternative to the application making
> packet copies itself. But if that's the best a given implementation
> can do then that's is sufficient to claim API conformance since the
> ODP spec does not make specific performance guarantees. It's the
> reason, for example, that odp-linux was able to get away with not
> having real support for ordered queues by substituting atomic queues
> initially.
Ordered vs atomic queue spec was intentionally written so that ordered queues
can be implemented with atomic queues (HW).
>
> >
> > I think we should lesser the synchronization guarantees of
> odp_packet_has_ref(), so that it returns if a packet "may have" still have
> multiple references. And same thing for odp_packet_unshared_len() (may
> have shared bytes). If application needs
>
> Disagree. Just be honest. "Maybe" predicates are useless.
"May still have other references to it": has_ref() returns always 1 after
packet_ref() but may not return 0 right after free, or never.
>
> to be sure, it would need to be assured some other way (e.g.
> calculating itself how many references was created / freed). Most
> application would not be interested on the book keeping, but on the
> possibility of creating and using references.
>
> Again, no need to do anything special since with references every
> reference must be freed to avoid memory leaks anyway and this is true
> regardless of whether copies are being done or true segment sharing is
> going on.
I'll send an API and validation test updates (for option 1)) with simple copy
style implementation.
-Petri
