diff mbox

[PATCHv5,8/9] api: packet: add head/tail room manipulation

Message ID 1418733042-18047-9-git-send-email-taras.kondratiuk@linaro.org
State Accepted
Commit 543fd5e56c6cf468ed25ccc8fe3e5c48a0c39fc2
Headers show

Commit Message

Taras Kondratiuk Dec. 16, 2014, 12:30 p.m. UTC
From: Bill Fischofer <bill.fischofer@linaro.org>

Signed-off-by: Bill Fischofer <bill.fischofer@linaro.org>
Signed-off-by: Taras Kondratiuk <taras.kondratiuk@linaro.org>
---
 example/ipsec/odp_ipsec.c                          |   4 +-
 example/ipsec/odp_ipsec_stream.c                   |   2 +-
 platform/linux-generic/include/api/odp_packet.h    | 275 +++++++++++++++++----
 .../linux-generic/include/odp_packet_internal.h    |  33 +++
 platform/linux-generic/odp_packet.c                |  97 +++++++-
 5 files changed, 349 insertions(+), 62 deletions(-)

Comments

Bill Fischofer Dec. 16, 2014, 1:35 p.m. UTC | #1
Taras is just repackaging the v4 patch from December 12th.

We can certainly consider this a trivial bug-fix if needed. However I would
like to understand what the advantage of not making these adjustments as
part of these operations since without them after a push_head() operation
all off the offsets will be incorrect.  Is this some sort of performance
concern or is there are reason why it's preferable to ask every application
take responsibility for these adjustments as opposed to letting the
implementation do them?

I ask this because in SoCs that maintain this sort of meta data in HW would
need special additional SW to "undo" these adjustments if that's a required
feature of the ODP API.

On Tue, Dec 16, 2014 at 7:13 AM, Savolainen, Petri (NSN - FI/Espoo) <
petri.savolainen@nsn.com> wrote:
>
> > diff --git a/platform/linux-generic/include/odp_packet_internal.h
> > b/platform/linux-generic/include/odp_packet_internal.h
> > index 0b24300..bd5daf0 100644
> > --- a/platform/linux-generic/include/odp_packet_internal.h
> > +++ b/platform/linux-generic/include/odp_packet_internal.h
> > @@ -188,6 +188,39 @@ static inline void *packet_map(odp_packet_hdr_t
> > *pkt_hdr,
> >                         pkt_hdr->headroom + pkt_hdr->frame_len);
> >  }
> >
> > +#define pull_offset(x, len) (x = x < len ? 0 : x - len)
> > +
> > +static inline void push_head(odp_packet_hdr_t *pkt_hdr, size_t len)
> > +{
> > +     pkt_hdr->headroom  -= len;
> > +     pkt_hdr->frame_len += len;
> > +     pkt_hdr->l2_offset += len;
> > +     pkt_hdr->l3_offset += len;
> > +     pkt_hdr->l4_offset += len;
>
> "User is responsible to update packet meta-data offsets when needed."
>
> This operation must not modify L2/l3/l4 offsets.
>
>
> > +}
> > +
> > +static inline void pull_head(odp_packet_hdr_t *pkt_hdr, size_t len)
> > +{
> > +     pkt_hdr->headroom  += len;
> > +     pkt_hdr->frame_len -= len;
> > +     pull_offset(pkt_hdr->l2_offset, len);
> > +     pull_offset(pkt_hdr->l3_offset, len);
> > +     pull_offset(pkt_hdr->l4_offset, len);
>
> Same thing here.
>
> -Petri
>
>
> > +}
> > +
> > +static inline void push_tail(odp_packet_hdr_t *pkt_hdr, size_t len)
> > +{
> > +     pkt_hdr->tailroom  -= len;
> > +     pkt_hdr->frame_len += len;
> > +}
> > +
> > +
> > +static inline void pull_tail(odp_packet_hdr_t *pkt_hdr, size_t len)
> > +{
> > +     pkt_hdr->tailroom  += len;
> > +     pkt_hdr->frame_len -= len;
> > +}
> > +
>
>
>
>
> _______________________________________________
> lng-odp mailing list
> lng-odp@lists.linaro.org
> http://lists.linaro.org/mailman/listinfo/lng-odp
>
Bill Fischofer Dec. 16, 2014, 1:54 p.m. UTC | #2
If an application strips off an L2 header that hasn't affected the L3 or L4
headers, but without these adjustments odp_packet_l3_ptr() and
odp_packet_l4_ptr() would have no meaning.

Similarly, if an application pushes a shim header onto a packet, again it
hasn't affected the validity of whatever headers are part of the packet,
but again the accessor functions for these other headers would no longer be
pointing to the correct places in the packet.

So I'm not sure I understand the point you're making.

