Re: [lng-odp] [API-NEXT PATCHv4 0/5] Add Packet Splice/Reference APIs

[Bill Fischofer]

On Tue, Oct 18, 2016 at 9:56 AM, Ola Liljedahl 
wrote:

>
>
> On 17 October 2016 at 16:34, Bill Fischofer 
> wrote:
>
>>
>>
>> On Mon, Oct 17, 2016 at 8:54 AM, Ola Liljedahl 
>> wrote:
>>
>>> On 10 October 2016 at 17:50, Bill Fischofer 
>>> wrote:
>>>
 This patch adds support for packet references and splices following
 discussions at LAS16 on this subject.

 I've changed things around from Petri's original proposal by splitting
 this
 into two separate APIs: odp_packet_splice() and odp_packet_ref(), where
 the
 latter is just a splice of a zero-length header on to a base packet. The
 various odp packet manipulation APIs have also been enhanced to behave
 sensibly when presented with a spliced packet as input. Reference
 counts are
 used to enable odp_packet_free() to not free a packet until all splices
 based
 on it are also freed.

>>> Seems OK.
>>>
>>>

 Also added are two new APIs for working with spliced packets as these
 seem
 necessary for completeness:

 - odp_packet_is_a_splice() tells whether an input packet is a splice,
 and if
 so how many spliced packets it contains

 - odp_packet_is_spliced() tells whether any splices have been created
 on this
 packet, and if so how many.

>>> Is there any conceptual difference between the base packet and any
>>> splices?
>>>
>>> Can you add a splice to a splice?
>>>
>>
>> Architecturally, yes. Compound splices of arbitrary depth can be
>> accommodated , however not every implementation may be able to support
>> this. I'd expect us to need to add some odp_packet_capability() info to
>> allow implementation limits to be communicated to applications, but we need
>> to specify what sort of things we want to be able to limit this way.
>>
>>
>>>
>>>

 Note that there is no odp_packet_unsplice() API. To remove a splice
 from a
 base packet currently requres that the splice be freed via an
 odp_packet_free() call. We should discuss and decide if such an API is
 warranted for symmetry.

>>> As long as odp_packet_free() respects any additional references (to
>>> segments)
>>> caused by splicing, I don't think we need any free splice call. How
>>> would it be
>>> different from odp_packet_free()?
>>>
>>
>> After splicing the header packet becomes the handle for the resulting
>> splice, while the handle for the base packet is still valid. An unsplice
>> operation would separate these so that what was the splice handle becomes
>> just a handle to the header, which can then still be used. Not sure if
>> there is a use case for this. It would basically be an "undo" on the splice.
>>
> Aha.
>
>
>>
>>
>>>
>>> What happens when you truncate the tail? E.g. of the base packet?
>>> Does this affect the splices? (I prefer not).
>>>
>>
>> Yes. After a successful splice, all offsets beyond the splice point are
>> shared with all other splices so any change to that part of the splice is
>> visible to all other splices on the same base packet. For this reason it's
>> suggested that the shared portion be treated as read only. However, if the
>> application changes the base packet or an offset in a splice that is
>> visible to other splices, then it's the application responsibility to
>> ensure that proper synchronization is performed to avoid unpredictable
>> behavior.
>>
> Is it really unpredictable?
> If you change packet data through one handle, all other handles referring
> to the same segment should also see this change.
>
> And implementation that implements splicing through copying will not see
> such changes. So perhaps implementation dependent behaviour.
>
>
The fundamental issue here is that ODP assumes that only one thread at a
time can "own" a packet handle, so manipulations of a packet via that
handle are assumed to be lockless since only one thread at a time can be
using that handle. However, with the introduction of references and splices
you can have two or more separate odp_packet_t handles referring to the
same data and there's nothing to prevent multiple threads to be referring
to that shared data via their own packet handles that just happen to be
referring to the same base packet. So unless we wanted to introduce a lot
of locking semantics onto packet operations (something that would be
detrimental to performance) then the safest course is just to say "treat
the shared portion as read-only" or else things are unpredictable. However
if the application manages its use so that only one thread updates the base
packet then these changes could be visible in a controlled manner to other
threads that view it through other handles. But even here there's the
question of whether one thread might be trying to reference data that
another thread was invalidating (e.g., via an odp_packet_pull_tail()
operation on the base packet). So 

