| Message ID | 1418753883-27568-1-git-send-email-bill.fischofer@linaro.org |
|---|---|
| State | New |
| Headers | show |
Good it describes what I'm looking at. And I thin this should be split up into three patches. patch 1, api: odp_platform_types: add ODP_PACKET_OFFSET_INVALID patch 2, linux-generic: use ODP_PACKET_OFFSET_INVALID patch 3, api: odp_packet: improve documentation all patches need to have the long description added to explain further. Cheers, Anders On 16 December 2014 at 19:18, Bill Fischofer <bill.fischofer@linaro.org> wrote: > Fix for Bug #1004. > > Signed-off-by: Bill Fischofer <bill.fischofer@linaro.org> > --- > platform/linux-generic/include/api/odp_packet.h | 7 +++++-- > .../linux-generic/include/api/odp_platform_types.h | 3 +++ > .../linux-generic/include/odp_packet_internal.h | 22 ++++++++++++++++++---- > platform/linux-generic/odp_packet.c | 10 ++++++---- > 4 files changed, 32 insertions(+), 10 deletions(-) > > diff --git a/platform/linux-generic/include/api/odp_packet.h b/platform/linux-generic/include/api/odp_packet.h > index 97c2cb6..d0f15ac 100644 > --- a/platform/linux-generic/include/api/odp_packet.h > +++ b/platform/linux-generic/include/api/odp_packet.h > @@ -469,6 +469,7 @@ void *odp_packet_l2_ptr(odp_packet_t pkt, uint32_t *len); > * @param pkt Packet handle > * > * @return Layer 2 start offset > + * @retval ODP_PACKET_OFFSET_INVALID If not found > */ > uint32_t odp_packet_l2_offset(odp_packet_t pkt); > > @@ -514,7 +515,8 @@ void *odp_packet_l3_ptr(odp_packet_t pkt, uint32_t *len); > * > * @param pkt Packet handle > * > - * @return Layer 3 start offset or ODP_PACKET_OFFSET_INVALID if not found > + * @return Layer 3 start offset > + * @retval ODP_PACKET_OFFSET_INVALID If not found > */ > uint32_t odp_packet_l3_offset(odp_packet_t pkt); > > @@ -560,7 +562,8 @@ void *odp_packet_l4_ptr(odp_packet_t pkt, uint32_t *len); > * > * @param pkt Packet handle > * > - * @return Layer 4 start offset or ODP_PACKET_OFFSET_INVALID if not found > + * @return Layer 4 start offset > + * @retval ODP_PACKET_OFFSET_INVALID If not found > */ > uint32_t odp_packet_l4_offset(odp_packet_t pkt); > > diff --git a/platform/linux-generic/include/api/odp_platform_types.h b/platform/linux-generic/include/api/odp_platform_types.h > index 2cfba87..8671b42 100644 > --- a/platform/linux-generic/include/api/odp_platform_types.h > +++ b/platform/linux-generic/include/api/odp_platform_types.h > @@ -47,6 +47,9 @@ typedef odp_buffer_t odp_packet_t; > /** Invalid packet */ > #define ODP_PACKET_INVALID ODP_BUFFER_INVALID > > +/** Invalid packet offset */ > +#define ODP_PACKET_OFFSET_INVALID (0xffffffff) > + > /** ODP packet segment */ > typedef odp_buffer_t odp_packet_seg_t; > > diff --git a/platform/linux-generic/include/odp_packet_internal.h b/platform/linux-generic/include/odp_packet_internal.h > index a0eff30..515c127 100644 > --- a/platform/linux-generic/include/odp_packet_internal.h > +++ b/platform/linux-generic/include/odp_packet_internal.h > @@ -169,6 +169,12 @@ static inline void packet_init(pool_entry_t *pool, > len = sizeof(odp_packet_hdr_t) - start_offset; > memset(start, 0, len); > > + /* Set metadata items that initialize to non-zero values */ > + pkt_hdr->l2_offset = ODP_PACKET_OFFSET_INVALID; > + pkt_hdr->l3_offset = ODP_PACKET_OFFSET_INVALID; > + pkt_hdr->l4_offset = ODP_PACKET_OFFSET_INVALID; > + pkt_hdr->payload_offset = ODP_PACKET_OFFSET_INVALID; > + > /* > * Packet headroom is set from the pool's headroom > * Packet tailroom is rounded up to fill the last > @@ -192,15 +198,23 @@ 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) > +#define pull_offset(x, len) do { \ > + if (x != ODP_PACKET_OFFSET_INVALID) \ > + x = x < len ? ODP_PACKET_OFFSET_INVALID : x - len; \ > + } while (0) > + > +#define push_offset(x, len) do { \ > + if (x != ODP_PACKET_OFFSET_INVALID) \ > + x += len; \ > + } while (0) > > 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; > + push_offset(pkt_hdr->l2_offset, len); > + push_offset(pkt_hdr->l3_offset, len); > + push_offset(pkt_hdr->l4_offset, len); > } > > static inline void pull_head(odp_packet_hdr_t *pkt_hdr, size_t len) > diff --git a/platform/linux-generic/odp_packet.c b/platform/linux-generic/odp_packet.c > index 0ab9866..65e6288 100644 > --- a/platform/linux-generic/odp_packet.c > +++ b/platform/linux-generic/odp_packet.c > @@ -776,10 +776,10 @@ int _odp_packet_parse(odp_packet_t pkt) > pkt_hdr->error_flags.all = 0; > pkt_hdr->input_flags.all = 0; > pkt_hdr->output_flags.all = 0; > - pkt_hdr->l2_offset = 0; > - pkt_hdr->l3_offset = 0; > - pkt_hdr->l4_offset = 0; > - pkt_hdr->payload_offset = 0; > + pkt_hdr->l2_offset = ODP_PACKET_OFFSET_INVALID; > + pkt_hdr->l3_offset = ODP_PACKET_OFFSET_INVALID; > + pkt_hdr->l4_offset = ODP_PACKET_OFFSET_INVALID; > + pkt_hdr->payload_offset = ODP_PACKET_OFFSET_INVALID; > pkt_hdr->vlan_s_tag = 0; > pkt_hdr->vlan_c_tag = 0; > pkt_hdr->l3_protocol = 0; > @@ -861,6 +861,7 @@ int _odp_packet_parse(odp_packet_t pkt) > > default: > pkt_hdr->input_flags.l3 = 0; > + pkt_hdr->l3_offset = ODP_PACKET_OFFSET_INVALID; > ip_proto = 255; /* Reserved invalid by IANA */ > } > > @@ -892,6 +893,7 @@ int _odp_packet_parse(odp_packet_t pkt) > > default: > pkt_hdr->input_flags.l4 = 0; > + pkt_hdr->l4_offset = ODP_PACKET_OFFSET_INVALID; > break; > } > > -- > 1.8.3.2 > > _______________________________________________ > lng-odp mailing list > lng-odp@lists.linaro.org > http://lists.linaro.org/mailman/listinfo/lng-odp
This is a trivial patch. I see no advantage to reworking it as you suggest. On Tue, Dec 16, 2014 at 5:34 PM, Anders Roxell <anders.roxell@linaro.org> wrote: > > Good it describes what I'm looking at. > And I thin this should be split up into three patches. > patch 1, api: odp_platform_types: add ODP_PACKET_OFFSET_INVALID > patch 2, linux-generic: use ODP_PACKET_OFFSET_INVALID > patch 3, api: odp_packet: improve documentation > > all patches need to have the long description added to explain further. > > Cheers, > Anders > > On 16 December 2014 at 19:18, Bill Fischofer <bill.fischofer@linaro.org> > wrote: > > Fix for Bug #1004. > > > > Signed-off-by: Bill Fischofer <bill.fischofer@linaro.org> > > --- > > platform/linux-generic/include/api/odp_packet.h | 7 +++++-- > > .../linux-generic/include/api/odp_platform_types.h | 3 +++ > > .../linux-generic/include/odp_packet_internal.h | 22 > ++++++++++++++++++---- > > platform/linux-generic/odp_packet.c | 10 ++++++---- > > 4 files changed, 32 insertions(+), 10 deletions(-) > > > > diff --git a/platform/linux-generic/include/api/odp_packet.h > b/platform/linux-generic/include/api/odp_packet.h > > index 97c2cb6..d0f15ac 100644 > > --- a/platform/linux-generic/include/api/odp_packet.h > > +++ b/platform/linux-generic/include/api/odp_packet.h > > @@ -469,6 +469,7 @@ void *odp_packet_l2_ptr(odp_packet_t pkt, uint32_t > *len); > > * @param pkt Packet handle > > * > > * @return Layer 2 start offset > > + * @retval ODP_PACKET_OFFSET_INVALID If not found > > */ > > uint32_t odp_packet_l2_offset(odp_packet_t pkt); > > > > @@ -514,7 +515,8 @@ void *odp_packet_l3_ptr(odp_packet_t pkt, uint32_t > *len); > > * > > * @param pkt Packet handle > > * > > - * @return Layer 3 start offset or ODP_PACKET_OFFSET_INVALID if not > found > > + * @return Layer 3 start offset > > + * @retval ODP_PACKET_OFFSET_INVALID If not found > > */ > > uint32_t odp_packet_l3_offset(odp_packet_t pkt); > > > > @@ -560,7 +562,8 @@ void *odp_packet_l4_ptr(odp_packet_t pkt, uint32_t > *len); > > * > > * @param pkt Packet handle > > * > > - * @return Layer 4 start offset or ODP_PACKET_OFFSET_INVALID if not > found > > + * @return Layer 4 start offset > > + * @retval ODP_PACKET_OFFSET_INVALID If not found > > */ > > uint32_t odp_packet_l4_offset(odp_packet_t pkt); > > > > diff --git a/platform/linux-generic/include/api/odp_platform_types.h > b/platform/linux-generic/include/api/odp_platform_types.h > > index 2cfba87..8671b42 100644 > > --- a/platform/linux-generic/include/api/odp_platform_types.h > > +++ b/platform/linux-generic/include/api/odp_platform_types.h > > @@ -47,6 +47,9 @@ typedef odp_buffer_t odp_packet_t; > > /** Invalid packet */ > > #define ODP_PACKET_INVALID ODP_BUFFER_INVALID > > > > +/** Invalid packet offset */ > > +#define ODP_PACKET_OFFSET_INVALID (0xffffffff) > > + > > /** ODP packet segment */ > > typedef odp_buffer_t odp_packet_seg_t; > > > > diff --git a/platform/linux-generic/include/odp_packet_internal.h > b/platform/linux-generic/include/odp_packet_internal.h > > index a0eff30..515c127 100644 > > --- a/platform/linux-generic/include/odp_packet_internal.h > > +++ b/platform/linux-generic/include/odp_packet_internal.h > > @@ -169,6 +169,12 @@ static inline void packet_init(pool_entry_t *pool, > > len = sizeof(odp_packet_hdr_t) - start_offset; > > memset(start, 0, len); > > > > + /* Set metadata items that initialize to non-zero values */ > > + pkt_hdr->l2_offset = ODP_PACKET_OFFSET_INVALID; > > + pkt_hdr->l3_offset = ODP_PACKET_OFFSET_INVALID; > > + pkt_hdr->l4_offset = ODP_PACKET_OFFSET_INVALID; > > + pkt_hdr->payload_offset = ODP_PACKET_OFFSET_INVALID; > > + > > /* > > * Packet headroom is set from the pool's headroom > > * Packet tailroom is rounded up to fill the last > > @@ -192,15 +198,23 @@ 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) > > +#define pull_offset(x, len) do { \ > > + if (x != ODP_PACKET_OFFSET_INVALID) \ > > + x = x < len ? ODP_PACKET_OFFSET_INVALID : x - len; \ > > + } while (0) > > + > > +#define push_offset(x, len) do { \ > > + if (x != ODP_PACKET_OFFSET_INVALID) \ > > + x += len; \ > > + } while (0) > > > > 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; > > + push_offset(pkt_hdr->l2_offset, len); > > + push_offset(pkt_hdr->l3_offset, len); > > + push_offset(pkt_hdr->l4_offset, len); > > } > > > > static inline void pull_head(odp_packet_hdr_t *pkt_hdr, size_t len) > > diff --git a/platform/linux-generic/odp_packet.c > b/platform/linux-generic/odp_packet.c > > index 0ab9866..65e6288 100644 > > --- a/platform/linux-generic/odp_packet.c > > +++ b/platform/linux-generic/odp_packet.c > > @@ -776,10 +776,10 @@ int _odp_packet_parse(odp_packet_t pkt) > > pkt_hdr->error_flags.all = 0; > > pkt_hdr->input_flags.all = 0; > > pkt_hdr->output_flags.all = 0; > > - pkt_hdr->l2_offset = 0; > > - pkt_hdr->l3_offset = 0; > > - pkt_hdr->l4_offset = 0; > > - pkt_hdr->payload_offset = 0; > > + pkt_hdr->l2_offset = ODP_PACKET_OFFSET_INVALID; > > + pkt_hdr->l3_offset = ODP_PACKET_OFFSET_INVALID; > > + pkt_hdr->l4_offset = ODP_PACKET_OFFSET_INVALID; > > + pkt_hdr->payload_offset = ODP_PACKET_OFFSET_INVALID; > > pkt_hdr->vlan_s_tag = 0; > > pkt_hdr->vlan_c_tag = 0; > > pkt_hdr->l3_protocol = 0; > > @@ -861,6 +861,7 @@ int _odp_packet_parse(odp_packet_t pkt) > > > > default: > > pkt_hdr->input_flags.l3 = 0; > > + pkt_hdr->l3_offset = ODP_PACKET_OFFSET_INVALID; > > ip_proto = 255; /* Reserved invalid by IANA */ > > } > > > > @@ -892,6 +893,7 @@ int _odp_packet_parse(odp_packet_t pkt) > > > > default: > > pkt_hdr->input_flags.l4 = 0; > > + pkt_hdr->l4_offset = ODP_PACKET_OFFSET_INVALID; > > break; > > } > > > > -- > > 1.8.3.2 > > > > _______________________________________________ > > lng-odp mailing list > > lng-odp@lists.linaro.org > > http://lists.linaro.org/mailman/listinfo/lng-odp >
On 17 December 2014 at 02:21, Bill Fischofer <bill.fischofer@linaro.org> wrote: > This is a trivial patch. I see no advantage to reworking it as you suggest. I agree this is a trivial patch. So if we take this step by step again! 1. if we don't split up patches it will be harder to review them. 2. if I'm only interested in API changes I want as little implementation in those patches as possible and here is an perfect example of that. 3. If I filter patches and only want to look at API patches I would miss this patch because you say linux-generic on this one! so again why do we split up patches? a. make the review process more efficient! b. make it easier to bisect! c. smaller patches less patches will be on the list for ages... e.g., the buffer and packet patch, that was split by Taras! d. external app writers are only interested in API changes and can monitor api patches and review them. Cheers, Anders > > On Tue, Dec 16, 2014 at 5:34 PM, Anders Roxell <anders.roxell@linaro.org> > wrote: >> >> Good it describes what I'm looking at. >> And I thin this should be split up into three patches. >> patch 1, api: odp_platform_types: add ODP_PACKET_OFFSET_INVALID >> patch 2, linux-generic: use ODP_PACKET_OFFSET_INVALID >> patch 3, api: odp_packet: improve documentation >> >> all patches need to have the long description added to explain further. >> >> Cheers, >> Anders >> >> On 16 December 2014 at 19:18, Bill Fischofer <bill.fischofer@linaro.org> >> wrote: >> > Fix for Bug #1004. >> > >> > Signed-off-by: Bill Fischofer <bill.fischofer@linaro.org> >> > --- >> > platform/linux-generic/include/api/odp_packet.h | 7 +++++-- >> > .../linux-generic/include/api/odp_platform_types.h | 3 +++ >> > .../linux-generic/include/odp_packet_internal.h | 22 >> > ++++++++++++++++++---- >> > platform/linux-generic/odp_packet.c | 10 ++++++---- >> > 4 files changed, 32 insertions(+), 10 deletions(-) >> > >> > diff --git a/platform/linux-generic/include/api/odp_packet.h >> > b/platform/linux-generic/include/api/odp_packet.h >> > index 97c2cb6..d0f15ac 100644 >> > --- a/platform/linux-generic/include/api/odp_packet.h >> > +++ b/platform/linux-generic/include/api/odp_packet.h >> > @@ -469,6 +469,7 @@ void *odp_packet_l2_ptr(odp_packet_t pkt, uint32_t >> > *len); >> > * @param pkt Packet handle >> > * >> > * @return Layer 2 start offset >> > + * @retval ODP_PACKET_OFFSET_INVALID If not found >> > */ >> > uint32_t odp_packet_l2_offset(odp_packet_t pkt); >> > >> > @@ -514,7 +515,8 @@ void *odp_packet_l3_ptr(odp_packet_t pkt, uint32_t >> > *len); >> > * >> > * @param pkt Packet handle >> > * >> > - * @return Layer 3 start offset or ODP_PACKET_OFFSET_INVALID if not >> > found >> > + * @return Layer 3 start offset >> > + * @retval ODP_PACKET_OFFSET_INVALID If not found >> > */ >> > uint32_t odp_packet_l3_offset(odp_packet_t pkt); >> > >> > @@ -560,7 +562,8 @@ void *odp_packet_l4_ptr(odp_packet_t pkt, uint32_t >> > *len); >> > * >> > * @param pkt Packet handle >> > * >> > - * @return Layer 4 start offset or ODP_PACKET_OFFSET_INVALID if not >> > found >> > + * @return Layer 4 start offset >> > + * @retval ODP_PACKET_OFFSET_INVALID If not found >> > */ >> > uint32_t odp_packet_l4_offset(odp_packet_t pkt); >> > >> > diff --git a/platform/linux-generic/include/api/odp_platform_types.h >> > b/platform/linux-generic/include/api/odp_platform_types.h >> > index 2cfba87..8671b42 100644 >> > --- a/platform/linux-generic/include/api/odp_platform_types.h >> > +++ b/platform/linux-generic/include/api/odp_platform_types.h >> > @@ -47,6 +47,9 @@ typedef odp_buffer_t odp_packet_t; >> > /** Invalid packet */ >> > #define ODP_PACKET_INVALID ODP_BUFFER_INVALID >> > >> > +/** Invalid packet offset */ >> > +#define ODP_PACKET_OFFSET_INVALID (0xffffffff) >> > + >> > /** ODP packet segment */ >> > typedef odp_buffer_t odp_packet_seg_t; >> > >> > diff --git a/platform/linux-generic/include/odp_packet_internal.h >> > b/platform/linux-generic/include/odp_packet_internal.h >> > index a0eff30..515c127 100644 >> > --- a/platform/linux-generic/include/odp_packet_internal.h >> > +++ b/platform/linux-generic/include/odp_packet_internal.h >> > @@ -169,6 +169,12 @@ static inline void packet_init(pool_entry_t *pool, >> > len = sizeof(odp_packet_hdr_t) - start_offset; >> > memset(start, 0, len); >> > >> > + /* Set metadata items that initialize to non-zero values */ >> > + pkt_hdr->l2_offset = ODP_PACKET_OFFSET_INVALID; >> > + pkt_hdr->l3_offset = ODP_PACKET_OFFSET_INVALID; >> > + pkt_hdr->l4_offset = ODP_PACKET_OFFSET_INVALID; >> > + pkt_hdr->payload_offset = ODP_PACKET_OFFSET_INVALID; >> > + >> > /* >> > * Packet headroom is set from the pool's headroom >> > * Packet tailroom is rounded up to fill the last >> > @@ -192,15 +198,23 @@ 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) >> > +#define pull_offset(x, len) do { \ >> > + if (x != ODP_PACKET_OFFSET_INVALID) \ >> > + x = x < len ? ODP_PACKET_OFFSET_INVALID : x - len; \ >> > + } while (0) >> > + >> > +#define push_offset(x, len) do { \ >> > + if (x != ODP_PACKET_OFFSET_INVALID) \ >> > + x += len; \ >> > + } while (0) >> > >> > 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; >> > + push_offset(pkt_hdr->l2_offset, len); >> > + push_offset(pkt_hdr->l3_offset, len); >> > + push_offset(pkt_hdr->l4_offset, len); >> > } >> > >> > static inline void pull_head(odp_packet_hdr_t *pkt_hdr, size_t len) >> > diff --git a/platform/linux-generic/odp_packet.c >> > b/platform/linux-generic/odp_packet.c >> > index 0ab9866..65e6288 100644 >> > --- a/platform/linux-generic/odp_packet.c >> > +++ b/platform/linux-generic/odp_packet.c >> > @@ -776,10 +776,10 @@ int _odp_packet_parse(odp_packet_t pkt) >> > pkt_hdr->error_flags.all = 0; >> > pkt_hdr->input_flags.all = 0; >> > pkt_hdr->output_flags.all = 0; >> > - pkt_hdr->l2_offset = 0; >> > - pkt_hdr->l3_offset = 0; >> > - pkt_hdr->l4_offset = 0; >> > - pkt_hdr->payload_offset = 0; >> > + pkt_hdr->l2_offset = ODP_PACKET_OFFSET_INVALID; >> > + pkt_hdr->l3_offset = ODP_PACKET_OFFSET_INVALID; >> > + pkt_hdr->l4_offset = ODP_PACKET_OFFSET_INVALID; >> > + pkt_hdr->payload_offset = ODP_PACKET_OFFSET_INVALID; >> > pkt_hdr->vlan_s_tag = 0; >> > pkt_hdr->vlan_c_tag = 0; >> > pkt_hdr->l3_protocol = 0; >> > @@ -861,6 +861,7 @@ int _odp_packet_parse(odp_packet_t pkt) >> > >> > default: >> > pkt_hdr->input_flags.l3 = 0; >> > + pkt_hdr->l3_offset = ODP_PACKET_OFFSET_INVALID; >> > ip_proto = 255; /* Reserved invalid by IANA */ >> > } >> > >> > @@ -892,6 +893,7 @@ int _odp_packet_parse(odp_packet_t pkt) >> > >> > default: >> > pkt_hdr->input_flags.l4 = 0; >> > + pkt_hdr->l4_offset = ODP_PACKET_OFFSET_INVALID; >> > break; >> > } >> > >> > -- >> > 1.8.3.2 >> > >> > _______________________________________________ >> > lng-odp mailing list >> > lng-odp@lists.linaro.org >> > http://lists.linaro.org/mailman/listinfo/lng-odp
You clearly read and understood the patch as written to be able to write the above. If you wish to re-write the patch you're free to do so but it is making a single logical and self-contained change and hence is properly scoped. On Wed, Dec 17, 2014 at 2:04 AM, Anders Roxell <anders.roxell@linaro.org> wrote: > > On 17 December 2014 at 02:21, Bill Fischofer <bill.fischofer@linaro.org> > wrote: > > This is a trivial patch. I see no advantage to reworking it as you > suggest. > > I agree this is a trivial patch. > > So if we take this step by step again! > > 1. if we don't split up patches it will be harder to review them. > 2. if I'm only interested in API changes I want as little > implementation in those patches as possible and here > is an perfect example of that. > 3. If I filter patches and only want to look at API patches I would > miss this patch because you say linux-generic on this one! > > so again why do we split up patches? > a. make the review process more efficient! > b. make it easier to bisect! > c. smaller patches less patches will be on the list for ages... e.g., > the buffer and packet patch, that was split by Taras! > d. external app writers are only interested in API changes and can > monitor api patches and review them. > > > Cheers, > Anders > > > > > On Tue, Dec 16, 2014 at 5:34 PM, Anders Roxell <anders.roxell@linaro.org > > > > wrote: > >> > >> Good it describes what I'm looking at. > >> And I thin this should be split up into three patches. > >> patch 1, api: odp_platform_types: add ODP_PACKET_OFFSET_INVALID > >> patch 2, linux-generic: use ODP_PACKET_OFFSET_INVALID > >> patch 3, api: odp_packet: improve documentation > >> > >> all patches need to have the long description added to explain further. > >> > >> Cheers, > >> Anders > >> > >> On 16 December 2014 at 19:18, Bill Fischofer <bill.fischofer@linaro.org > > > >> wrote: > >> > Fix for Bug #1004. > >> > > >> > Signed-off-by: Bill Fischofer <bill.fischofer@linaro.org> > >> > --- > >> > platform/linux-generic/include/api/odp_packet.h | 7 +++++-- > >> > .../linux-generic/include/api/odp_platform_types.h | 3 +++ > >> > .../linux-generic/include/odp_packet_internal.h | 22 > >> > ++++++++++++++++++---- > >> > platform/linux-generic/odp_packet.c | 10 ++++++---- > >> > 4 files changed, 32 insertions(+), 10 deletions(-) > >> > > >> > diff --git a/platform/linux-generic/include/api/odp_packet.h > >> > b/platform/linux-generic/include/api/odp_packet.h > >> > index 97c2cb6..d0f15ac 100644 > >> > --- a/platform/linux-generic/include/api/odp_packet.h > >> > +++ b/platform/linux-generic/include/api/odp_packet.h > >> > @@ -469,6 +469,7 @@ void *odp_packet_l2_ptr(odp_packet_t pkt, uint32_t > >> > *len); > >> > * @param pkt Packet handle > >> > * > >> > * @return Layer 2 start offset > >> > + * @retval ODP_PACKET_OFFSET_INVALID If not found > >> > */ > >> > uint32_t odp_packet_l2_offset(odp_packet_t pkt); > >> > > >> > @@ -514,7 +515,8 @@ void *odp_packet_l3_ptr(odp_packet_t pkt, uint32_t > >> > *len); > >> > * > >> > * @param pkt Packet handle > >> > * > >> > - * @return Layer 3 start offset or ODP_PACKET_OFFSET_INVALID if not > >> > found > >> > + * @return Layer 3 start offset > >> > + * @retval ODP_PACKET_OFFSET_INVALID If not found > >> > */ > >> > uint32_t odp_packet_l3_offset(odp_packet_t pkt); > >> > > >> > @@ -560,7 +562,8 @@ void *odp_packet_l4_ptr(odp_packet_t pkt, uint32_t > >> > *len); > >> > * > >> > * @param pkt Packet handle > >> > * > >> > - * @return Layer 4 start offset or ODP_PACKET_OFFSET_INVALID if not > >> > found > >> > + * @return Layer 4 start offset > >> > + * @retval ODP_PACKET_OFFSET_INVALID If not found > >> > */ > >> > uint32_t odp_packet_l4_offset(odp_packet_t pkt); > >> > > >> > diff --git a/platform/linux-generic/include/api/odp_platform_types.h > >> > b/platform/linux-generic/include/api/odp_platform_types.h > >> > index 2cfba87..8671b42 100644 > >> > --- a/platform/linux-generic/include/api/odp_platform_types.h > >> > +++ b/platform/linux-generic/include/api/odp_platform_types.h > >> > @@ -47,6 +47,9 @@ typedef odp_buffer_t odp_packet_t; > >> > /** Invalid packet */ > >> > #define ODP_PACKET_INVALID ODP_BUFFER_INVALID > >> > > >> > +/** Invalid packet offset */ > >> > +#define ODP_PACKET_OFFSET_INVALID (0xffffffff) > >> > + > >> > /** ODP packet segment */ > >> > typedef odp_buffer_t odp_packet_seg_t; > >> > > >> > diff --git a/platform/linux-generic/include/odp_packet_internal.h > >> > b/platform/linux-generic/include/odp_packet_internal.h > >> > index a0eff30..515c127 100644 > >> > --- a/platform/linux-generic/include/odp_packet_internal.h > >> > +++ b/platform/linux-generic/include/odp_packet_internal.h > >> > @@ -169,6 +169,12 @@ static inline void packet_init(pool_entry_t > *pool, > >> > len = sizeof(odp_packet_hdr_t) - start_offset; > >> > memset(start, 0, len); > >> > > >> > + /* Set metadata items that initialize to non-zero values */ > >> > + pkt_hdr->l2_offset = ODP_PACKET_OFFSET_INVALID; > >> > + pkt_hdr->l3_offset = ODP_PACKET_OFFSET_INVALID; > >> > + pkt_hdr->l4_offset = ODP_PACKET_OFFSET_INVALID; > >> > + pkt_hdr->payload_offset = ODP_PACKET_OFFSET_INVALID; > >> > + > >> > /* > >> > * Packet headroom is set from the pool's headroom > >> > * Packet tailroom is rounded up to fill the last > >> > @@ -192,15 +198,23 @@ 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) > >> > +#define pull_offset(x, len) do { \ > >> > + if (x != ODP_PACKET_OFFSET_INVALID) \ > >> > + x = x < len ? ODP_PACKET_OFFSET_INVALID : x - len; \ > >> > + } while (0) > >> > + > >> > +#define push_offset(x, len) do { \ > >> > + if (x != ODP_PACKET_OFFSET_INVALID) \ > >> > + x += len; \ > >> > + } while (0) > >> > > >> > 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; > >> > + push_offset(pkt_hdr->l2_offset, len); > >> > + push_offset(pkt_hdr->l3_offset, len); > >> > + push_offset(pkt_hdr->l4_offset, len); > >> > } > >> > > >> > static inline void pull_head(odp_packet_hdr_t *pkt_hdr, size_t len) > >> > diff --git a/platform/linux-generic/odp_packet.c > >> > b/platform/linux-generic/odp_packet.c > >> > index 0ab9866..65e6288 100644 > >> > --- a/platform/linux-generic/odp_packet.c > >> > +++ b/platform/linux-generic/odp_packet.c > >> > @@ -776,10 +776,10 @@ int _odp_packet_parse(odp_packet_t pkt) > >> > pkt_hdr->error_flags.all = 0; > >> > pkt_hdr->input_flags.all = 0; > >> > pkt_hdr->output_flags.all = 0; > >> > - pkt_hdr->l2_offset = 0; > >> > - pkt_hdr->l3_offset = 0; > >> > - pkt_hdr->l4_offset = 0; > >> > - pkt_hdr->payload_offset = 0; > >> > + pkt_hdr->l2_offset = ODP_PACKET_OFFSET_INVALID; > >> > + pkt_hdr->l3_offset = ODP_PACKET_OFFSET_INVALID; > >> > + pkt_hdr->l4_offset = ODP_PACKET_OFFSET_INVALID; > >> > + pkt_hdr->payload_offset = ODP_PACKET_OFFSET_INVALID; > >> > pkt_hdr->vlan_s_tag = 0; > >> > pkt_hdr->vlan_c_tag = 0; > >> > pkt_hdr->l3_protocol = 0; > >> > @@ -861,6 +861,7 @@ int _odp_packet_parse(odp_packet_t pkt) > >> > > >> > default: > >> > pkt_hdr->input_flags.l3 = 0; > >> > + pkt_hdr->l3_offset = ODP_PACKET_OFFSET_INVALID; > >> > ip_proto = 255; /* Reserved invalid by IANA */ > >> > } > >> > > >> > @@ -892,6 +893,7 @@ int _odp_packet_parse(odp_packet_t pkt) > >> > > >> > default: > >> > pkt_hdr->input_flags.l4 = 0; > >> > + pkt_hdr->l4_offset = ODP_PACKET_OFFSET_INVALID; > >> > break; > >> > } > >> > > >> > -- > >> > 1.8.3.2 > >> > > >> > _______________________________________________ > >> > lng-odp mailing list > >> > lng-odp@lists.linaro.org > >> > http://lists.linaro.org/mailman/listinfo/lng-odp >
I think this is a good part of the spec and the omission in the code is indeed a bug that should be corrected. If we couldn't find an L3 header, for example, then what does odp_packet_l3_ptr() return? Without this fix it returns the starting address of the packet, which is incorrect. With this fix it will return NULL. On Wed, Dec 17, 2014 at 6:38 AM, Savolainen, Petri (NSN - FI/Espoo) < petri.savolainen@nsn.com> wrote: > > We could take a timeout on this. Maybe it's better to specify that > l2/3/4_ptr and _offsets are valid only when odp_packet_has_l2/3/4() returns > 1. > > We wouldn't need to define ODP_PACKET_OFFSET_INVALID or NULL return values > for those functions. > > > -Petri > > > > -----Original Message----- > > From: lng-odp-bounces@lists.linaro.org [mailto:lng-odp- > > bounces@lists.linaro.org] On Behalf Of ext Bill Fischofer > > Sent: Tuesday, December 16, 2014 8:18 PM > > To: lng-odp@lists.linaro.org > > Subject: [lng-odp] [PATCH] linux-generic: add support for > > ODP_PACKET_OFFSET_INVALID > > > > Fix for Bug #1004. > > > > Signed-off-by: Bill Fischofer <bill.fischofer@linaro.org> > > --- > > platform/linux-generic/include/api/odp_packet.h | 7 +++++-- > > .../linux-generic/include/api/odp_platform_types.h | 3 +++ > > .../linux-generic/include/odp_packet_internal.h | 22 > > ++++++++++++++++++---- > > platform/linux-generic/odp_packet.c | 10 ++++++---- > > 4 files changed, 32 insertions(+), 10 deletions(-) > > > > diff --git a/platform/linux-generic/include/api/odp_packet.h > > b/platform/linux-generic/include/api/odp_packet.h > > index 97c2cb6..d0f15ac 100644 > > --- a/platform/linux-generic/include/api/odp_packet.h > > +++ b/platform/linux-generic/include/api/odp_packet.h > > @@ -469,6 +469,7 @@ void *odp_packet_l2_ptr(odp_packet_t pkt, uint32_t > > *len); > > * @param pkt Packet handle > > * > > * @return Layer 2 start offset > > + * @retval ODP_PACKET_OFFSET_INVALID If not found > > */ > > uint32_t odp_packet_l2_offset(odp_packet_t pkt); > > > > @@ -514,7 +515,8 @@ void *odp_packet_l3_ptr(odp_packet_t pkt, uint32_t > > *len); > > * > > * @param pkt Packet handle > > * > > - * @return Layer 3 start offset or ODP_PACKET_OFFSET_INVALID if not > > found > > + * @return Layer 3 start offset > > + * @retval ODP_PACKET_OFFSET_INVALID If not found > > */ > > uint32_t odp_packet_l3_offset(odp_packet_t pkt); > > > > @@ -560,7 +562,8 @@ void *odp_packet_l4_ptr(odp_packet_t pkt, uint32_t > > *len); > > * > > * @param pkt Packet handle > > * > > - * @return Layer 4 start offset or ODP_PACKET_OFFSET_INVALID if not > > found > > + * @return Layer 4 start offset > > + * @retval ODP_PACKET_OFFSET_INVALID If not found > > */ > > uint32_t odp_packet_l4_offset(odp_packet_t pkt); > > > > diff --git a/platform/linux-generic/include/api/odp_platform_types.h > > b/platform/linux-generic/include/api/odp_platform_types.h > > index 2cfba87..8671b42 100644 > > --- a/platform/linux-generic/include/api/odp_platform_types.h > > +++ b/platform/linux-generic/include/api/odp_platform_types.h > > @@ -47,6 +47,9 @@ typedef odp_buffer_t odp_packet_t; > > /** Invalid packet */ > > #define ODP_PACKET_INVALID ODP_BUFFER_INVALID > > > > +/** Invalid packet offset */ > > +#define ODP_PACKET_OFFSET_INVALID (0xffffffff) > > + > > /** ODP packet segment */ > > typedef odp_buffer_t odp_packet_seg_t; > > > > diff --git a/platform/linux-generic/include/odp_packet_internal.h > > b/platform/linux-generic/include/odp_packet_internal.h > > index a0eff30..515c127 100644 > > --- a/platform/linux-generic/include/odp_packet_internal.h > > +++ b/platform/linux-generic/include/odp_packet_internal.h > > @@ -169,6 +169,12 @@ static inline void packet_init(pool_entry_t *pool, > > len = sizeof(odp_packet_hdr_t) - start_offset; > > memset(start, 0, len); > > > > + /* Set metadata items that initialize to non-zero values */ > > + pkt_hdr->l2_offset = ODP_PACKET_OFFSET_INVALID; > > + pkt_hdr->l3_offset = ODP_PACKET_OFFSET_INVALID; > > + pkt_hdr->l4_offset = ODP_PACKET_OFFSET_INVALID; > > + pkt_hdr->payload_offset = ODP_PACKET_OFFSET_INVALID; > > + > > /* > > * Packet headroom is set from the pool's headroom > > * Packet tailroom is rounded up to fill the last > > @@ -192,15 +198,23 @@ 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) > > +#define pull_offset(x, len) do { \ > > + if (x != ODP_PACKET_OFFSET_INVALID) \ > > + x = x < len ? ODP_PACKET_OFFSET_INVALID : x - len; \ > > + } while (0) > > + > > +#define push_offset(x, len) do { \ > > + if (x != ODP_PACKET_OFFSET_INVALID) \ > > + x += len; \ > > + } while (0) > > > > 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; > > + push_offset(pkt_hdr->l2_offset, len); > > + push_offset(pkt_hdr->l3_offset, len); > > + push_offset(pkt_hdr->l4_offset, len); > > } > > > > static inline void pull_head(odp_packet_hdr_t *pkt_hdr, size_t len) > > diff --git a/platform/linux-generic/odp_packet.c b/platform/linux- > > generic/odp_packet.c > > index 0ab9866..65e6288 100644 > > --- a/platform/linux-generic/odp_packet.c > > +++ b/platform/linux-generic/odp_packet.c > > @@ -776,10 +776,10 @@ int _odp_packet_parse(odp_packet_t pkt) > > pkt_hdr->error_flags.all = 0; > > pkt_hdr->input_flags.all = 0; > > pkt_hdr->output_flags.all = 0; > > - pkt_hdr->l2_offset = 0; > > - pkt_hdr->l3_offset = 0; > > - pkt_hdr->l4_offset = 0; > > - pkt_hdr->payload_offset = 0; > > + pkt_hdr->l2_offset = ODP_PACKET_OFFSET_INVALID; > > + pkt_hdr->l3_offset = ODP_PACKET_OFFSET_INVALID; > > + pkt_hdr->l4_offset = ODP_PACKET_OFFSET_INVALID; > > + pkt_hdr->payload_offset = ODP_PACKET_OFFSET_INVALID; > > pkt_hdr->vlan_s_tag = 0; > > pkt_hdr->vlan_c_tag = 0; > > pkt_hdr->l3_protocol = 0; > > @@ -861,6 +861,7 @@ int _odp_packet_parse(odp_packet_t pkt) > > > > default: > > pkt_hdr->input_flags.l3 = 0; > > + pkt_hdr->l3_offset = ODP_PACKET_OFFSET_INVALID; > > ip_proto = 255; /* Reserved invalid by IANA */ > > } > > > > @@ -892,6 +893,7 @@ int _odp_packet_parse(odp_packet_t pkt) > > > > default: > > pkt_hdr->input_flags.l4 = 0; > > + pkt_hdr->l4_offset = ODP_PACKET_OFFSET_INVALID; > > break; > > } > > > > -- > > 1.8.3.2 > > > > _______________________________________________ > > lng-odp mailing list > > lng-odp@lists.linaro.org > > http://lists.linaro.org/mailman/listinfo/lng-odp >
No, the current code will not return NULL, hence this bug fix. On Wed, Dec 17, 2014 at 6:55 AM, Savolainen, Petri (NSN - FI/Espoo) < petri.savolainen@nsn.com> wrote: > > It can still return NULL. Application should first check only **once** > the _*has*_l3(). After that every _*l3*_prt() and _*l3*_offset() call > should succeed. > > > > -Petri > > > > *From:* ext Bill Fischofer [mailto:bill.fischofer@linaro.org] > *Sent:* Wednesday, December 17, 2014 2:48 PM > *To:* Savolainen, Petri (NSN - FI/Espoo) > *Cc:* lng-odp@lists.linaro.org > *Subject:* Re: [lng-odp] [PATCH] linux-generic: add support for > ODP_PACKET_OFFSET_INVALID > > > > I think this is a good part of the spec and the omission in the code is > indeed a bug that should be corrected. If we couldn't find an L3 header, > for example, then what does odp_packet_l3_ptr() return? Without this fix > it returns the starting address of the packet, which is incorrect. With > this fix it will return NULL. > > > > On Wed, Dec 17, 2014 at 6:38 AM, Savolainen, Petri (NSN - FI/Espoo) < > petri.savolainen@nsn.com> wrote: > > We could take a timeout on this. Maybe it's better to specify that > l2/3/4_ptr and _offsets are valid only when odp_packet_has_l2/3/4() returns > 1. > > We wouldn't need to define ODP_PACKET_OFFSET_INVALID or NULL return values > for those functions. > > > -Petri > > > > > -----Original Message----- > > From: lng-odp-bounces@lists.linaro.org [mailto:lng-odp- > > bounces@lists.linaro.org] On Behalf Of ext Bill Fischofer > > Sent: Tuesday, December 16, 2014 8:18 PM > > To: lng-odp@lists.linaro.org > > Subject: [lng-odp] [PATCH] linux-generic: add support for > > ODP_PACKET_OFFSET_INVALID > > > > Fix for Bug #1004. > > > > Signed-off-by: Bill Fischofer <bill.fischofer@linaro.org> > > --- > > platform/linux-generic/include/api/odp_packet.h | 7 +++++-- > > .../linux-generic/include/api/odp_platform_types.h | 3 +++ > > .../linux-generic/include/odp_packet_internal.h | 22 > > ++++++++++++++++++---- > > platform/linux-generic/odp_packet.c | 10 ++++++---- > > 4 files changed, 32 insertions(+), 10 deletions(-) > > > > diff --git a/platform/linux-generic/include/api/odp_packet.h > > b/platform/linux-generic/include/api/odp_packet.h > > index 97c2cb6..d0f15ac 100644 > > --- a/platform/linux-generic/include/api/odp_packet.h > > +++ b/platform/linux-generic/include/api/odp_packet.h > > @@ -469,6 +469,7 @@ void *odp_packet_l2_ptr(odp_packet_t pkt, uint32_t > > *len); > > * @param pkt Packet handle > > * > > * @return Layer 2 start offset > > + * @retval ODP_PACKET_OFFSET_INVALID If not found > > */ > > uint32_t odp_packet_l2_offset(odp_packet_t pkt); > > > > @@ -514,7 +515,8 @@ void *odp_packet_l3_ptr(odp_packet_t pkt, uint32_t > > *len); > > * > > * @param pkt Packet handle > > * > > - * @return Layer 3 start offset or ODP_PACKET_OFFSET_INVALID if not > > found > > + * @return Layer 3 start offset > > + * @retval ODP_PACKET_OFFSET_INVALID If not found > > */ > > uint32_t odp_packet_l3_offset(odp_packet_t pkt); > > > > @@ -560,7 +562,8 @@ void *odp_packet_l4_ptr(odp_packet_t pkt, uint32_t > > *len); > > * > > * @param pkt Packet handle > > * > > - * @return Layer 4 start offset or ODP_PACKET_OFFSET_INVALID if not > > found > > + * @return Layer 4 start offset > > + * @retval ODP_PACKET_OFFSET_INVALID If not found > > */ > > uint32_t odp_packet_l4_offset(odp_packet_t pkt); > > > > diff --git a/platform/linux-generic/include/api/odp_platform_types.h > > b/platform/linux-generic/include/api/odp_platform_types.h > > index 2cfba87..8671b42 100644 > > --- a/platform/linux-generic/include/api/odp_platform_types.h > > +++ b/platform/linux-generic/include/api/odp_platform_types.h > > @@ -47,6 +47,9 @@ typedef odp_buffer_t odp_packet_t; > > /** Invalid packet */ > > #define ODP_PACKET_INVALID ODP_BUFFER_INVALID > > > > +/** Invalid packet offset */ > > +#define ODP_PACKET_OFFSET_INVALID (0xffffffff) > > + > > /** ODP packet segment */ > > typedef odp_buffer_t odp_packet_seg_t; > > > > diff --git a/platform/linux-generic/include/odp_packet_internal.h > > b/platform/linux-generic/include/odp_packet_internal.h > > index a0eff30..515c127 100644 > > --- a/platform/linux-generic/include/odp_packet_internal.h > > +++ b/platform/linux-generic/include/odp_packet_internal.h > > @@ -169,6 +169,12 @@ static inline void packet_init(pool_entry_t *pool, > > len = sizeof(odp_packet_hdr_t) - start_offset; > > memset(start, 0, len); > > > > + /* Set metadata items that initialize to non-zero values */ > > + pkt_hdr->l2_offset = ODP_PACKET_OFFSET_INVALID; > > + pkt_hdr->l3_offset = ODP_PACKET_OFFSET_INVALID; > > + pkt_hdr->l4_offset = ODP_PACKET_OFFSET_INVALID; > > + pkt_hdr->payload_offset = ODP_PACKET_OFFSET_INVALID; > > + > > /* > > * Packet headroom is set from the pool's headroom > > * Packet tailroom is rounded up to fill the last > > @@ -192,15 +198,23 @@ 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) > > +#define pull_offset(x, len) do { \ > > + if (x != ODP_PACKET_OFFSET_INVALID) \ > > + x = x < len ? ODP_PACKET_OFFSET_INVALID : x - len; \ > > + } while (0) > > + > > +#define push_offset(x, len) do { \ > > + if (x != ODP_PACKET_OFFSET_INVALID) \ > > + x += len; \ > > + } while (0) > > > > 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; > > + push_offset(pkt_hdr->l2_offset, len); > > + push_offset(pkt_hdr->l3_offset, len); > > + push_offset(pkt_hdr->l4_offset, len); > > } > > > > static inline void pull_head(odp_packet_hdr_t *pkt_hdr, size_t len) > > diff --git a/platform/linux-generic/odp_packet.c b/platform/linux- > > generic/odp_packet.c > > index 0ab9866..65e6288 100644 > > --- a/platform/linux-generic/odp_packet.c > > +++ b/platform/linux-generic/odp_packet.c > > @@ -776,10 +776,10 @@ int _odp_packet_parse(odp_packet_t pkt) > > pkt_hdr->error_flags.all = 0; > > pkt_hdr->input_flags.all = 0; > > pkt_hdr->output_flags.all = 0; > > - pkt_hdr->l2_offset = 0; > > - pkt_hdr->l3_offset = 0; > > - pkt_hdr->l4_offset = 0; > > - pkt_hdr->payload_offset = 0; > > + pkt_hdr->l2_offset = ODP_PACKET_OFFSET_INVALID; > > + pkt_hdr->l3_offset = ODP_PACKET_OFFSET_INVALID; > > + pkt_hdr->l4_offset = ODP_PACKET_OFFSET_INVALID; > > + pkt_hdr->payload_offset = ODP_PACKET_OFFSET_INVALID; > > pkt_hdr->vlan_s_tag = 0; > > pkt_hdr->vlan_c_tag = 0; > > pkt_hdr->l3_protocol = 0; > > @@ -861,6 +861,7 @@ int _odp_packet_parse(odp_packet_t pkt) > > > > default: > > pkt_hdr->input_flags.l3 = 0; > > + pkt_hdr->l3_offset = ODP_PACKET_OFFSET_INVALID; > > ip_proto = 255; /* Reserved invalid by IANA */ > > } > > > > @@ -892,6 +893,7 @@ int _odp_packet_parse(odp_packet_t pkt) > > > > default: > > pkt_hdr->input_flags.l4 = 0; > > + pkt_hdr->l4_offset = ODP_PACKET_OFFSET_INVALID; > > break; > > } > > > > -- > > 1.8.3.2 > > > > > _______________________________________________ > > lng-odp mailing list > > lng-odp@lists.linaro.org > > http://lists.linaro.org/mailman/listinfo/lng-odp >
On 17 December 2014 at 13:55, Savolainen, Petri (NSN - FI/Espoo) <petri.savolainen@nsn.com> wrote: > It can still return NULL. Application should first check only *once* the > _has_l3(). After that every _l3_prt() and _l3_offset() call should succeed. So application must call both odp_packet_has_l3() and then odp_packet_l3_ptr() (or _offset())? Seems complicated and redundant to me, easy to get wrong in the application. Why can't _l3_ptr() check if the l3 offset is set and return NULL if not? Each API function would then be self-sufficient, I think this will make the API easier to use and applications more robust. The overhead of checking the validity of the l3 offset before returning the l3 pointer should be small compared to the overhead of making multiple ODP packet calls and translating packet handle to the buffer address where there Lx offsets live. This could be done branch-less (if you suspect your branch predictor not working well here) and just do a final conditional move (return offset != INVALID_OFFSET ? ptr : NULL). > > > > -Petri > > > > From: ext Bill Fischofer [mailto:bill.fischofer@linaro.org] > Sent: Wednesday, December 17, 2014 2:48 PM > To: Savolainen, Petri (NSN - FI/Espoo) > Cc: lng-odp@lists.linaro.org > Subject: Re: [lng-odp] [PATCH] linux-generic: add support for > ODP_PACKET_OFFSET_INVALID > > > > I think this is a good part of the spec and the omission in the code is > indeed a bug that should be corrected. If we couldn't find an L3 header, > for example, then what does odp_packet_l3_ptr() return? Without this fix it > returns the starting address of the packet, which is incorrect. With this > fix it will return NULL. > > > > On Wed, Dec 17, 2014 at 6:38 AM, Savolainen, Petri (NSN - FI/Espoo) > <petri.savolainen@nsn.com> wrote: > > We could take a timeout on this. Maybe it's better to specify that > l2/3/4_ptr and _offsets are valid only when odp_packet_has_l2/3/4() returns > 1. > > We wouldn't need to define ODP_PACKET_OFFSET_INVALID or NULL return values > for those functions. > > > -Petri > > > >> -----Original Message----- >> From: lng-odp-bounces@lists.linaro.org [mailto:lng-odp- >> bounces@lists.linaro.org] On Behalf Of ext Bill Fischofer >> Sent: Tuesday, December 16, 2014 8:18 PM >> To: lng-odp@lists.linaro.org >> Subject: [lng-odp] [PATCH] linux-generic: add support for >> ODP_PACKET_OFFSET_INVALID >> >> Fix for Bug #1004. >> >> Signed-off-by: Bill Fischofer <bill.fischofer@linaro.org> >> --- >> platform/linux-generic/include/api/odp_packet.h | 7 +++++-- >> .../linux-generic/include/api/odp_platform_types.h | 3 +++ >> .../linux-generic/include/odp_packet_internal.h | 22 >> ++++++++++++++++++---- >> platform/linux-generic/odp_packet.c | 10 ++++++---- >> 4 files changed, 32 insertions(+), 10 deletions(-) >> >> diff --git a/platform/linux-generic/include/api/odp_packet.h >> b/platform/linux-generic/include/api/odp_packet.h >> index 97c2cb6..d0f15ac 100644 >> --- a/platform/linux-generic/include/api/odp_packet.h >> +++ b/platform/linux-generic/include/api/odp_packet.h >> @@ -469,6 +469,7 @@ void *odp_packet_l2_ptr(odp_packet_t pkt, uint32_t >> *len); >> * @param pkt Packet handle >> * >> * @return Layer 2 start offset >> + * @retval ODP_PACKET_OFFSET_INVALID If not found >> */ >> uint32_t odp_packet_l2_offset(odp_packet_t pkt); >> >> @@ -514,7 +515,8 @@ void *odp_packet_l3_ptr(odp_packet_t pkt, uint32_t >> *len); >> * >> * @param pkt Packet handle >> * >> - * @return Layer 3 start offset or ODP_PACKET_OFFSET_INVALID if not >> found >> + * @return Layer 3 start offset >> + * @retval ODP_PACKET_OFFSET_INVALID If not found >> */ >> uint32_t odp_packet_l3_offset(odp_packet_t pkt); >> >> @@ -560,7 +562,8 @@ void *odp_packet_l4_ptr(odp_packet_t pkt, uint32_t >> *len); >> * >> * @param pkt Packet handle >> * >> - * @return Layer 4 start offset or ODP_PACKET_OFFSET_INVALID if not >> found >> + * @return Layer 4 start offset >> + * @retval ODP_PACKET_OFFSET_INVALID If not found >> */ >> uint32_t odp_packet_l4_offset(odp_packet_t pkt); >> >> diff --git a/platform/linux-generic/include/api/odp_platform_types.h >> b/platform/linux-generic/include/api/odp_platform_types.h >> index 2cfba87..8671b42 100644 >> --- a/platform/linux-generic/include/api/odp_platform_types.h >> +++ b/platform/linux-generic/include/api/odp_platform_types.h >> @@ -47,6 +47,9 @@ typedef odp_buffer_t odp_packet_t; >> /** Invalid packet */ >> #define ODP_PACKET_INVALID ODP_BUFFER_INVALID >> >> +/** Invalid packet offset */ >> +#define ODP_PACKET_OFFSET_INVALID (0xffffffff) >> + >> /** ODP packet segment */ >> typedef odp_buffer_t odp_packet_seg_t; >> >> diff --git a/platform/linux-generic/include/odp_packet_internal.h >> b/platform/linux-generic/include/odp_packet_internal.h >> index a0eff30..515c127 100644 >> --- a/platform/linux-generic/include/odp_packet_internal.h >> +++ b/platform/linux-generic/include/odp_packet_internal.h >> @@ -169,6 +169,12 @@ static inline void packet_init(pool_entry_t *pool, >> len = sizeof(odp_packet_hdr_t) - start_offset; >> memset(start, 0, len); >> >> + /* Set metadata items that initialize to non-zero values */ >> + pkt_hdr->l2_offset = ODP_PACKET_OFFSET_INVALID; >> + pkt_hdr->l3_offset = ODP_PACKET_OFFSET_INVALID; >> + pkt_hdr->l4_offset = ODP_PACKET_OFFSET_INVALID; >> + pkt_hdr->payload_offset = ODP_PACKET_OFFSET_INVALID; >> + >> /* >> * Packet headroom is set from the pool's headroom >> * Packet tailroom is rounded up to fill the last >> @@ -192,15 +198,23 @@ 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) >> +#define pull_offset(x, len) do { \ >> + if (x != ODP_PACKET_OFFSET_INVALID) \ >> + x = x < len ? ODP_PACKET_OFFSET_INVALID : x - len; \ >> + } while (0) >> + >> +#define push_offset(x, len) do { \ >> + if (x != ODP_PACKET_OFFSET_INVALID) \ >> + x += len; \ >> + } while (0) >> >> 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; >> + push_offset(pkt_hdr->l2_offset, len); >> + push_offset(pkt_hdr->l3_offset, len); >> + push_offset(pkt_hdr->l4_offset, len); >> } >> >> static inline void pull_head(odp_packet_hdr_t *pkt_hdr, size_t len) >> diff --git a/platform/linux-generic/odp_packet.c b/platform/linux- >> generic/odp_packet.c >> index 0ab9866..65e6288 100644 >> --- a/platform/linux-generic/odp_packet.c >> +++ b/platform/linux-generic/odp_packet.c >> @@ -776,10 +776,10 @@ int _odp_packet_parse(odp_packet_t pkt) >> pkt_hdr->error_flags.all = 0; >> pkt_hdr->input_flags.all = 0; >> pkt_hdr->output_flags.all = 0; >> - pkt_hdr->l2_offset = 0; >> - pkt_hdr->l3_offset = 0; >> - pkt_hdr->l4_offset = 0; >> - pkt_hdr->payload_offset = 0; >> + pkt_hdr->l2_offset = ODP_PACKET_OFFSET_INVALID; >> + pkt_hdr->l3_offset = ODP_PACKET_OFFSET_INVALID; >> + pkt_hdr->l4_offset = ODP_PACKET_OFFSET_INVALID; >> + pkt_hdr->payload_offset = ODP_PACKET_OFFSET_INVALID; >> pkt_hdr->vlan_s_tag = 0; >> pkt_hdr->vlan_c_tag = 0; >> pkt_hdr->l3_protocol = 0; >> @@ -861,6 +861,7 @@ int _odp_packet_parse(odp_packet_t pkt) >> >> default: >> pkt_hdr->input_flags.l3 = 0; >> + pkt_hdr->l3_offset = ODP_PACKET_OFFSET_INVALID; >> ip_proto = 255; /* Reserved invalid by IANA */ >> } >> >> @@ -892,6 +893,7 @@ int _odp_packet_parse(odp_packet_t pkt) >> >> default: >> pkt_hdr->input_flags.l4 = 0; >> + pkt_hdr->l4_offset = ODP_PACKET_OFFSET_INVALID; >> break; >> } >> >> -- >> 1.8.3.2 >> > >> _______________________________________________ >> 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 --git a/platform/linux-generic/include/api/odp_packet.h b/platform/linux-generic/include/api/odp_packet.h index 97c2cb6..d0f15ac 100644 --- a/platform/linux-generic/include/api/odp_packet.h +++ b/platform/linux-generic/include/api/odp_packet.h @@ -469,6 +469,7 @@ void *odp_packet_l2_ptr(odp_packet_t pkt, uint32_t *len); * @param pkt Packet handle * * @return Layer 2 start offset + * @retval ODP_PACKET_OFFSET_INVALID If not found */ uint32_t odp_packet_l2_offset(odp_packet_t pkt); @@ -514,7 +515,8 @@ void *odp_packet_l3_ptr(odp_packet_t pkt, uint32_t *len); * * @param pkt Packet handle * - * @return Layer 3 start offset or ODP_PACKET_OFFSET_INVALID if not found + * @return Layer 3 start offset + * @retval ODP_PACKET_OFFSET_INVALID If not found */ uint32_t odp_packet_l3_offset(odp_packet_t pkt); @@ -560,7 +562,8 @@ void *odp_packet_l4_ptr(odp_packet_t pkt, uint32_t *len); * * @param pkt Packet handle * - * @return Layer 4 start offset or ODP_PACKET_OFFSET_INVALID if not found + * @return Layer 4 start offset + * @retval ODP_PACKET_OFFSET_INVALID If not found */ uint32_t odp_packet_l4_offset(odp_packet_t pkt); diff --git a/platform/linux-generic/include/api/odp_platform_types.h b/platform/linux-generic/include/api/odp_platform_types.h index 2cfba87..8671b42 100644 --- a/platform/linux-generic/include/api/odp_platform_types.h +++ b/platform/linux-generic/include/api/odp_platform_types.h @@ -47,6 +47,9 @@ typedef odp_buffer_t odp_packet_t; /** Invalid packet */ #define ODP_PACKET_INVALID ODP_BUFFER_INVALID +/** Invalid packet offset */ +#define ODP_PACKET_OFFSET_INVALID (0xffffffff) + /** ODP packet segment */ typedef odp_buffer_t odp_packet_seg_t; diff --git a/platform/linux-generic/include/odp_packet_internal.h b/platform/linux-generic/include/odp_packet_internal.h index a0eff30..515c127 100644 --- a/platform/linux-generic/include/odp_packet_internal.h +++ b/platform/linux-generic/include/odp_packet_internal.h @@ -169,6 +169,12 @@ static inline void packet_init(pool_entry_t *pool, len = sizeof(odp_packet_hdr_t) - start_offset; memset(start, 0, len); + /* Set metadata items that initialize to non-zero values */ + pkt_hdr->l2_offset = ODP_PACKET_OFFSET_INVALID; + pkt_hdr->l3_offset = ODP_PACKET_OFFSET_INVALID; + pkt_hdr->l4_offset = ODP_PACKET_OFFSET_INVALID; + pkt_hdr->payload_offset = ODP_PACKET_OFFSET_INVALID; + /* * Packet headroom is set from the pool's headroom * Packet tailroom is rounded up to fill the last @@ -192,15 +198,23 @@ 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) +#define pull_offset(x, len) do { \ + if (x != ODP_PACKET_OFFSET_INVALID) \ + x = x < len ? ODP_PACKET_OFFSET_INVALID : x - len; \ + } while (0) + +#define push_offset(x, len) do { \ + if (x != ODP_PACKET_OFFSET_INVALID) \ + x += len; \ + } while (0) 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; + push_offset(pkt_hdr->l2_offset, len); + push_offset(pkt_hdr->l3_offset, len); + push_offset(pkt_hdr->l4_offset, len); } static inline void pull_head(odp_packet_hdr_t *pkt_hdr, size_t len) diff --git a/platform/linux-generic/odp_packet.c b/platform/linux-generic/odp_packet.c index 0ab9866..65e6288 100644 --- a/platform/linux-generic/odp_packet.c +++ b/platform/linux-generic/odp_packet.c @@ -776,10 +776,10 @@ int _odp_packet_parse(odp_packet_t pkt) pkt_hdr->error_flags.all = 0; pkt_hdr->input_flags.all = 0; pkt_hdr->output_flags.all = 0; - pkt_hdr->l2_offset = 0; - pkt_hdr->l3_offset = 0; - pkt_hdr->l4_offset = 0; - pkt_hdr->payload_offset = 0; + pkt_hdr->l2_offset = ODP_PACKET_OFFSET_INVALID; + pkt_hdr->l3_offset = ODP_PACKET_OFFSET_INVALID; + pkt_hdr->l4_offset = ODP_PACKET_OFFSET_INVALID; + pkt_hdr->payload_offset = ODP_PACKET_OFFSET_INVALID; pkt_hdr->vlan_s_tag = 0; pkt_hdr->vlan_c_tag = 0; pkt_hdr->l3_protocol = 0; @@ -861,6 +861,7 @@ int _odp_packet_parse(odp_packet_t pkt) default: pkt_hdr->input_flags.l3 = 0; + pkt_hdr->l3_offset = ODP_PACKET_OFFSET_INVALID; ip_proto = 255; /* Reserved invalid by IANA */ } @@ -892,6 +893,7 @@ int _odp_packet_parse(odp_packet_t pkt) default: pkt_hdr->input_flags.l4 = 0; + pkt_hdr->l4_offset = ODP_PACKET_OFFSET_INVALID; break; }
Fix for Bug #1004. Signed-off-by: Bill Fischofer <bill.fischofer@linaro.org> --- platform/linux-generic/include/api/odp_packet.h | 7 +++++-- .../linux-generic/include/api/odp_platform_types.h | 3 +++ .../linux-generic/include/odp_packet_internal.h | 22 ++++++++++++++++++---- platform/linux-generic/odp_packet.c | 10 ++++++---- 4 files changed, 32 insertions(+), 10 deletions(-) -- 1.8.3.2