On Tue, Dec 16, 2014 at 7:48 AM, Savolainen, Petri (NSN - FI/Espoo) <
petri.savolainen@nsn.com> wrote:
>
>  The reason is that only application knows what it’s doing to the packet.
> Application is likely to update the offsets anyway after the push/pull
> operation. Implementation would just speculate what is happening to the
> packet.
>
>
>
> -Petri
>
>
>
>
>
> *From:* ext Bill Fischofer [mailto:bill.fischofer@linaro.org]
> *Sent:* Tuesday, December 16, 2014 3:36 PM
> *To:* Savolainen, Petri (NSN - FI/Espoo)
> *Cc:* ext Taras Kondratiuk; lng-odp@lists.linaro.org
> *Subject:* Re: [lng-odp] [PATCHv5 8/9] api: packet: add head/tail room
> manipulation
>
>
>
> Taras is just repackaging the v4 patch from December 12th.
>
>
>
> We can certainly consider this a trivial bug-fix if needed. However I
> would like to understand what the advantage of not making these adjustments
> as part of these operations since without them after a push_head()
> operation all off the offsets will be incorrect.  Is this some sort of
> performance concern or is there are reason why it's preferable to ask every
> application take responsibility for these adjustments as opposed to letting
> the implementation do them?
>
>
>
> I ask this because in SoCs that maintain this sort of meta data in HW
> would need special additional SW to "undo" these adjustments if that's a
> required feature of the ODP API.
>
>
>
> On Tue, Dec 16, 2014 at 7:13 AM, Savolainen, Petri (NSN - FI/Espoo) <
> petri.savolainen@nsn.com> wrote:
>
> > diff --git a/platform/linux-generic/include/odp_packet_internal.h
> > b/platform/linux-generic/include/odp_packet_internal.h
> > index 0b24300..bd5daf0 100644
> > --- a/platform/linux-generic/include/odp_packet_internal.h
> > +++ b/platform/linux-generic/include/odp_packet_internal.h
> > @@ -188,6 +188,39 @@ static inline void *packet_map(odp_packet_hdr_t
> > *pkt_hdr,
> >                         pkt_hdr->headroom + pkt_hdr->frame_len);
> >  }
> >
> > +#define pull_offset(x, len) (x = x < len ? 0 : x - len)
> > +
> > +static inline void push_head(odp_packet_hdr_t *pkt_hdr, size_t len)
> > +{
> > +     pkt_hdr->headroom  -= len;
> > +     pkt_hdr->frame_len += len;
> > +     pkt_hdr->l2_offset += len;
> > +     pkt_hdr->l3_offset += len;
> > +     pkt_hdr->l4_offset += len;
>
> "User is responsible to update packet meta-data offsets when needed."
>
> This operation must not modify L2/l3/l4 offsets.
>
>
> > +}
> > +
> > +static inline void pull_head(odp_packet_hdr_t *pkt_hdr, size_t len)
> > +{
> > +     pkt_hdr->headroom  += len;
> > +     pkt_hdr->frame_len -= len;
> > +     pull_offset(pkt_hdr->l2_offset, len);
> > +     pull_offset(pkt_hdr->l3_offset, len);
> > +     pull_offset(pkt_hdr->l4_offset, len);
>
> Same thing here.
>
> -Petri
>
>
> > +}
> > +
> > +static inline void push_tail(odp_packet_hdr_t *pkt_hdr, size_t len)
> > +{
> > +     pkt_hdr->tailroom  -= len;
> > +     pkt_hdr->frame_len += len;
> > +}
> > +
> > +
> > +static inline void pull_tail(odp_packet_hdr_t *pkt_hdr, size_t len)
> > +{
> > +     pkt_hdr->tailroom  += len;
> > +     pkt_hdr->frame_len -= len;
> > +}
> > +
>
>
>
>
> _______________________________________________
> lng-odp mailing list
> lng-odp@lists.linaro.org
> http://lists.linaro.org/mailman/listinfo/lng-odp
>
Mike Holmes Dec. 16, 2014, 1:56 p.m. UTC | #3
One question, does this have to be solved today ?

I'd like to get 0.5 out today and 0.6 is next week it can take the patches
after discussion ?

On 16 December 2014 at 08:54, Bill Fischofer <bill.fischofer@linaro.org>
wrote:
>
> If an application strips off an L2 header that hasn't affected the L3 or
> L4 headers, but without these adjustments odp_packet_l3_ptr() and
> odp_packet_l4_ptr() would have no meaning.
>
> Similarly, if an application pushes a shim header onto a packet, again it
> hasn't affected the validity of whatever headers are part of the packet,
> but again the accessor functions for these other headers would no longer be
> pointing to the correct places in the packet.
>
> So I'm not sure I understand the point you're making.
>
> On Tue, Dec 16, 2014 at 7:48 AM, Savolainen, Petri (NSN - FI/Espoo) <
> petri.savolainen@nsn.com> wrote:
>>
>>  The reason is that only application knows what it’s doing to the
>> packet. Application is likely to update the offsets anyway after the
>> push/pull operation. Implementation would just speculate what is happening
>> to the packet.
>>
>>
>>
>> -Petri
>>
>>
>>
>>
>>
>> *From:* ext Bill Fischofer [mailto:bill.fischofer@linaro.org]
>> *Sent:* Tuesday, December 16, 2014 3:36 PM
>> *To:* Savolainen, Petri (NSN - FI/Espoo)
>> *Cc:* ext Taras Kondratiuk; lng-odp@lists.linaro.org
>> *Subject:* Re: [lng-odp] [PATCHv5 8/9] api: packet: add head/tail room
>> manipulation
>>
>>
>>
>> Taras is just repackaging the v4 patch from December 12th.
>>
>>
>>
>> We can certainly consider this a trivial bug-fix if needed. However I
>> would like to understand what the advantage of not making these adjustments
>> as part of these operations since without them after a push_head()
>> operation all off the offsets will be incorrect.  Is this some sort of
>> performance concern or is there are reason why it's preferable to ask every
>> application take responsibility for these adjustments as opposed to letting
>> the implementation do them?
>>
>>
>>
>> I ask this because in SoCs that maintain this sort of meta data in HW
>> would need special additional SW to "undo" these adjustments if that's a
>> required feature of the ODP API.
>>
>>
>>
>> On Tue, Dec 16, 2014 at 7:13 AM, Savolainen, Petri (NSN - FI/Espoo) <
>> petri.savolainen@nsn.com> wrote:
>>
>> > diff --git a/platform/linux-generic/include/odp_packet_internal.h
>> > b/platform/linux-generic/include/odp_packet_internal.h
>> > index 0b24300..bd5daf0 100644
>> > --- a/platform/linux-generic/include/odp_packet_internal.h
>> > +++ b/platform/linux-generic/include/odp_packet_internal.h
>> > @@ -188,6 +188,39 @@ static inline void *packet_map(odp_packet_hdr_t
>> > *pkt_hdr,
>> >                         pkt_hdr->headroom + pkt_hdr->frame_len);
>> >  }
>> >
>> > +#define pull_offset(x, len) (x = x < len ? 0 : x - len)
>> > +
>> > +static inline void push_head(odp_packet_hdr_t *pkt_hdr, size_t len)
>> > +{
>> > +     pkt_hdr->headroom  -= len;
>> > +     pkt_hdr->frame_len += len;
>> > +     pkt_hdr->l2_offset += len;
>> > +     pkt_hdr->l3_offset += len;
>> > +     pkt_hdr->l4_offset += len;
>>
>> "User is responsible to update packet meta-data offsets when needed."
>>
>> This operation must not modify L2/l3/l4 offsets.
>>
>>
>> > +}
>> > +
>> > +static inline void pull_head(odp_packet_hdr_t *pkt_hdr, size_t len)
>> > +{
>> > +     pkt_hdr->headroom  += len;
>> > +     pkt_hdr->frame_len -= len;
>> > +     pull_offset(pkt_hdr->l2_offset, len);
>> > +     pull_offset(pkt_hdr->l3_offset, len);
>> > +     pull_offset(pkt_hdr->l4_offset, len);
>>
>> Same thing here.
>>
>> -Petri
>>
>>
>> > +}
>> > +
>> > +static inline void push_tail(odp_packet_hdr_t *pkt_hdr, size_t len)
>> > +{
>> > +     pkt_hdr->tailroom  -= len;
>> > +     pkt_hdr->frame_len += len;
>> > +}
>> > +
>> > +
>> > +static inline void pull_tail(odp_packet_hdr_t *pkt_hdr, size_t len)
>> > +{
>> > +     pkt_hdr->tailroom  += len;
>> > +     pkt_hdr->frame_len -= len;
>> > +}
>> > +
>>
>>
>>
>>
>> _______________________________________________
>> lng-odp mailing list
>> lng-odp@lists.linaro.org
>> http://lists.linaro.org/mailman/listinfo/lng-odp
>>
>
> _______________________________________________
> lng-odp mailing list
> lng-odp@lists.linaro.org
> http://lists.linaro.org/mailman/listinfo/lng-odp
>
>
Bill Fischofer Dec. 16, 2014, 1:59 p.m. UTC | #4
That would be my preference.  I already posted two patches against the
buffer code that Maxim merged earlier today.