Re: [lng-odp] [API-NEXT PATCHv4 0/5] Add Packet Splice/Reference APIs

[Ola Liljedahl]

On 17 October 2016 at 16:34, Bill Fischofer 
wrote:

>
>
> On Mon, Oct 17, 2016 at 8:54 AM, Ola Liljedahl 
> wrote:
>
>> On 10 October 2016 at 17:50, Bill Fischofer 
>> wrote:
>>
>>> This patch adds support for packet references and splices following
>>> discussions at LAS16 on this subject.
>>>
>>> I've changed things around from Petri's original proposal by splitting
>>> this
>>> into two separate APIs: odp_packet_splice() and odp_packet_ref(), where
>>> the
>>> latter is just a splice of a zero-length header on to a base packet. The
>>> various odp packet manipulation APIs have also been enhanced to behave
>>> sensibly when presented with a spliced packet as input. Reference counts
>>> are
>>> used to enable odp_packet_free() to not free a packet until all splices
>>> based
>>> on it are also freed.
>>>
>> Seems OK.
>>
>>
>>>
>>> Also added are two new APIs for working with spliced packets as these
>>> seem
>>> necessary for completeness:
>>>
>>> - odp_packet_is_a_splice() tells whether an input packet is a splice,
>>> and if
>>> so how many spliced packets it contains
>>>
>>> - odp_packet_is_spliced() tells whether any splices have been created on
>>> this
>>> packet, and if so how many.
>>>
>> Is there any conceptual difference between the base packet and any
>> splices?
>>
>> Can you add a splice to a splice?
>>
>
> Architecturally, yes. Compound splices of arbitrary depth can be
> accommodated , however not every implementation may be able to support
> this. I'd expect us to need to add some odp_packet_capability() info to
> allow implementation limits to be communicated to applications, but we need
> to specify what sort of things we want to be able to limit this way.
>
>
>>
>>
>>>
>>> Note that there is no odp_packet_unsplice() API. To remove a splice from
>>> a
>>> base packet currently requres that the splice be freed via an
>>> odp_packet_free() call. We should discuss and decide if such an API is
>>> warranted for symmetry.
>>>
>> As long as odp_packet_free() respects any additional references (to
>> segments)
>> caused by splicing, I don't think we need any free splice call. How would
>> it be
>> different from odp_packet_free()?
>>
>
> After splicing the header packet becomes the handle for the resulting
> splice, while the handle for the base packet is still valid. An unsplice
> operation would separate these so that what was the splice handle becomes
> just a handle to the header, which can then still be used. Not sure if
> there is a use case for this. It would basically be an "undo" on the splice.
>
Aha.


>
>
>>
>> What happens when you truncate the tail? E.g. of the base packet?
>> Does this affect the splices? (I prefer not).
>>
>
> Yes. After a successful splice, all offsets beyond the splice point are
> shared with all other splices so any change to that part of the splice is
> visible to all other splices on the same base packet. For this reason it's
> suggested that the shared portion be treated as read only. However, if the
> application changes the base packet or an offset in a splice that is
> visible to other splices, then it's the application responsibility to
> ensure that proper synchronization is performed to avoid unpredictable
> behavior.
>
Is it really unpredictable?
If you change packet data through one handle, all other handles referring
to the same segment should also see this change.

And implementation that implements splicing through copying will not see
such changes. So perhaps implementation dependent behaviour.



>
>
>>
>> To use this splice API for fragmentation, you want to be able to add
>> splices with new
>> L2/IP headers at suitable places (at each MTU point) and then truncate
>> the base packet
>> so that only the just added splice refers the the data beyond the new end
>> of the base
>> packet.
>>
>
> That's not a use case that's currently supported as a splice is created
> with an offset only rather than an offset and length, so splices go from
> the specified offset through the end of the base packet.
>
That's OK. That's what I want. What I don't want it that if I truncate the
length of the packet through one handle, that should affect the other
handles. Is that behaviour really intrinsic to the splice API definition?


> If we wanted we could expand the API to accommodate a length. We should
> discuss further. The concern here is whether such extensions would be
> efficiently implementable on all platforms.
>
>
>>
>> It would be useful to use the API for reassembly. Splice fragment N onto
>> fragment N+1
>> (after the IP header of frag N+1) and free fragment N+1. Repeat for all
>> fragments from
>> last to first. The splice for the first fragment will then refer to the
>> complete reassembled
>> datagram.
>>
>> It seems like these use cases should be supported by this API if the
>> specification is
>> written with this in mind.
>>
>
> That would be a reasonable use case 

