On Fri, Feb 10, 2017 at 6:24 AM, Savolainen, Petri (Nokia - FI/Espoo) <petri.savolai...@nokia-bell-labs.com> wrote: > > >> -----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.
I agree the spec needs to be tightened to cover the possibility that an implementation may substitute copy operations and the validation tests need to accommodate that. If you'd like to propose a revised spec wording I'll be happy to update the validation test to conform to the revision. I agree option 1) is the better choice here as it is simple and straightforward to both implement and test. We want has_ref() to indicate that actual sharing is going on. > >> >> 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. Agreed, as noted above. I'd be explicit and state that implementations may use copy operations internally for any ref() call and that has_ref() provides a means for applications to discover whether this happened. BTW, this is one of the reasons I originally wanted has_ref() to return the actual number of references and not just be a boolean. Implementations that do reference counting have to know this anyway since that's how they know when they should actually release storage when an odp_packet_free() call is made. If we keep this as a boolean it will be problematic to state how this API behaves if an implementation does sharing for some ref() calls and has to fall back to copy operations for more complex cases such as doing a ref on a ref, which may be difficult in some implementations. > >> >> 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. Which is why I think we agree that 1) is the correct choice here. > >> >> 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). Yes, it's just that the intent behind ordered queues is to support more than one core operating on a flow simultaneously and that doesn't actually happen if you substitute atomic queues, even though they are semantically equivalent. So it's a performance statement, not a functional statement. The same is true of references. The intent is that these are zero-copy operations but if the implementation does copies under the covers the result is semantically equivalent but at lower performance levels. > > >> >> > >> > 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. Again, that makes the predicate non-deterministic so I'd question why have it at all. All other ODP predicates are deterministic, which is why they are useful to applications. It's not clear how any application would make use of a "maybe" predicate. > >> >> 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. It's not clear why we need a copy style implementation given that we have a zero-copy implementation. While it's good to discuss this as an option in the ODP implementation guide, as that's useful for other implementations, trying to put that into odp-linux at this stage would seem to be a step backwards. If we need to improve the current implementation we should do so, same as we've done for other areas (scheduler, ordered queues, pools, etc.). > > -Petri >