On Tue, Dec 16, 2014 at 7:56 AM, Mike Holmes <mike.holmes@linaro.org> wrote:
>
> One question, does this have to be solved today ?
>
> I'd like to get 0.5 out today and 0.6 is next week it can take the patches
> after discussion ?
>
> On 16 December 2014 at 08:54, Bill Fischofer <bill.fischofer@linaro.org>
> wrote:
>>
>> If an application strips off an L2 header that hasn't affected the L3 or
>> L4 headers, but without these adjustments odp_packet_l3_ptr() and
>> odp_packet_l4_ptr() would have no meaning.
>>
>> Similarly, if an application pushes a shim header onto a packet, again it
>> hasn't affected the validity of whatever headers are part of the packet,
>> but again the accessor functions for these other headers would no longer be
>> pointing to the correct places in the packet.
>>
>> So I'm not sure I understand the point you're making.
>>
>> On Tue, Dec 16, 2014 at 7:48 AM, Savolainen, Petri (NSN - FI/Espoo) <
>> petri.savolainen@nsn.com> wrote:
>>>
>>>  The reason is that only application knows what it’s doing to the
>>> packet. Application is likely to update the offsets anyway after the
>>> push/pull operation. Implementation would just speculate what is happening
>>> to the packet.
>>>
>>>
>>>
>>> -Petri
>>>
>>>
>>>
>>>
>>>
>>> *From:* ext Bill Fischofer [mailto:bill.fischofer@linaro.org]
>>> *Sent:* Tuesday, December 16, 2014 3:36 PM
>>> *To:* Savolainen, Petri (NSN - FI/Espoo)
>>> *Cc:* ext Taras Kondratiuk; lng-odp@lists.linaro.org
>>> *Subject:* Re: [lng-odp] [PATCHv5 8/9] api: packet: add head/tail room
>>> manipulation
>>>
>>>
>>>
>>> Taras is just repackaging the v4 patch from December 12th.
>>>
>>>
>>>
>>> We can certainly consider this a trivial bug-fix if needed. However I
>>> would like to understand what the advantage of not making these adjustments
>>> as part of these operations since without them after a push_head()
>>> operation all off the offsets will be incorrect.  Is this some sort of
>>> performance concern or is there are reason why it's preferable to ask every
>>> application take responsibility for these adjustments as opposed to letting
>>> the implementation do them?
>>>
>>>
>>>
>>> I ask this because in SoCs that maintain this sort of meta data in HW
>>> would need special additional SW to "undo" these adjustments if that's a
>>> required feature of the ODP API.
>>>
>>>
>>>
>>> On Tue, Dec 16, 2014 at 7:13 AM, Savolainen, Petri (NSN - FI/Espoo) <
>>> petri.savolainen@nsn.com> wrote:
>>>
>>> > diff --git a/platform/linux-generic/include/odp_packet_internal.h
>>> > b/platform/linux-generic/include/odp_packet_internal.h
>>> > index 0b24300..bd5daf0 100644
>>> > --- a/platform/linux-generic/include/odp_packet_internal.h
>>> > +++ b/platform/linux-generic/include/odp_packet_internal.h
>>> > @@ -188,6 +188,39 @@ static inline void *packet_map(odp_packet_hdr_t
>>> > *pkt_hdr,
>>> >                         pkt_hdr->headroom + pkt_hdr->frame_len);
>>> >  }
>>> >
>>> > +#define pull_offset(x, len) (x = x < len ? 0 : x - len)
>>> > +
>>> > +static inline void push_head(odp_packet_hdr_t *pkt_hdr, size_t len)
>>> > +{
>>> > +     pkt_hdr->headroom  -= len;
>>> > +     pkt_hdr->frame_len += len;
>>> > +     pkt_hdr->l2_offset += len;
>>> > +     pkt_hdr->l3_offset += len;
>>> > +     pkt_hdr->l4_offset += len;
>>>
>>> "User is responsible to update packet meta-data offsets when needed."
>>>
>>> This operation must not modify L2/l3/l4 offsets.
>>>
>>>
>>> > +}
>>> > +
>>> > +static inline void pull_head(odp_packet_hdr_t *pkt_hdr, size_t len)
>>> > +{
>>> > +     pkt_hdr->headroom  += len;
>>> > +     pkt_hdr->frame_len -= len;
>>> > +     pull_offset(pkt_hdr->l2_offset, len);
>>> > +     pull_offset(pkt_hdr->l3_offset, len);
>>> > +     pull_offset(pkt_hdr->l4_offset, len);
>>>
>>> Same thing here.
>>>
>>> -Petri
>>>
>>>
>>> > +}
>>> > +
>>> > +static inline void push_tail(odp_packet_hdr_t *pkt_hdr, size_t len)
>>> > +{
>>> > +     pkt_hdr->tailroom  -= len;
>>> > +     pkt_hdr->frame_len += len;
>>> > +}
>>> > +
>>> > +
>>> > +static inline void pull_tail(odp_packet_hdr_t *pkt_hdr, size_t len)
>>> > +{
>>> > +     pkt_hdr->tailroom  += len;
>>> > +     pkt_hdr->frame_len -= len;
>>> > +}
>>> > +
>>>
>>>
>>>
>>>
>>> _______________________________________________
>>> lng-odp mailing list
>>> lng-odp@lists.linaro.org
>>> http://lists.linaro.org/mailman/listinfo/lng-odp
>>>
>>
>> _______________________________________________
>> lng-odp mailing list
>> lng-odp@lists.linaro.org
>> http://lists.linaro.org/mailman/listinfo/lng-odp
>>
>>
>
> --
> *Mike Holmes*
> Linaro  Sr Technical Manager
> LNG - ODP
>
Balasubramanian Manoharan Dec. 17, 2014, 8:15 a.m. UTC | #5
Hi,

 From the implementation point of view it cannot predict in which part 