Re: [lng-odp] [API-NEXT PATCHv4 0/5] Add Packet Splice/Reference APIs

[Bill Fischofer]

On Mon, Oct 17, 2016 at 8:54 AM, Ola Liljedahl 
wrote:

> On 10 October 2016 at 17:50, Bill Fischofer 
> wrote:
>
>> This patch adds support for packet references and splices following
>> discussions at LAS16 on this subject.
>>
>> I've changed things around from Petri's original proposal by splitting
>> this
>> into two separate APIs: odp_packet_splice() and odp_packet_ref(), where
>> the
>> latter is just a splice of a zero-length header on to a base packet. The
>> various odp packet manipulation APIs have also been enhanced to behave
>> sensibly when presented with a spliced packet as input. Reference counts
>> are
>> used to enable odp_packet_free() to not free a packet until all splices
>> based
>> on it are also freed.
>>
> Seems OK.
>
>
>>
>> Also added are two new APIs for working with spliced packets as these seem
>> necessary for completeness:
>>
>> - odp_packet_is_a_splice() tells whether an input packet is a splice, and
>> if
>> so how many spliced packets it contains
>>
>> - odp_packet_is_spliced() tells whether any splices have been created on
>> this
>> packet, and if so how many.
>>
> Is there any conceptual difference between the base packet and any splices?
>
> Can you add a splice to a splice?
>

Architecturally, yes. Compound splices of arbitrary depth can be
accommodated , however not every implementation may be able to support
this. I'd expect us to need to add some odp_packet_capability() info to
allow implementation limits to be communicated to applications, but we need
to specify what sort of things we want to be able to limit this way.


>
>
>>
>> Note that there is no odp_packet_unsplice() API. To remove a splice from a
>> base packet currently requres that the splice be freed via an
>> odp_packet_free() call. We should discuss and decide if such an API is
>> warranted for symmetry.
>>
> As long as odp_packet_free() respects any additional references (to
> segments)
> caused by splicing, I don't think we need any free splice call. How would
> it be
> different from odp_packet_free()?
>

After splicing the header packet becomes the handle for the resulting
splice, while the handle for the base packet is still valid. An unsplice
operation would separate these so that what was the splice handle becomes
just a handle to the header, which can then still be used. Not sure if
there is a use case for this. It would basically be an "undo" on the splice.


>
> What happens when you truncate the tail? E.g. of the base packet?
> Does this affect the splices? (I prefer not).
>

Yes. After a successful splice, all offsets beyond the splice point are
shared with all other splices so any change to that part of the splice is
visible to all other splices on the same base packet. For this reason it's
suggested that the shared portion be treated as read only. However, if the
application changes the base packet or an offset in a splice that is
visible to other splices, then it's the application responsibility to
ensure that proper synchronization is performed to avoid unpredictable
behavior.


>
> To use this splice API for fragmentation, you want to be able to add
> splices with new
> L2/IP headers at suitable places (at each MTU point) and then truncate the
> base packet
> so that only the just added splice refers the the data beyond the new end
> of the base
> packet.
>

That's not a use case that's currently supported as a splice is created
with an offset only rather than an offset and length, so splices go from
the specified offset through the end of the base packet. If we wanted we
could expand the API to accommodate a length. We should discuss further.
The concern here is whether such extensions would be efficiently
implementable on all platforms.


>
> It would be useful to use the API for reassembly. Splice fragment N onto
> fragment N+1
> (after the IP header of frag N+1) and free fragment N+1. Repeat for all
> fragments from
> last to first. The splice for the first fragment will then refer to the
> complete reassembled
> datagram.
>
> It seems like these use cases should be supported by this API if the
> specification is
> written with this in mind.
>

That would be a reasonable use case with the ability to specify a length as
well as an offset on odp_packet_splice(), however this would also require
explicit support for nested splices since ipfrag reassembly can involve
many such operations.


>
> I will check the later patches as well. We have a bunch of use cases
> define here:
> https://docs.google.com/document/d/1r4olxr39fHvgFACQp_
> 1RL_mh9736no3TFaVM7_XkUtM/edit
>
>
I'll look at that doc and add comments there. I notice it's viewable but
not commentable, please enable comments on that doc.