the application is pushing the new header either in L2, L3 or L4. Hence 
the application should take the responsibility to update the offsets.
The implementation should only update the available headroom value.

Regards,
Bala

On 16/12/14 7:24 pm, Bill Fischofer wrote:
> If an application strips off an L2 header that hasn't affected the L3 
> or L4 headers, but without these adjustments odp_packet_l3_ptr() and 
> odp_packet_l4_ptr() would have no meaning.
>
> Similarly, if an application pushes a shim header onto a packet, again 
> it hasn't affected the validity of whatever headers are part of the 
> packet, but again the accessor functions for these other headers would 
> no longer be pointing to the correct places in the packet.
>
> So I'm not sure I understand the point you're making.
>
> On Tue, Dec 16, 2014 at 7:48 AM, Savolainen, Petri (NSN - FI/Espoo) 
> <petri.savolainen@nsn.com <mailto:petri.savolainen@nsn.com>> wrote:
>
>     The reason is that only application knows what it’s doing to the
>     packet. Application is likely to update the offsets anyway after
>     the push/pull operation. Implementation would just speculate what
>     is happening to the packet.
>
>     -Petri
>
>     *From:*ext Bill Fischofer [mailto:bill.fischofer@linaro.org
>     <mailto:bill.fischofer@linaro.org>]
>     *Sent:* Tuesday, December 16, 2014 3:36 PM
>     *To:* Savolainen, Petri (NSN - FI/Espoo)
>     *Cc:* ext Taras Kondratiuk; lng-odp@lists.linaro.org
>     <mailto:lng-odp@lists.linaro.org>
>     *Subject:* Re: [lng-odp] [PATCHv5 8/9] api: packet: add head/tail
>     room manipulation
>
>     Taras is just repackaging the v4 patch from December 12th.
>
>     We can certainly consider this a trivial bug-fix if needed.
>     However I would like to understand what the advantage of not
>     making these adjustments as part of these operations since without
>     them after a push_head() operation all off the offsets will be
>     incorrect.  Is this some sort of performance concern or is there
>     are reason why it's preferable to ask every application take
>     responsibility for these adjustments as opposed to letting the
>     implementation do them?
>
>     I ask this because in SoCs that maintain this sort of meta data in
>     HW would need special additional SW to "undo" these adjustments if
>     that's a required feature of the ODP API.
>
>     On Tue, Dec 16, 2014 at 7:13 AM, Savolainen, Petri (NSN -
>     FI/Espoo) <petri.savolainen@nsn.com
>     <mailto:petri.savolainen@nsn.com>> wrote:
>
>     > diff --git a/platform/linux-generic/include/odp_packet_internal.h
>     > b/platform/linux-generic/include/odp_packet_internal.h
>     > index 0b24300..bd5daf0 100644
>     > --- a/platform/linux-generic/include/odp_packet_internal.h
>     > +++ b/platform/linux-generic/include/odp_packet_internal.h
>     > @@ -188,6 +188,39 @@ static inline void *packet_map(odp_packet_hdr_t
>     > *pkt_hdr,
>     >  pkt_hdr->headroom + pkt_hdr->frame_len);
>     >  }
>     >
>     > +#define pull_offset(x, len) (x = x < len ? 0 : x - len)
>     > +
>     > +static inline void push_head(odp_packet_hdr_t *pkt_hdr, size_t len)
>     > +{
>     > +     pkt_hdr->headroom  -= len;
>     > +     pkt_hdr->frame_len += len;
>     > +     pkt_hdr->l2_offset += len;
>     > +     pkt_hdr->l3_offset += len;
>     > +     pkt_hdr->l4_offset += len;
>
>     "User is responsible to update packet meta-data offsets when needed."
>
>     This operation must not modify L2/l3/l4 offsets.
>
>
>     > +}
>     > +
>     > +static inline void pull_head(odp_packet_hdr_t *pkt_hdr, size_t len)
>     > +{
>     > +     pkt_hdr->headroom  += len;
>     > +     pkt_hdr->frame_len -= len;
>     > +  pull_offset(pkt_hdr->l2_offset, len);
>     > +  pull_offset(pkt_hdr->l3_offset, len);
>     > +  pull_offset(pkt_hdr->l4_offset, len);
>
>     Same thing here.
>
>     -Petri
>
>
>     > +}
>     > +
>     > +static inline void push_tail(odp_packet_hdr_t *pkt_hdr, size_t len)
>     > +{
>     > +     pkt_hdr->tailroom  -= len;
>     > +     pkt_hdr->frame_len += len;
>     > +}
>     > +
>     > +
>     > +static inline void pull_tail(odp_packet_hdr_t *pkt_hdr, size_t len)
>     > +{
>     > +     pkt_hdr->tailroom  += len;
>     > +     pkt_hdr->frame_len -= len;
>     > +}
>     > +
>
>
>
>
>     _______________________________________________
>     lng-odp mailing list
>     lng-odp@lists.linaro.org <mailto:lng-odp@lists.linaro.org>
>     http://lists.linaro.org/mailman/listinfo/lng-odp
>
>
>
> _______________________________________________
> lng-odp mailing list
> lng-odp@lists.linaro.org
> http://lists.linaro.org/mailman/listinfo/lng-odp
Ola Liljedahl Dec. 17, 2014, 10:03 a.m. UTC | #6
Use cases, use cases, use cases...
Talking about API details in isolation will probably just make us go
round in circles.

In my view, ODP should just provide precomputed L2/L3/L4 offsets for
ingress packets as those values likely have been computed by the (HW)
parser anyway (needed by the classifier) and we want to be able to
utilize that and not perform redundant operations in SW. As packets
are processed and headers removed, the Lx offsets become irrelevant
one after one (not necessarily all of them).

When popping headers, I think the application expects to get the
correct (updated) header offsets from the remaining ODP packet
L2/L3/L4 accessors. So if some bytes are trimmed from the head of the
packet, remaining Lx offsets should remain correct (however that is
done, possibly they need to be updated because the reference point has
changed).

If an application is e.g. pushing new headers in front of a packet,
does that application then continue with looking at headers further
in? Seems unlikely, we are on the egress path now, building a new
packet (by adding headers) to be transmitted. Also some headers you
put in front might be new L2 and L3 (and L4) headers (e.g. for
tunneling), what should the ODP packet L2/L3/L4 offsets refer to,
inner or outer headers?

When are the Lx offsets important on the egress path? Does the HW (or
SW) need to look at them in order to know what areas to checksum (e.g.
IPv4, TCP or UDP checksum offload)? If the application pushed those
header on top of the payload, the application is probably also the
most suitable to set the corresponding Lx offsets for the packet.

On 17 December 2014 at 09:15, Balasubramanian Manoharan
<bala.manoharan@linaro.org> wrote:
> Hi,
>
> From the implementation point of view it cannot predict in which part the
> application is pushing the new header either in L2, L3 or L4. Hence the
> application should take the responsibility to update the offsets.
> The implementation should only update the available headroom value.
>
> Regards,
> Bala
>
>
> On 16/12/14 7:24 pm, Bill Fischofer wrote:
>
> If an application strips off an L2 header that hasn't affected the L3 or L4
> headers, but without these adjustments odp_packet_l3_ptr() and
> odp_packet_l4_ptr() would have no meaning.
>
> Similarly, if an application pushes a shim header onto a packet, again it
> hasn't affected the validity of whatever headers are part of the packet, but
> again the accessor functions for these other headers would no longer be
> pointing to the correct places in the packet.
>
> So I'm not sure I understand the point you're making.
>
> On Tue, Dec 16, 2014 at 7:48 AM, Savolainen, Petri (NSN - FI/Espoo)
> <petri.savolainen@nsn.com> wrote:
>>
>> The reason is that only application knows what it’s doing to the packet.
>> Application is likely to update the offsets anyway after the push/pull
>> operation. Implementation would just speculate what is happening to the
>> packet.
>>
>>
>>
>> -Petri
>>
>>
>>
>>
>>
>> From: ext Bill Fischofer [mailto:bill.fischofer@linaro.org]
>> Sent: Tuesday, December 16, 2014 3:36 PM
>> To: Savolainen, Petri (NSN - FI/Espoo)
>> Cc: ext Taras Kondratiuk; lng-odp@lists.linaro.org
>> Subject: Re: [lng-odp] [PATCHv5 8/9] api: packet: add head/tail room
>> manipulation
>>
>>
>>
>> Taras is just repackaging the v4 patch from December 12th.
>>
>>
>>
>> We can certainly consider this a trivial bug-fix if needed. However I
>> would like to understand what the advantage of not making these adjustments
>> as part of these operations since without them after a push_head() operation
>> all off the offsets will be incorrect.  Is this some sort of performance
>> concern or is there are reason why it's preferable to ask every application
>> take responsibility for these adjustments as opposed to letting the
>> implementation do them?
>>
>>
>>
>> I ask this because in SoCs that maintain this sort of meta data in HW
>> would need special additional SW to "undo" these adjustments if that's a
>> required feature of the ODP API.
>>
>>
>>
>> On Tue, Dec 16, 2014 at 7:13 AM, Savolainen, Petri (NSN - FI/Espoo)
>> <petri.savolainen@nsn.com> wrote:
>>
>> > diff --git a/platform/linux-generic/include/odp_packet_internal.h
>> > b/platform/linux-generic/include/odp_packet_internal.h
>> > index 0b24300..bd5daf0 100644
>> > --- a/platform/linux-generic/include/odp_packet_internal.h
>> > +++ b/platform/linux-generic/include/odp_packet_internal.h
>> > @@ -188,6 +188,39 @@ static inline void *packet_map(odp_packet_hdr_t
>> > *pkt_hdr,
>> >                         pkt_hdr->headroom + pkt_hdr->frame_len);
>> >  }
>> >
>> > +#define pull_offset(x, len) (x = x < len ? 0 : x - len)
>> > +
>> > +static inline void push_head(odp_packet_hdr_t *pkt_hdr, size_t len)
>> > +{
>> > +     pkt_hdr->headroom  -= len;
>> > +     pkt_hdr->frame_len += len;
>> > +     pkt_hdr->l2_offset += len;
>> > +     pkt_hdr->l3_offset += len;
>> > +     pkt_hdr->l4_offset += len;
>>
>> "User is responsible to update packet meta-data offsets when needed."
>>
>> This operation must not modify L2/l3/l4 offsets.
>>
>>
>> > +}
>> > +
>> > +static inline void pull_head(odp_packet_hdr_t *pkt_hdr, size_t len)
>> > +{
>> > +     pkt_hdr->headroom  += len;
>> > +     pkt_hdr->frame_len -= len;
>> > +     pull_offset(pkt_hdr->l2_offset, len);
>> > +     pull_offset(pkt_hdr->l3_offset, len);
>> > +     pull_offset(pkt_hdr->l4_offset, len);
>>
>> Same thing here.
>>
>> -Petri
>>
>>
>> > +}
>> > +
>> > +static inline void push_tail(odp_packet_hdr_t *pkt_hdr, size_t len)
>> > +{
>> > +     pkt_hdr->tailroom  -= len;
>> > +     pkt_hdr->frame_len += len;
>> > +}
>> > +
>> > +
>> > +static inline void pull_tail(odp_packet_hdr_t *pkt_hdr, size_t len)
>> > +{
>> > +     pkt_hdr->tailroom  += len;
>> > +     pkt_hdr->frame_len -= len;
>> > +}
>> > +
>>
>>
>>
>>
>> _______________________________________________
>> lng-odp mailing list
>> lng-odp@lists.linaro.org
>> http://lists.linaro.org/mailman/listinfo/lng-odp
>
>
>
> _______________________________________________
> lng-odp mailing list
> lng-odp@lists.linaro.org
> http://lists.linaro.org/mailman/listinfo/lng-odp
>
>
>
> _______________________________________________
> lng-odp mailing list
> lng-odp@lists.linaro.org
> http://lists.linaro.org/mailman/listinfo/lng-odp
>
diff mbox