>
>
>>
>> Changes for v4:
>> - Add negative tests to validation test suite
>> - Fix implementation bugs relating to negative tests
>>
>> Changes for v3:
>> - Bug fixes (detected by the validation tests)
>> - Addition of validation tests for 

Re: [lng-odp] [API-NEXT PATCHv4 0/5] Add Packet Splice/Reference APIs

[Ola Liljedahl]

On 10 October 2016 at 17:50, Bill Fischofer 
wrote:

> This patch adds support for packet references and splices following
> discussions at LAS16 on this subject.
>
> I've changed things around from Petri's original proposal by splitting this
> into two separate APIs: odp_packet_splice() and odp_packet_ref(), where the
> latter is just a splice of a zero-length header on to a base packet. The
> various odp packet manipulation APIs have also been enhanced to behave
> sensibly when presented with a spliced packet as input. Reference counts
> are
> used to enable odp_packet_free() to not free a packet until all splices
> based
> on it are also freed.
>
Seems OK.


>
> Also added are two new APIs for working with spliced packets as these seem
> necessary for completeness:
>
> - odp_packet_is_a_splice() tells whether an input packet is a splice, and
> if
> so how many spliced packets it contains
>
> - odp_packet_is_spliced() tells whether any splices have been created on
> this
> packet, and if so how many.
>
Is there any conceptual difference between the base packet and any splices?

Can you add a splice to a splice?


>
> Note that there is no odp_packet_unsplice() API. To remove a splice from a
> base packet currently requres that the splice be freed via an
> odp_packet_free() call. We should discuss and decide if such an API is
> warranted for symmetry.
>
As long as odp_packet_free() respects any additional references (to
segments)
caused by splicing, I don't think we need any free splice call. How would
it be
different from odp_packet_free()?

What happens when you truncate the tail? E.g. of the base packet?
Does this affect the splices? (I prefer not).

To use this splice API for fragmentation, you want to be able to add
splices with new
L2/IP headers at suitable places (at each MTU point) and then truncate the
base packet
so that only the just added splice refers the the data beyond the new end
of the base
packet.

It would be useful to use the API for reassembly. Splice fragment N onto
fragment N+1
(after the IP header of frag N+1) and free fragment N+1. Repeat for all
fragments from
last to first. The splice for the first fragment will then refer to the
complete reassembled
datagram.

It seems like these use cases should be supported by this API if the
specification is
written with this in mind.

I will check the later patches as well. We have a bunch of use cases define
here:
https://docs.google.com/document/d/1r4olxr39fHvgFACQp_1RL_mh9736no3TFaVM7_XkUtM/edit



>
> Changes for v4:
> - Add negative tests to validation test suite
> - Fix implementation bugs relating to negative tests
>
> Changes for v3:
> - Bug fixes (detected by the validation tests)
> - Addition of validation tests for these new APIs
> - Diagrams and User Guide documentation for these new APIs
>
> Changes for v2:
> - Bug fixes
> - Enhance ODP packet segment APIs to behave properly with spliced packets
>
> Bill Fischofer (5):
>   api: packet: add support for packet splices and references
>   linux-generic: packet: implement splice/reference apis
>   validation: packet: add packet splice/reference tests
>   doc: images: add images for packet splice/reference documentation
>   doc: userguide: add user documentation for packet splice/reference
> APIs
>
>  doc/images/doublesplice.svg|  67 ++
>  doc/images/pktref.svg  |  49 +
>  doc/images/splice.svg  |  64 ++
>  doc/users-guide/users-guide-packet.adoc| 118 ++
>  include/odp/api/spec/packet.h  | 103 +
>  .../linux-generic/include/odp_packet_internal.h|  54 -
>  platform/linux-generic/odp_packet.c| 241
> ++---
>  test/common_plat/validation/api/packet/packet.c| 176 +++
>  test/common_plat/validation/api/packet/packet.h|   1 +
>  9 files changed, 836 insertions(+), 37 deletions(-)
>  create mode 100644 doc/images/doublesplice.svg
>  create mode 100644 doc/images/pktref.svg
>  create mode 100644 doc/images/splice.svg
>
> --
> 2.7.4
>
>