Patch

diff --git a/example/ipsec/odp_ipsec.c b/example/ipsec/odp_ipsec.c
index ae04a4e..223ca61 100644
--- a/example/ipsec/odp_ipsec.c
+++ b/example/ipsec/odp_ipsec.c
@@ -807,10 +807,10 @@  pkt_disposition_e do_ipsec_in_finish(odp_packet_t pkt,
 	odph_ipv4_csum_update(pkt);
 
 	/* Correct the packet length and move payload into position */
-	odp_packet_set_len(pkt, odp_packet_get_len(pkt) - (hdr_len + trl_len));
 	memmove(ipv4_data_p(ip),
 		ipv4_data_p(ip) + hdr_len,
 		odp_be_to_cpu_16(ip->tot_len));
+	odp_packet_pull_tail(pkt, hdr_len + trl_len);
 
 	/* Fall through to next state */
 	return PKT_CONTINUE;
@@ -924,7 +924,7 @@  pkt_disposition_e do_ipsec_out_classify(odp_packet_t pkt,
 
 	/* Set IPv4 length before authentication */
 	ipv4_adjust_len(ip, hdr_len + trl_len);
-	odp_packet_set_len(pkt, odp_packet_get_len(pkt) + (hdr_len + trl_len));
+	odp_packet_push_tail(pkt, hdr_len + trl_len);
 
 	/* Save remaining context */
 	ctx->ipsec.hdr_len = hdr_len;
diff --git a/example/ipsec/odp_ipsec_stream.c b/example/ipsec/odp_ipsec_stream.c
index 93de198..1e932df 100644
--- a/example/ipsec/odp_ipsec_stream.c
+++ b/example/ipsec/odp_ipsec_stream.c
@@ -302,7 +302,7 @@  odp_packet_t create_ipv4_packet(stream_db_entry_t *stream,
 
 	/* Since ESP can pad we can now fix IP length */
 	ip->tot_len = odp_cpu_to_be_16(data - (uint8_t *)ip);
-	odp_packet_set_len(pkt, data - base);
+	odp_packet_push_tail(pkt, data - base);
 
 	/* Close AH if specified */
 	if (ah) {
diff --git a/platform/linux-generic/include/api/odp_packet.h b/platform/linux-generic/include/api/odp_packet.h
index cb5f177..853709d 100644
--- a/platform/linux-generic/include/api/odp_packet.h
+++ b/platform/linux-generic/include/api/odp_packet.h
@@ -113,6 +113,235 @@  odp_packet_t odp_packet_from_buffer(odp_buffer_t buf);
  */
 odp_buffer_t odp_packet_to_buffer(odp_packet_t pkt);
 
+
+/*
+ *
+ * Pointers and lengths
+ * ********************************************************
+ *
+ */
+
+/**
+ * Packet head address
+ *
+ * Returns start address of the first segment. Packet level headroom starts
+ * from here. Use odp_packet_data() or odp_packet_l2_ptr() to return the
+ * packet data start address.
+ *
+ * @param pkt  Packet handle
+ *
+ * @return Pointer to the start address of the first packet segment
+ *
+ * @see odp_packet_data(), odp_packet_l2_ptr(), odp_packet_headroom()
+ */
+void *odp_packet_head(odp_packet_t pkt);
+
+/**
+ * Total packet buffer length
+ *
+ * Returns sum of buffer lengths over all packet segments.
+ *
+ * @param pkt  Packet handle
+ *
+ * @return  Total packet buffer length in bytes
+ *
+ * @see odp_packet_reset()
+ */
+uint32_t odp_packet_buf_len(odp_packet_t pkt);
+
+/**
+ * Packet data pointer
+ *
+ * Returns the current packet data pointer. When a packet is received
+ * from packet input, this points to the first byte of the received
+ * packet. Packet level offsets are calculated relative to this position.
+ *
+ * User can adjust the data pointer with head_push/head_pull (does not modify
+ * segmentation) and add_data/rem_data calls (may modify segmentation).
+ *
+ * @param pkt  Packet handle
+ *
+ * @return  Pointer to the packet data
+ *
+ * @see odp_packet_l2_ptr(), odp_packet_seg_len()
+ */
+void *odp_packet_data(odp_packet_t pkt);
+
+/**
+ * Packet segment data length
+ *
+ * Returns number of data bytes following the current data pointer
+ * (odp_packet_data()) location in the segment.
+ *
+ * @param pkt  Packet handle
+ *
+ * @return  Segment data length in bytes (pointed by odp_packet_data())
+ *
+ * @see odp_packet_data()
+ */
+uint32_t odp_packet_seg_len(odp_packet_t pkt);
+
+/**
+ * Packet data length
+ *
+ * Returns sum of data lengths over all packet segments.
+ *
+ * @param pkt  Packet handle
+ *
+ * @return Packet data length
+ */
+uint32_t odp_packet_len(odp_packet_t pkt);
+
+/**
+ * Packet headroom length
+ *
+ * Returns the current packet level headroom length.
+ *
+ * @param pkt  Packet handle
+ *
+ * @return Headroom length
+ */
+uint32_t odp_packet_headroom(odp_packet_t pkt);
+
+/**
+ * Packet tailroom length
+ *
+ * Returns the current packet level tailroom length.
+ *
+ * @param pkt  Packet handle
+ *
+ * @return Tailroom length
+ */
+uint32_t odp_packet_tailroom(odp_packet_t pkt);
+
+/**
+ * Packet tailroom pointer
+ *
+ * Returns pointer to the start of the current packet level tailroom.
+ *
+ * User can adjust the tail pointer with tail_push/tail_pull (does not modify
+ * segmentation) and add_data/rem_data calls (may modify segmentation).
+ *
+ * @param pkt  Packet handle
+ *
+ * @return  Tailroom pointer
+ *
+ * @see odp_packet_tailroom()
+ */
+void *odp_packet_tail(odp_packet_t pkt);
+
+/**
+ * Push out packet head
+ *
+ * Increase packet data length by moving packet head into packet headroom.
+ * Packet headroom is decreased with the same amount. The packet head may be
+ * pushed out up to 'headroom' bytes. Packet is not modified if there's not
+ * enough headroom space.
+ *
+ * odp_packet_xxx:
+ * seg_len  += len
+ * len      += len
+ * headroom -= len
+ * data     -= len
+ *
+ * Operation does not modify packet segmentation or move data. Handles and
+ * pointers remain valid. User is responsible to update packet meta-data
+ * offsets when needed.
+ *
+ * @param pkt  Packet handle
+ * @param len  Number of bytes to push the head (0 ... headroom)
+ *
+ * @return The new data pointer
+ * @retval NULL  Requested offset exceeds available headroom
+ *
+ * @see odp_packet_headroom(), odp_packet_pull_head()
+ */
+void *odp_packet_push_head(odp_packet_t pkt, uint32_t len);
+
+/**
+ * Pull in packet head
+ *
+ * Decrease packet data length by removing data from the head of the packet.
+ * Packet headroom is increased with the same amount. Packet head may be pulled
+ * in up to seg_len - 1 bytes (i.e. packet data pointer must stay in the
+ * first segment). Packet is not modified if there's not enough data.
+ *
+ * odp_packet_xxx:
+ * seg_len  -= len
+ * len      -= len
+ * headroom += len
+ * data     += len
+ *
+ * Operation does not modify packet segmentation or move data. Handles and
+ * pointers remain valid. User is responsible to update packet meta-data
+ * offsets when needed.
+ *
+ * @param pkt  Packet handle
+ * @param len  Number of bytes to pull the head (0 ... seg_len - 1)
+ *
+ * @return The new data pointer, or NULL in case of an error.
+ * @retval NULL  Requested offset exceeds packet segment length
+ *
+ * @see odp_packet_seg_len(), odp_packet_push_head()
+ */
+void *odp_packet_pull_head(odp_packet_t pkt, uint32_t len);
+
+/**
+ * Push out packet tail
+ *
+ * Increase packet data length by moving packet tail into packet tailroom.
+ * Packet tailroom is decreased with the same amount. The packet tail may be
+ * pushed out up to 'tailroom' bytes. Packet is not modified if there's not
+ * enough tailroom.
+ *
+ * last_seg:
+ * data_len += len
+ *
+ * odp_packet_xxx:
+ * len      += len
+ * tail     += len
+ * tailroom -= len
+ *
+ * Operation does not modify packet segmentation or move data. Handles,
+ * pointers and offsets remain valid.
+ *
+ * @param pkt  Packet handle
+ * @param len  Number of bytes to push the tail (0 ... tailroom)
+ *
+ * @return The old tail pointer
+ * @retval NULL  Requested offset exceeds available tailroom
+ *
+ * @see odp_packet_tailroom(), odp_packet_pull_tail()
+ */
+void *odp_packet_push_tail(odp_packet_t pkt, uint32_t len);
+
+/**
+ * Pull in packet tail
+ *
+ * Decrease packet data length by removing data from the tail of the packet.
+ * Packet tailroom is increased with the same amount. Packet tail may be pulled
+ * in up to last segment data_len - 1 bytes. (i.e. packet tail must stay in the
+ * last segment). Packet is not modified if there's not enough data.
+ *
+ * last_seg:
+ * data_len -= len
+ *
+ * odp_packet_xxx:
+ * len      -= len
+ * tail     -= len
+ * tailroom += len
+ *
+ * Operation does not modify packet segmentation or move data. Handles and
+ * pointers remain valid. User is responsible to update packet meta-data
+ * offsets when needed.
+ *
+ * @param pkt  Packet handle
+ * @param len  Number of bytes to pull the tail (0 ... last_seg:data_len - 1)
+ *
+ * @return The new tail pointer
+ * @retval NULL  The specified offset exceeds allowable data length
+ */
+void *odp_packet_pull_tail(odp_packet_t pkt, uint32_t len);
 /**
  * Set the packet length
  *
@@ -177,52 +406,6 @@  uint64_t odp_packet_user_u64(odp_packet_t pkt);
 void odp_packet_user_u64_set(odp_packet_t pkt, uint64_t ctx);
 
 /**
- * Packet buffer start address
- *
- * Returns a pointer to the start of the packet buffer. The address is not
- * necessarily the same as packet data address. E.g. on a received Ethernet
- * frame, the protocol header may start 2 or 6 bytes within the buffer to
- * ensure 32 or 64-bit alignment of the IP header.
- *
- * Use odp_packet_l2(pkt) to get the start address of a received valid frame
- * or odp_packet_data(pkt) to get the current packet data address.
- *
- * @param pkt  Packet handle
- *
- * @return  Pointer to the start of the packet buffer
- *
- * @see odp_packet_l2(), odp_packet_data()
- */
-uint8_t *odp_packet_addr(odp_packet_t pkt);
-
-/**
- * Packet buffer maximum data size
- *
- * @note odp_packet_buf_size(pkt) != odp_packet_get_len(pkt), the former returns
- *       the max length of the buffer, the latter the size of a received packet.
- *
- * @param pkt  Packet handle
- *
- * @return Packet buffer maximum data size
- */
-size_t odp_packet_buf_size(odp_packet_t pkt);
-
-/**
- * Packet data pointer
- *
- * Returns the current packet data pointer. When a packet is received
- * from packet input, this points to the first byte of the received
- * packet. Packet level offsets are calculated relative to this position.
- *
- * @param pkt  Packet handle
- *
- * @return  Pointer to the packet data
- *
- * @see odp_packet_l2(), odp_packet_addr()
- */
-void *odp_packet_data(odp_packet_t pkt);
-
-/**
  * Layer 2 start pointer
  *
  * Returns pointer to the start of the layer 2 header. Optionally, outputs
diff --git a/platform/linux-generic/include/odp_packet_internal.h b/platform/linux-generic/include/odp_packet_internal.h
index 0b24300..bd5daf0 100644
--- a/platform/linux-generic/include/odp_packet_internal.h
+++ b/platform/linux-generic/include/odp_packet_internal.h
@@ -188,6 +188,39 @@  static inline void *packet_map(odp_packet_hdr_t *pkt_hdr,
 			  pkt_hdr->headroom + pkt_hdr->frame_len);
 }
 
+#define pull_offset(x, len) (x = x < len ? 0 : x - len)
+
+static inline void push_head(odp_packet_hdr_t *pkt_hdr, size_t len)
+{
+	pkt_hdr->headroom  -= len;
+	pkt_hdr->frame_len += len;
+	pkt_hdr->l2_offset += len;
+	pkt_hdr->l3_offset += len;
+	pkt_hdr->l4_offset += len;
+}
+
+static inline void pull_head(odp_packet_hdr_t *pkt_hdr, size_t len)
+{
+	pkt_hdr->headroom  += len;
+	pkt_hdr->frame_len -= len;
+	pull_offset(pkt_hdr->l2_offset, len);
+	pull_offset(pkt_hdr->l3_offset, len);
+	pull_offset(pkt_hdr->l4_offset, len);
+}
+
+static inline void push_tail(odp_packet_hdr_t *pkt_hdr, size_t len)
+{
+	pkt_hdr->tailroom  -= len;
+	pkt_hdr->frame_len += len;
+}
+
+
+static inline void pull_tail(odp_packet_hdr_t *pkt_hdr, size_t len)
+{
+	pkt_hdr->tailroom  += len;
+	pkt_hdr->frame_len -= len;
+}
+
 static inline void packet_set_len(odp_packet_t pkt, uint32_t len)
 {
 	odp_packet_hdr(pkt)->frame_len = len;
diff --git a/platform/linux-generic/odp_packet.c b/platform/linux-generic/odp_packet.c
index ff05849..9f18540 100644
--- a/platform/linux-generic/odp_packet.c
+++ b/platform/linux-generic/odp_packet.c
@@ -78,28 +78,106 @@  odp_buffer_t odp_packet_to_buffer(odp_packet_t pkt)
 	return (odp_buffer_t)pkt;
 }
 
-void odp_packet_set_len(odp_packet_t pkt, size_t len)
+/*
+ *
+ * Pointers and lengths
+ * ********************************************************
+ *
+ */
+
+void *odp_packet_head(odp_packet_t pkt)
+{
+	odp_packet_hdr_t *pkt_hdr = odp_packet_hdr(pkt);
+	return buffer_map(&pkt_hdr->buf_hdr, 0, NULL, 0);
+}
+
+uint32_t odp_packet_buf_len(odp_packet_t pkt)
+{
+	return odp_packet_hdr(pkt)->buf_hdr.size;
+}
+
+void *odp_packet_data(odp_packet_t pkt)
 {
-	odp_packet_hdr(pkt)->frame_len = len;
+	odp_packet_hdr_t *pkt_hdr = odp_packet_hdr(pkt);
+	return packet_map(pkt_hdr, 0, NULL);
+}
+
+uint32_t odp_packet_seg_len(odp_packet_t pkt)
+{
+	odp_packet_hdr_t *pkt_hdr = odp_packet_hdr(pkt);
+	uint32_t seglen;
+
+	/* Call returns length of 1st data segment */
+	packet_map(pkt_hdr, 0, &seglen);
+	return seglen;
 }
 
-size_t odp_packet_get_len(odp_packet_t pkt)
+uint32_t odp_packet_len(odp_packet_t pkt)
 {
 	return odp_packet_hdr(pkt)->frame_len;
 }
 
-uint8_t *odp_packet_addr(odp_packet_t pkt)
+uint32_t odp_packet_headroom(odp_packet_t pkt)
+{
+	return odp_packet_hdr(pkt)->headroom;
+}
+
+uint32_t odp_packet_tailroom(odp_packet_t pkt)
+{
+	return odp_packet_hdr(pkt)->tailroom;
+}
+
+void *odp_packet_tail(odp_packet_t pkt)
 {
 	odp_packet_hdr_t *pkt_hdr = odp_packet_hdr(pkt);
-	return buffer_map(&pkt_hdr->buf_hdr, 0, NULL, 0);
+	return packet_map(pkt_hdr, pkt_hdr->frame_len, NULL);
 }
 
-void *odp_packet_data(odp_packet_t pkt)
+void *odp_packet_push_head(odp_packet_t pkt, uint32_t len)
+{
+	odp_packet_hdr_t *pkt_hdr = odp_packet_hdr(pkt);
+
+	if (len > pkt_hdr->headroom)
+		return NULL;
+
+	push_head(pkt_hdr, len);
+	return packet_map(pkt_hdr, 0, NULL);
+}
+
+void *odp_packet_pull_head(odp_packet_t pkt, uint32_t len)
 {
 	odp_packet_hdr_t *pkt_hdr = odp_packet_hdr(pkt);
+
+	if (len > pkt_hdr->frame_len)
+		return NULL;
+
+	pull_head(pkt_hdr, len);
 	return packet_map(pkt_hdr, 0, NULL);
 }
 
+void *odp_packet_push_tail(odp_packet_t pkt, uint32_t len)
+{
+	odp_packet_hdr_t *pkt_hdr = odp_packet_hdr(pkt);
+	uint32_t origin = pkt_hdr->frame_len;
+
+	if (len > pkt_hdr->tailroom)
+		return NULL;
+
+	push_tail(pkt_hdr, len);
+	return packet_map(pkt_hdr, origin, NULL);
+}
+
+void *odp_packet_pull_tail(odp_packet_t pkt, uint32_t len)
+{
+	odp_packet_hdr_t *pkt_hdr = odp_packet_hdr(pkt);
+
+	if (len > pkt_hdr->frame_len)
+		return NULL;
+
+	pull_tail(pkt_hdr, len);
+	return packet_map(pkt_hdr, pkt_hdr->frame_len, NULL);
+}
+
 void *odp_packet_user_ptr(odp_packet_t pkt)
 {
 	return odp_packet_hdr(pkt)->buf_hdr.buf_ctx;
@@ -265,13 +343,6 @@  int odp_packet_is_valid(odp_packet_t pkt)
 	return odp_buffer_is_valid(buf);
 }
 
-size_t odp_packet_buf_size(odp_packet_t pkt)
-{
-	odp_buffer_t buf = odp_packet_to_buffer(pkt);
-
-	return odp_buffer_size(buf);
-}
-
 /*
  *
  * Internal Use Routines