| Message ID | 1463392220-22856-6-git-send-email-matias.elo@nokia.com |
|---|---|
| State | Accepted |
| Commit | f2ed9070247b53110d18fef193867c86085a2d89 |
| Headers | show |
On 16 May 2016 at 05:50, Matias Elo <matias.elo@nokia.com> wrote: > Using memset to initialize struct odp_packet_hdr_t contents > to 0 has a significant overhead. Instead, initialize only > the required members of the struct. > > Signed-off-by: Matias Elo <matias.elo@nokia.com> > --- > platform/linux-generic/include/odp_packet_internal.h | 13 +++++++++---- > platform/linux-generic/odp_packet.c | 18 > ++++++------------ > 2 files changed, 15 insertions(+), 16 deletions(-) > > diff --git a/platform/linux-generic/include/odp_packet_internal.h > b/platform/linux-generic/include/odp_packet_internal.h > index 1306b05..c87bc9f 100644 > --- a/platform/linux-generic/include/odp_packet_internal.h > +++ b/platform/linux-generic/include/odp_packet_internal.h > @@ -4,7 +4,6 @@ > * SPDX-License-Identifier: BSD-3-Clause > */ > - > /** > * @file > * > @@ -129,11 +128,16 @@ ODP_STATIC_ASSERT(sizeof(output_flags_t) == > sizeof(uint32_t), > > /** > * Internal Packet header > + * > + * To optimize fast path performance this struct is not initialized to > zero in > + * packet_init(). Because of this any new fields added must be reviewed > for > + * initialization requirements. > */ This file will not contribute to the generated API documentation and I think it should. This platform specific warning should appear there as a doxygen @warning Perhaps we need to start a trend of adding a doxygen platform specific module called "PLATFORM SPECIFICS", then we can add information with the @addtogroup platform_specifics and users across platforms will know where to look for this sort of information. typedef struct { > /* common buffer header */ > odp_buffer_hdr_t buf_hdr; > > + /* Following members are initialized by packet_init() */ > input_flags_t input_flags; > error_flags_t error_flags; > output_flags_t output_flags; > @@ -142,15 +146,16 @@ typedef struct { > uint32_t l3_offset; /**< offset to L3 hdr, e.g. IPv4, IPv6 */ > uint32_t l4_offset; /**< offset to L4 hdr (TCP, UDP, SCTP, also > ICMP) */ > > - uint32_t l3_len; /**< Layer 3 length */ > - uint32_t l4_len; /**< Layer 4 length */ > - > uint32_t frame_len; > uint32_t headroom; > uint32_t tailroom; > > odp_pktio_t input; > > + /* Members below are not initialized by packet_init() */ > + uint32_t l3_len; /**< Layer 3 length */ > + uint32_t l4_len; /**< Layer 4 length */ > + > uint32_t flow_hash; /**< Flow hash value */ > odp_time_t timestamp; /**< Timestamp value */ > > diff --git a/platform/linux-generic/odp_packet.c > b/platform/linux-generic/odp_packet.c > index 4f523c9..8dde404 100644 > --- a/platform/linux-generic/odp_packet.c > +++ b/platform/linux-generic/odp_packet.c > @@ -49,19 +49,11 @@ void packet_parse_reset(odp_packet_hdr_t *pkt_hdr) > static void packet_init(pool_entry_t *pool, odp_packet_hdr_t *pkt_hdr, > size_t size, int parse) > { > - /* > - * Reset parser metadata. Note that we clear via memset to make > - * this routine indepenent of any additional adds to packet > metadata. > - */ > - const size_t start_offset = ODP_FIELD_SIZEOF(odp_packet_hdr_t, > buf_hdr); > - uint8_t *start; > - size_t len; > + pkt_hdr->input_flags.all = 0; > + pkt_hdr->output_flags.all = 0; > + pkt_hdr->error_flags.all = 0; > > - start = (uint8_t *)pkt_hdr + start_offset; > - 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 = 0; > pkt_hdr->l3_offset = ODP_PACKET_OFFSET_INVALID; > pkt_hdr->l4_offset = ODP_PACKET_OFFSET_INVALID; > > @@ -79,6 +71,8 @@ static void packet_init(pool_entry_t *pool, > odp_packet_hdr_t *pkt_hdr, > pkt_hdr->tailroom = > (pool->s.seg_size * pkt_hdr->buf_hdr.segcount) - > (pool->s.headroom + size); > + > + pkt_hdr->input = ODP_PKTIO_INVALID; > } > > odp_packet_t packet_alloc(odp_pool_t pool_hdl, uint32_t len, int parse) > -- > 1.9.1 > > _______________________________________________ > lng-odp mailing list > lng-odp@lists.linaro.org > https://lists.linaro.org/mailman/listinfo/lng-odp >
On Mon, May 16, 2016 at 9:25 AM, Mike Holmes <mike.holmes@linaro.org> wrote: > On 16 May 2016 at 05:50, Matias Elo <matias.elo@nokia.com> wrote: > > > Using memset to initialize struct odp_packet_hdr_t contents > > to 0 has a significant overhead. Instead, initialize only > > the required members of the struct. > > > > Signed-off-by: Matias Elo <matias.elo@nokia.com> > > --- > > platform/linux-generic/include/odp_packet_internal.h | 13 +++++++++---- > > platform/linux-generic/odp_packet.c | 18 > > ++++++------------ > > 2 files changed, 15 insertions(+), 16 deletions(-) > > > > diff --git a/platform/linux-generic/include/odp_packet_internal.h > > b/platform/linux-generic/include/odp_packet_internal.h > > index 1306b05..c87bc9f 100644 > > --- a/platform/linux-generic/include/odp_packet_internal.h > > +++ b/platform/linux-generic/include/odp_packet_internal.h > > @@ -4,7 +4,6 @@ > > * SPDX-License-Identifier: BSD-3-Clause > > */ > > > - > > /** > > * @file > > * > > @@ -129,11 +128,16 @@ ODP_STATIC_ASSERT(sizeof(output_flags_t) == > > sizeof(uint32_t), > > > > /** > > * Internal Packet header > > + * > > + * To optimize fast path performance this struct is not initialized to > > zero in > > + * packet_init(). Because of this any new fields added must be reviewed > > for > > + * initialization requirements. > > */ > > > This file will not contribute to the generated API documentation and I > It's not relevant to the API, only to the odp-linux implementation of the API. > think it should. This platform specific warning should appear there as a > doxygen @warning > Perhaps we need to start a trend of adding a doxygen platform specific > module called "PLATFORM SPECIFICS", then we can add information with the > @addtogroup platform_specifics and users across platforms will know where > to look for this sort of information. The best place for this sort of implementation documentation is in the comments in the C code itself. We just want a reminder to future maintainers of the code of a shortcut that was taken for performance reasons and the cautions that need to be observed as a result. If we want to have these sort of notes be collated as part of a "Maintainer's Guide" for odp-linux, that would seem a worthwhile project. Something to consider adding for Tiger Moth? > > typedef struct { > > /* common buffer header */ > > odp_buffer_hdr_t buf_hdr; > > > > + /* Following members are initialized by packet_init() */ > > input_flags_t input_flags; > > error_flags_t error_flags; > > output_flags_t output_flags; > > @@ -142,15 +146,16 @@ typedef struct { > > uint32_t l3_offset; /**< offset to L3 hdr, e.g. IPv4, IPv6 */ > > uint32_t l4_offset; /**< offset to L4 hdr (TCP, UDP, SCTP, also > > ICMP) */ > > > > - uint32_t l3_len; /**< Layer 3 length */ > > - uint32_t l4_len; /**< Layer 4 length */ > > - > > uint32_t frame_len; > > uint32_t headroom; > > uint32_t tailroom; > > > > odp_pktio_t input; > > > > + /* Members below are not initialized by packet_init() */ > > + uint32_t l3_len; /**< Layer 3 length */ > > + uint32_t l4_len; /**< Layer 4 length */ > > + > > uint32_t flow_hash; /**< Flow hash value */ > > odp_time_t timestamp; /**< Timestamp value */ > > > > diff --git a/platform/linux-generic/odp_packet.c > > b/platform/linux-generic/odp_packet.c > > index 4f523c9..8dde404 100644 > > --- a/platform/linux-generic/odp_packet.c > > +++ b/platform/linux-generic/odp_packet.c > > @@ -49,19 +49,11 @@ void packet_parse_reset(odp_packet_hdr_t *pkt_hdr) > > static void packet_init(pool_entry_t *pool, odp_packet_hdr_t *pkt_hdr, > > size_t size, int parse) > > { > > - /* > > - * Reset parser metadata. Note that we clear via memset to make > > - * this routine indepenent of any additional adds to packet > > metadata. > > - */ > > - const size_t start_offset = ODP_FIELD_SIZEOF(odp_packet_hdr_t, > > buf_hdr); > > - uint8_t *start; > > - size_t len; > > + pkt_hdr->input_flags.all = 0; > > + pkt_hdr->output_flags.all = 0; > > + pkt_hdr->error_flags.all = 0; > > > > - start = (uint8_t *)pkt_hdr + start_offset; > > - 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 = 0; > > pkt_hdr->l3_offset = ODP_PACKET_OFFSET_INVALID; > > pkt_hdr->l4_offset = ODP_PACKET_OFFSET_INVALID; > > > > @@ -79,6 +71,8 @@ static void packet_init(pool_entry_t *pool, > > odp_packet_hdr_t *pkt_hdr, > > pkt_hdr->tailroom = > > (pool->s.seg_size * pkt_hdr->buf_hdr.segcount) - > > (pool->s.headroom + size); > > + > > + pkt_hdr->input = ODP_PKTIO_INVALID; > > } > > > > odp_packet_t packet_alloc(odp_pool_t pool_hdl, uint32_t len, int parse) > > -- > > 1.9.1 > > > > _______________________________________________ > > lng-odp mailing list > > lng-odp@lists.linaro.org > > https://lists.linaro.org/mailman/listinfo/lng-odp > > > > > > -- > Mike Holmes > Technical Manager - Linaro Networking Group > Linaro.org <http://www.linaro.org/> *│ *Open source software for ARM SoCs > "Work should be fun and collaborative, the rest follows" > _______________________________________________ > lng-odp mailing list > lng-odp@lists.linaro.org > https://lists.linaro.org/mailman/listinfo/lng-odp >
On 16 May 2016 at 18:52, Bill Fischofer <bill.fischofer@linaro.org> wrote: > > > On Mon, May 16, 2016 at 9:25 AM, Mike Holmes <mike.holmes@linaro.org> > wrote: > >> On 16 May 2016 at 05:50, Matias Elo <matias.elo@nokia.com> wrote: >> >> > Using memset to initialize struct odp_packet_hdr_t contents >> > to 0 has a significant overhead. Instead, initialize only >> > the required members of the struct. >> > >> > Signed-off-by: Matias Elo <matias.elo@nokia.com> >> > --- >> > platform/linux-generic/include/odp_packet_internal.h | 13 +++++++++---- >> > platform/linux-generic/odp_packet.c | 18 >> > ++++++------------ >> > 2 files changed, 15 insertions(+), 16 deletions(-) >> > >> > diff --git a/platform/linux-generic/include/odp_packet_internal.h >> > b/platform/linux-generic/include/odp_packet_internal.h >> > index 1306b05..c87bc9f 100644 >> > --- a/platform/linux-generic/include/odp_packet_internal.h >> > +++ b/platform/linux-generic/include/odp_packet_internal.h >> > @@ -4,7 +4,6 @@ >> > * SPDX-License-Identifier: BSD-3-Clause >> > */ >> > >> - >> > /** >> > * @file >> > * >> > @@ -129,11 +128,16 @@ ODP_STATIC_ASSERT(sizeof(output_flags_t) == >> > sizeof(uint32_t), >> > >> > /** >> > * Internal Packet header >> > + * >> > + * To optimize fast path performance this struct is not initialized to >> > zero in >> > + * packet_init(). Because of this any new fields added must be reviewed >> > for >> > + * initialization requirements. >> > */ >> >> >> This file will not contribute to the generated API documentation and I >> > > It's not relevant to the API, only to the odp-linux implementation of the > API. > Exactly, as a user of the odp-linux implementation dont I want to know that some fields are not set ? > > >> think it should. This platform specific warning should appear there as a >> doxygen @warning >> Perhaps we need to start a trend of adding a doxygen platform specific >> module called "PLATFORM SPECIFICS", then we can add information with the >> @addtogroup platform_specifics and users across platforms will know where >> to look for this sort of information. > > > The best place for this sort of implementation documentation is in the > comments in the C code itself. We just want a reminder to future > maintainers of the code of a shortcut that was taken for performance > reasons and the cautions that need to be observed as a result. > I dont agree, too many implementations copy this code, they need to be aware that a normal assumption is broken when they build upon it in their implementation. > > If we want to have these sort of notes be collated as part of a > "Maintainer's Guide" for odp-linux, that would seem a worthwhile project. > Something to consider adding for Tiger Moth? > I think it has to be in the implementer's guide since implementers often use odp-linux as the starting point, we need to warn them. It also has to be in the API guide if these structs will be seen by an application. I think it needs to happen before Monarch since this behaviour is unexpected and we need to make sure it is noticed as the other implementations home in on a fully compliant Monarch API > > >> >> typedef struct { >> > /* common buffer header */ >> > odp_buffer_hdr_t buf_hdr; >> > >> > + /* Following members are initialized by packet_init() */ >> > input_flags_t input_flags; >> > error_flags_t error_flags; >> > output_flags_t output_flags; >> > @@ -142,15 +146,16 @@ typedef struct { >> > uint32_t l3_offset; /**< offset to L3 hdr, e.g. IPv4, IPv6 */ >> > uint32_t l4_offset; /**< offset to L4 hdr (TCP, UDP, SCTP, also >> > ICMP) */ >> > >> > - uint32_t l3_len; /**< Layer 3 length */ >> > - uint32_t l4_len; /**< Layer 4 length */ >> > - >> > uint32_t frame_len; >> > uint32_t headroom; >> > uint32_t tailroom; >> > >> > odp_pktio_t input; >> > >> > + /* Members below are not initialized by packet_init() */ >> > + uint32_t l3_len; /**< Layer 3 length */ >> > + uint32_t l4_len; /**< Layer 4 length */ >> > + >> > uint32_t flow_hash; /**< Flow hash value */ >> > odp_time_t timestamp; /**< Timestamp value */ >> > >> > diff --git a/platform/linux-generic/odp_packet.c >> > b/platform/linux-generic/odp_packet.c >> > index 4f523c9..8dde404 100644 >> > --- a/platform/linux-generic/odp_packet.c >> > +++ b/platform/linux-generic/odp_packet.c >> > @@ -49,19 +49,11 @@ void packet_parse_reset(odp_packet_hdr_t *pkt_hdr) >> > static void packet_init(pool_entry_t *pool, odp_packet_hdr_t *pkt_hdr, >> > size_t size, int parse) >> > { >> > - /* >> > - * Reset parser metadata. Note that we clear via memset to make >> > - * this routine indepenent of any additional adds to packet >> > metadata. >> > - */ >> > - const size_t start_offset = ODP_FIELD_SIZEOF(odp_packet_hdr_t, >> > buf_hdr); >> > - uint8_t *start; >> > - size_t len; >> > + pkt_hdr->input_flags.all = 0; >> > + pkt_hdr->output_flags.all = 0; >> > + pkt_hdr->error_flags.all = 0; >> > >> > - start = (uint8_t *)pkt_hdr + start_offset; >> > - 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 = 0; >> > pkt_hdr->l3_offset = ODP_PACKET_OFFSET_INVALID; >> > pkt_hdr->l4_offset = ODP_PACKET_OFFSET_INVALID; >> > >> > @@ -79,6 +71,8 @@ static void packet_init(pool_entry_t *pool, >> > odp_packet_hdr_t *pkt_hdr, >> > pkt_hdr->tailroom = >> > (pool->s.seg_size * pkt_hdr->buf_hdr.segcount) - >> > (pool->s.headroom + size); >> > + >> > + pkt_hdr->input = ODP_PKTIO_INVALID; >> > } >> > >> > odp_packet_t packet_alloc(odp_pool_t pool_hdl, uint32_t len, int parse) >> > -- >> > 1.9.1 >> > >> > _______________________________________________ >> > lng-odp mailing list >> > lng-odp@lists.linaro.org >> > https://lists.linaro.org/mailman/listinfo/lng-odp >> > >> >> >> >> -- >> Mike Holmes >> Technical Manager - Linaro Networking Group >> Linaro.org <http://www.linaro.org/> *│ *Open source software for ARM SoCs >> "Work should be fun and collaborative, the rest follows" >> _______________________________________________ >> lng-odp mailing list >> lng-odp@lists.linaro.org >> https://lists.linaro.org/mailman/listinfo/lng-odp >> > >
On Tue, May 17, 2016 at 12:42 PM, Mike Holmes <mike.holmes@linaro.org> wrote: > > > On 16 May 2016 at 18:52, Bill Fischofer <bill.fischofer@linaro.org> wrote: > >> >> >> On Mon, May 16, 2016 at 9:25 AM, Mike Holmes <mike.holmes@linaro.org> >> wrote: >> >>> On 16 May 2016 at 05:50, Matias Elo <matias.elo@nokia.com> wrote: >>> >>> > Using memset to initialize struct odp_packet_hdr_t contents >>> > to 0 has a significant overhead. Instead, initialize only >>> > the required members of the struct. >>> > >>> > Signed-off-by: Matias Elo <matias.elo@nokia.com> >>> > --- >>> > platform/linux-generic/include/odp_packet_internal.h | 13 >>> +++++++++---- >>> > platform/linux-generic/odp_packet.c | 18 >>> > ++++++------------ >>> > 2 files changed, 15 insertions(+), 16 deletions(-) >>> > >>> > diff --git a/platform/linux-generic/include/odp_packet_internal.h >>> > b/platform/linux-generic/include/odp_packet_internal.h >>> > index 1306b05..c87bc9f 100644 >>> > --- a/platform/linux-generic/include/odp_packet_internal.h >>> > +++ b/platform/linux-generic/include/odp_packet_internal.h >>> > @@ -4,7 +4,6 @@ >>> > * SPDX-License-Identifier: BSD-3-Clause >>> > */ >>> > >>> - >>> > /** >>> > * @file >>> > * >>> > @@ -129,11 +128,16 @@ ODP_STATIC_ASSERT(sizeof(output_flags_t) == >>> > sizeof(uint32_t), >>> > >>> > /** >>> > * Internal Packet header >>> > + * >>> > + * To optimize fast path performance this struct is not initialized to >>> > zero in >>> > + * packet_init(). Because of this any new fields added must be >>> reviewed >>> > for >>> > + * initialization requirements. >>> > */ >>> >>> >>> This file will not contribute to the generated API documentation and I >>> >> >> It's not relevant to the API, only to the odp-linux implementation of the >> API. >> > > Exactly, as a user of the odp-linux implementation dont I want to know > that some fields are not set ? > No, because the internals of odp-linux (or any other implementation) are not visible to users. The only thing users see is what the API exposes. These optimizations have no impact on the API. The only people who care about this are the maintainers of odp-linux. > >> >> >>> think it should. This platform specific warning should appear there as a >>> doxygen @warning >>> Perhaps we need to start a trend of adding a doxygen platform specific >>> module called "PLATFORM SPECIFICS", then we can add information with the >>> @addtogroup platform_specifics and users across platforms will know where >>> to look for this sort of information. >> >> >> The best place for this sort of implementation documentation is in the >> comments in the C code itself. We just want a reminder to future >> maintainers of the code of a shortcut that was taken for performance >> reasons and the cautions that need to be observed as a result. >> > > I dont agree, too many implementations copy this code, they need to be > aware that a normal assumption is broken when they build upon it in their > implementation. > Blindly copying code is never a good idea. However, if you copy it its function is unchanged, so that has no effect. The only way you get in trouble is by copying and then changing other things without understanding what you're doing. That's always a recipe for trouble. > > >> >> If we want to have these sort of notes be collated as part of a >> "Maintainer's Guide" for odp-linux, that would seem a worthwhile project. >> Something to consider adding for Tiger Moth? >> > > I think it has to be in the implementer's guide since implementers often > use odp-linux as the starting point, we need to warn them. It also has to > be in the API guide if these structs will be seen by an application. > > I think it needs to happen before Monarch since this behaviour is > unexpected and we need to make sure it is noticed as the other > implementations home in on a fully compliant Monarch API > > Again, the fields that this patch deletes were never exposed as part of any ODP API. They were positioning for future extensions which never materialized. If they do (via new APIs) then they will be added back as needed. There's nothing nefarious going on here. The only change is that before you could add a new field and be somewhat sloppy about it. Now you have to think about what you're doing. > > >> >> >>> >>> typedef struct { >>> > /* common buffer header */ >>> > odp_buffer_hdr_t buf_hdr; >>> > >>> > + /* Following members are initialized by packet_init() */ >>> > input_flags_t input_flags; >>> > error_flags_t error_flags; >>> > output_flags_t output_flags; >>> > @@ -142,15 +146,16 @@ typedef struct { >>> > uint32_t l3_offset; /**< offset to L3 hdr, e.g. IPv4, IPv6 */ >>> > uint32_t l4_offset; /**< offset to L4 hdr (TCP, UDP, SCTP, also >>> > ICMP) */ >>> > >>> > - uint32_t l3_len; /**< Layer 3 length */ >>> > - uint32_t l4_len; /**< Layer 4 length */ >>> > - >>> > uint32_t frame_len; >>> > uint32_t headroom; >>> > uint32_t tailroom; >>> > >>> > odp_pktio_t input; >>> > >>> > + /* Members below are not initialized by packet_init() */ >>> > + uint32_t l3_len; /**< Layer 3 length */ >>> > + uint32_t l4_len; /**< Layer 4 length */ >>> > + >>> > uint32_t flow_hash; /**< Flow hash value */ >>> > odp_time_t timestamp; /**< Timestamp value */ >>> > >>> > diff --git a/platform/linux-generic/odp_packet.c >>> > b/platform/linux-generic/odp_packet.c >>> > index 4f523c9..8dde404 100644 >>> > --- a/platform/linux-generic/odp_packet.c >>> > +++ b/platform/linux-generic/odp_packet.c >>> > @@ -49,19 +49,11 @@ void packet_parse_reset(odp_packet_hdr_t *pkt_hdr) >>> > static void packet_init(pool_entry_t *pool, odp_packet_hdr_t *pkt_hdr, >>> > size_t size, int parse) >>> > { >>> > - /* >>> > - * Reset parser metadata. Note that we clear via memset to make >>> > - * this routine indepenent of any additional adds to packet >>> > metadata. >>> > - */ >>> > - const size_t start_offset = ODP_FIELD_SIZEOF(odp_packet_hdr_t, >>> > buf_hdr); >>> > - uint8_t *start; >>> > - size_t len; >>> > + pkt_hdr->input_flags.all = 0; >>> > + pkt_hdr->output_flags.all = 0; >>> > + pkt_hdr->error_flags.all = 0; >>> > >>> > - start = (uint8_t *)pkt_hdr + start_offset; >>> > - 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 = 0; >>> > pkt_hdr->l3_offset = ODP_PACKET_OFFSET_INVALID; >>> > pkt_hdr->l4_offset = ODP_PACKET_OFFSET_INVALID; >>> > >>> > @@ -79,6 +71,8 @@ static void packet_init(pool_entry_t *pool, >>> > odp_packet_hdr_t *pkt_hdr, >>> > pkt_hdr->tailroom = >>> > (pool->s.seg_size * pkt_hdr->buf_hdr.segcount) - >>> > (pool->s.headroom + size); >>> > + >>> > + pkt_hdr->input = ODP_PKTIO_INVALID; >>> > } >>> > >>> > odp_packet_t packet_alloc(odp_pool_t pool_hdl, uint32_t len, int >>> parse) >>> > -- >>> > 1.9.1 >>> > >>> > _______________________________________________ >>> > lng-odp mailing list >>> > lng-odp@lists.linaro.org >>> > https://lists.linaro.org/mailman/listinfo/lng-odp >>> > >>> >>> >>> >>> -- >>> Mike Holmes >>> Technical Manager - Linaro Networking Group >>> Linaro.org <http://www.linaro.org/> *│ *Open source software for ARM >>> SoCs >>> "Work should be fun and collaborative, the rest follows" >>> _______________________________________________ >>> lng-odp mailing list >>> lng-odp@lists.linaro.org >>> https://lists.linaro.org/mailman/listinfo/lng-odp >>> >> >> > > > -- > Mike Holmes > Technical Manager - Linaro Networking Group > Linaro.org <http://www.linaro.org/> *│ *Open source software for ARM SoCs > "Work should be fun and collaborative, the rest follows" > > >
On 18 May 2016 at 02:17, Elo, Matias (Nokia - FI/Espoo) < matias.elo@nokia.com> wrote: > > > > > *From:* Bill Fischofer [mailto:bill.fischofer@linaro.org] > *Sent:* Tuesday, May 17, 2016 9:46 PM > *To:* Mike Holmes <mike.holmes@linaro.org> > *Cc:* Elo, Matias (Nokia - FI/Espoo) <matias.elo@nokia.com>; lng-odp < > lng-odp@lists.linaro.org> > *Subject:* Re: [lng-odp] [PATCH v2 5/5] linux-generic: packet: initialize > only selected odp_packet_hdr_t members > > > > > > > > On Tue, May 17, 2016 at 12:42 PM, Mike Holmes <mike.holmes@linaro.org> > wrote: > > > > > > On 16 May 2016 at 18:52, Bill Fischofer <bill.fischofer@linaro.org> wrote: > > > > > > On Mon, May 16, 2016 at 9:25 AM, Mike Holmes <mike.holmes@linaro.org> > wrote: > > On 16 May 2016 at 05:50, Matias Elo <matias.elo@nokia.com> wrote: > > > Using memset to initialize struct odp_packet_hdr_t contents > > to 0 has a significant overhead. Instead, initialize only > > the required members of the struct. > > > > Signed-off-by: Matias Elo <matias.elo@nokia.com> > > --- > > platform/linux-generic/include/odp_packet_internal.h | 13 +++++++++---- > > platform/linux-generic/odp_packet.c | 18 > > ++++++------------ > > 2 files changed, 15 insertions(+), 16 deletions(-) > > > > diff --git a/platform/linux-generic/include/odp_packet_internal.h > > b/platform/linux-generic/include/odp_packet_internal.h > > index 1306b05..c87bc9f 100644 > > --- a/platform/linux-generic/include/odp_packet_internal.h > > +++ b/platform/linux-generic/include/odp_packet_internal.h > > @@ -4,7 +4,6 @@ > > * SPDX-License-Identifier: BSD-3-Clause > > */ > > > - > > /** > > * @file > > * > > @@ -129,11 +128,16 @@ ODP_STATIC_ASSERT(sizeof(output_flags_t) == > > sizeof(uint32_t), > > > > /** > > * Internal Packet header > > + * > > + * To optimize fast path performance this struct is not initialized to > > zero in > > + * packet_init(). Because of this any new fields added must be reviewed > > for > > + * initialization requirements. > > */ > > This file will not contribute to the generated API documentation and I > > > > It's not relevant to the API, only to the odp-linux implementation of the > API. > > > > Exactly, as a user of the odp-linux implementation dont I want to know > that some fields are not set ? > > > > No, because the internals of odp-linux (or any other implementation) are > not visible to users. The only thing users see is what the API exposes. > These optimizations have no impact on the API. The only people who care > about this are the maintainers of odp-linux. > > > > This is exactly what was done here. Either the uninitialized members are > not visible to the user at all (op_result, l3_len, l4_len) or the input > flags are used the check if the values are valid (flow_hash, timetamp). > > I am clearly missing something, I will leave it that I dont think we make it clear enough to any reusers and our own implementers becasue that /** doxygen comment is not published anywhere. > > > -Matias > > > > > > think it should. This platform specific warning should appear there as a > doxygen @warning > Perhaps we need to start a trend of adding a doxygen platform specific > module called "PLATFORM SPECIFICS", then we can add information with the > @addtogroup platform_specifics and users across platforms will know where > to look for this sort of information. > > > > The best place for this sort of implementation documentation is in the > comments in the C code itself. We just want a reminder to future > maintainers of the code of a shortcut that was taken for performance > reasons and the cautions that need to be observed as a result. > > > > I dont agree, too many implementations copy this code, they need to be > aware that a normal assumption is broken when they build upon it in their > implementation. > > > > Blindly copying code is never a good idea. However, if you copy it its > function is unchanged, so that has no effect. The only way you get in > trouble is by copying and then changing other things without understanding > what you're doing. That's always a recipe for trouble. > > > > > > > > If we want to have these sort of notes be collated as part of a > "Maintainer's Guide" for odp-linux, that would seem a worthwhile project. > Something to consider adding for Tiger Moth? > > > > I think it has to be in the implementer's guide since implementers often > use odp-linux as the starting point, we need to warn them. It also has to > be in the API guide if these structs will be seen by an application. > > > > I think it needs to happen before Monarch since this behaviour is > unexpected and we need to make sure it is noticed as the other > implementations home in on a fully compliant Monarch API > > > > > > Again, the fields that this patch deletes were never exposed as part of > any ODP API. They were positioning for future extensions which never > materialized. If they do (via new APIs) then they will be added back as > needed. There's nothing nefarious going on here. The only change is that > before you could add a new field and be somewhat sloppy about it. Now you > have to think about what you're doing. > > > > > > > > > > > typedef struct { > > /* common buffer header */ > > odp_buffer_hdr_t buf_hdr; > > > > + /* Following members are initialized by packet_init() */ > > input_flags_t input_flags; > > error_flags_t error_flags; > > output_flags_t output_flags; > > @@ -142,15 +146,16 @@ typedef struct { > > uint32_t l3_offset; /**< offset to L3 hdr, e.g. IPv4, IPv6 */ > > uint32_t l4_offset; /**< offset to L4 hdr (TCP, UDP, SCTP, also > > ICMP) */ > > > > - uint32_t l3_len; /**< Layer 3 length */ > > - uint32_t l4_len; /**< Layer 4 length */ > > - > > uint32_t frame_len; > > uint32_t headroom; > > uint32_t tailroom; > > > > odp_pktio_t input; > > > > + /* Members below are not initialized by packet_init() */ > > + uint32_t l3_len; /**< Layer 3 length */ > > + uint32_t l4_len; /**< Layer 4 length */ > > + > > uint32_t flow_hash; /**< Flow hash value */ > > odp_time_t timestamp; /**< Timestamp value */ > > > > diff --git a/platform/linux-generic/odp_packet.c > > b/platform/linux-generic/odp_packet.c > > index 4f523c9..8dde404 100644 > > --- a/platform/linux-generic/odp_packet.c > > +++ b/platform/linux-generic/odp_packet.c > > @@ -49,19 +49,11 @@ void packet_parse_reset(odp_packet_hdr_t *pkt_hdr) > > static void packet_init(pool_entry_t *pool, odp_packet_hdr_t *pkt_hdr, > > size_t size, int parse) > > { > > - /* > > - * Reset parser metadata. Note that we clear via memset to make > > - * this routine indepenent of any additional adds to packet > > metadata. > > - */ > > - const size_t start_offset = ODP_FIELD_SIZEOF(odp_packet_hdr_t, > > buf_hdr); > > - uint8_t *start; > > - size_t len; > > + pkt_hdr->input_flags.all = 0; > > + pkt_hdr->output_flags.all = 0; > > + pkt_hdr->error_flags.all = 0; > > > > - start = (uint8_t *)pkt_hdr + start_offset; > > - 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 = 0; > > pkt_hdr->l3_offset = ODP_PACKET_OFFSET_INVALID; > > pkt_hdr->l4_offset = ODP_PACKET_OFFSET_INVALID; > > > > @@ -79,6 +71,8 @@ static void packet_init(pool_entry_t *pool, > > odp_packet_hdr_t *pkt_hdr, > > pkt_hdr->tailroom = > > (pool->s.seg_size * pkt_hdr->buf_hdr.segcount) - > > (pool->s.headroom + size); > > + > > + pkt_hdr->input = ODP_PKTIO_INVALID; > > } > > > > odp_packet_t packet_alloc(odp_pool_t pool_hdl, uint32_t len, int parse) > > -- > > 1.9.1 > > > > _______________________________________________ > > lng-odp mailing list > > lng-odp@lists.linaro.org > > https://lists.linaro.org/mailman/listinfo/lng-odp > > > > > -- > Mike Holmes > Technical Manager - Linaro Networking Group > > Linaro.org <http://www.linaro.org/> *│ *Open source software for ARM SoCs > "Work should be fun and collaborative, the rest follows" > > _______________________________________________ > lng-odp mailing list > lng-odp@lists.linaro.org > https://lists.linaro.org/mailman/listinfo/lng-odp > > > > > > > > -- > > Mike Holmes > > Technical Manager - Linaro Networking Group > > Linaro.org <http://www.linaro.org/> *│ *Open source software for ARM SoCs > > "Work should be fun and collaborative, the rest follows" > > > > >
On Wed, May 18, 2016 at 12:31 PM, Mike Holmes <mike.holmes@linaro.org> wrote: > > > On 18 May 2016 at 02:17, Elo, Matias (Nokia - FI/Espoo) < > matias.elo@nokia.com> wrote: > >> >> >> >> >> *From:* Bill Fischofer [mailto:bill.fischofer@linaro.org] >> *Sent:* Tuesday, May 17, 2016 9:46 PM >> *To:* Mike Holmes <mike.holmes@linaro.org> >> *Cc:* Elo, Matias (Nokia - FI/Espoo) <matias.elo@nokia.com>; lng-odp < >> lng-odp@lists.linaro.org> >> *Subject:* Re: [lng-odp] [PATCH v2 5/5] linux-generic: packet: >> initialize only selected odp_packet_hdr_t members >> >> >> >> >> >> >> >> On Tue, May 17, 2016 at 12:42 PM, Mike Holmes <mike.holmes@linaro.org> >> wrote: >> >> >> >> >> >> On 16 May 2016 at 18:52, Bill Fischofer <bill.fischofer@linaro.org> >> wrote: >> >> >> >> >> >> On Mon, May 16, 2016 at 9:25 AM, Mike Holmes <mike.holmes@linaro.org> >> wrote: >> >> On 16 May 2016 at 05:50, Matias Elo <matias.elo@nokia.com> wrote: >> >> > Using memset to initialize struct odp_packet_hdr_t contents >> > to 0 has a significant overhead. Instead, initialize only >> > the required members of the struct. >> > >> > Signed-off-by: Matias Elo <matias.elo@nokia.com> >> > --- >> > platform/linux-generic/include/odp_packet_internal.h | 13 +++++++++---- >> > platform/linux-generic/odp_packet.c | 18 >> > ++++++------------ >> > 2 files changed, 15 insertions(+), 16 deletions(-) >> > >> > diff --git a/platform/linux-generic/include/odp_packet_internal.h >> > b/platform/linux-generic/include/odp_packet_internal.h >> > index 1306b05..c87bc9f 100644 >> > --- a/platform/linux-generic/include/odp_packet_internal.h >> > +++ b/platform/linux-generic/include/odp_packet_internal.h >> > @@ -4,7 +4,6 @@ >> > * SPDX-License-Identifier: BSD-3-Clause >> > */ >> > >> - >> > /** >> > * @file >> > * >> > @@ -129,11 +128,16 @@ ODP_STATIC_ASSERT(sizeof(output_flags_t) == >> > sizeof(uint32_t), >> > >> > /** >> > * Internal Packet header >> > + * >> > + * To optimize fast path performance this struct is not initialized to >> > zero in >> > + * packet_init(). Because of this any new fields added must be reviewed >> > for >> > + * initialization requirements. >> > */ >> >> This file will not contribute to the generated API documentation and I >> >> >> >> It's not relevant to the API, only to the odp-linux implementation of the >> API. >> >> >> >> Exactly, as a user of the odp-linux implementation dont I want to know >> that some fields are not set ? >> >> >> >> No, because the internals of odp-linux (or any other implementation) are >> not visible to users. The only thing users see is what the API exposes. >> These optimizations have no impact on the API. The only people who care >> about this are the maintainers of odp-linux. >> >> >> >> This is exactly what was done here. Either the uninitialized members are >> not visible to the user at all (op_result, l3_len, l4_len) or the input >> flags are used the check if the values are valid (flow_hash, timetamp). >> >> > I am clearly missing something, I will leave it that I dont think we make > it clear enough to any reusers and our own implementers becasue that /** > doxygen comment is not published anywhere. > If someone is reusing code they should be looking at that code (and the C comments within it) before doing so blindly. That's why I wanted to see the C comments added as a note to the reader of the code. I don't think you expect someone to look elsewhere for that sort of information. Besides, we currently have no place to put such doxygen output as this would logically be part of the internal documentation for a specific implementation of ODP rather than part of either the general or platform-specific documentation of the ODP API spec. Basically what these changes do is remove otherwise unused internal variables. That's all. They were put there originally in anticipation that they might be externalized via ODP APIs, but that hasn't happened. If it does happen in the future then this code (as part of the implementation of those new APIs) would be modified. For now, given that Matias has demonstrated a meaningful performance advantage to deleting them, that seems a very reasonable thing to do. > >> >> >> -Matias >> >> >> >> >> >> think it should. This platform specific warning should appear there as a >> doxygen @warning >> Perhaps we need to start a trend of adding a doxygen platform specific >> module called "PLATFORM SPECIFICS", then we can add information with the >> @addtogroup platform_specifics and users across platforms will know where >> to look for this sort of information. >> >> >> >> The best place for this sort of implementation documentation is in the >> comments in the C code itself. We just want a reminder to future >> maintainers of the code of a shortcut that was taken for performance >> reasons and the cautions that need to be observed as a result. >> >> >> >> I dont agree, too many implementations copy this code, they need to be >> aware that a normal assumption is broken when they build upon it in their >> implementation. >> >> >> >> Blindly copying code is never a good idea. However, if you copy it its >> function is unchanged, so that has no effect. The only way you get in >> trouble is by copying and then changing other things without understanding >> what you're doing. That's always a recipe for trouble. >> >> >> >> >> >> >> >> If we want to have these sort of notes be collated as part of a >> "Maintainer's Guide" for odp-linux, that would seem a worthwhile project. >> Something to consider adding for Tiger Moth? >> >> >> >> I think it has to be in the implementer's guide since implementers often >> use odp-linux as the starting point, we need to warn them. It also has to >> be in the API guide if these structs will be seen by an application. >> >> >> >> I think it needs to happen before Monarch since this behaviour is >> unexpected and we need to make sure it is noticed as the other >> implementations home in on a fully compliant Monarch API >> >> >> >> >> >> Again, the fields that this patch deletes were never exposed as part of >> any ODP API. They were positioning for future extensions which never >> materialized. If they do (via new APIs) then they will be added back as >> needed. There's nothing nefarious going on here. The only change is that >> before you could add a new field and be somewhat sloppy about it. Now you >> have to think about what you're doing. >> >> >> >> >> >> >> >> >> >> >> typedef struct { >> > /* common buffer header */ >> > odp_buffer_hdr_t buf_hdr; >> > >> > + /* Following members are initialized by packet_init() */ >> > input_flags_t input_flags; >> > error_flags_t error_flags; >> > output_flags_t output_flags; >> > @@ -142,15 +146,16 @@ typedef struct { >> > uint32_t l3_offset; /**< offset to L3 hdr, e.g. IPv4, IPv6 */ >> > uint32_t l4_offset; /**< offset to L4 hdr (TCP, UDP, SCTP, also >> > ICMP) */ >> > >> > - uint32_t l3_len; /**< Layer 3 length */ >> > - uint32_t l4_len; /**< Layer 4 length */ >> > - >> > uint32_t frame_len; >> > uint32_t headroom; >> > uint32_t tailroom; >> > >> > odp_pktio_t input; >> > >> > + /* Members below are not initialized by packet_init() */ >> > + uint32_t l3_len; /**< Layer 3 length */ >> > + uint32_t l4_len; /**< Layer 4 length */ >> > + >> > uint32_t flow_hash; /**< Flow hash value */ >> > odp_time_t timestamp; /**< Timestamp value */ >> > >> > diff --git a/platform/linux-generic/odp_packet.c >> > b/platform/linux-generic/odp_packet.c >> > index 4f523c9..8dde404 100644 >> > --- a/platform/linux-generic/odp_packet.c >> > +++ b/platform/linux-generic/odp_packet.c >> > @@ -49,19 +49,11 @@ void packet_parse_reset(odp_packet_hdr_t *pkt_hdr) >> > static void packet_init(pool_entry_t *pool, odp_packet_hdr_t *pkt_hdr, >> > size_t size, int parse) >> > { >> > - /* >> > - * Reset parser metadata. Note that we clear via memset to make >> > - * this routine indepenent of any additional adds to packet >> > metadata. >> > - */ >> > - const size_t start_offset = ODP_FIELD_SIZEOF(odp_packet_hdr_t, >> > buf_hdr); >> > - uint8_t *start; >> > - size_t len; >> > + pkt_hdr->input_flags.all = 0; >> > + pkt_hdr->output_flags.all = 0; >> > + pkt_hdr->error_flags.all = 0; >> > >> > - start = (uint8_t *)pkt_hdr + start_offset; >> > - 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 = 0; >> > pkt_hdr->l3_offset = ODP_PACKET_OFFSET_INVALID; >> > pkt_hdr->l4_offset = ODP_PACKET_OFFSET_INVALID; >> > >> > @@ -79,6 +71,8 @@ static void packet_init(pool_entry_t *pool, >> > odp_packet_hdr_t *pkt_hdr, >> > pkt_hdr->tailroom = >> > (pool->s.seg_size * pkt_hdr->buf_hdr.segcount) - >> > (pool->s.headroom + size); >> > + >> > + pkt_hdr->input = ODP_PKTIO_INVALID; >> > } >> > >> > odp_packet_t packet_alloc(odp_pool_t pool_hdl, uint32_t len, int parse) >> > -- >> > 1.9.1 >> > >> > _______________________________________________ >> > lng-odp mailing list >> > lng-odp@lists.linaro.org >> > https://lists.linaro.org/mailman/listinfo/lng-odp >> > >> >> >> -- >> Mike Holmes >> Technical Manager - Linaro Networking Group >> >> Linaro.org <http://www.linaro.org/> *│ *Open source software for ARM SoCs >> "Work should be fun and collaborative, the rest follows" >> >> _______________________________________________ >> lng-odp mailing list >> lng-odp@lists.linaro.org >> https://lists.linaro.org/mailman/listinfo/lng-odp >> >> >> >> >> >> >> >> -- >> >> Mike Holmes >> >> Technical Manager - Linaro Networking Group >> >> Linaro.org <http://www.linaro.org/> *│ *Open source software for ARM SoCs >> >> "Work should be fun and collaborative, the rest follows" >> >> >> >> >> > > > > -- > Mike Holmes > Technical Manager - Linaro Networking Group > Linaro.org <http://www.linaro.org/> *│ *Open source software for ARM SoCs > "Work should be fun and collaborative, the rest follows" > > >
diff --git a/platform/linux-generic/include/odp_packet_internal.h b/platform/linux-generic/include/odp_packet_internal.h index 1306b05..c87bc9f 100644 --- a/platform/linux-generic/include/odp_packet_internal.h +++ b/platform/linux-generic/include/odp_packet_internal.h @@ -4,7 +4,6 @@ * SPDX-License-Identifier: BSD-3-Clause */ - /** * @file * @@ -129,11 +128,16 @@ ODP_STATIC_ASSERT(sizeof(output_flags_t) == sizeof(uint32_t), /** * Internal Packet header + * + * To optimize fast path performance this struct is not initialized to zero in + * packet_init(). Because of this any new fields added must be reviewed for + * initialization requirements. */ typedef struct { /* common buffer header */ odp_buffer_hdr_t buf_hdr; + /* Following members are initialized by packet_init() */ input_flags_t input_flags; error_flags_t error_flags; output_flags_t output_flags; @@ -142,15 +146,16 @@ typedef struct { uint32_t l3_offset; /**< offset to L3 hdr, e.g. IPv4, IPv6 */ uint32_t l4_offset; /**< offset to L4 hdr (TCP, UDP, SCTP, also ICMP) */ - uint32_t l3_len; /**< Layer 3 length */ - uint32_t l4_len; /**< Layer 4 length */ - uint32_t frame_len; uint32_t headroom; uint32_t tailroom; odp_pktio_t input; + /* Members below are not initialized by packet_init() */ + uint32_t l3_len; /**< Layer 3 length */ + uint32_t l4_len; /**< Layer 4 length */ + uint32_t flow_hash; /**< Flow hash value */ odp_time_t timestamp; /**< Timestamp value */ diff --git a/platform/linux-generic/odp_packet.c b/platform/linux-generic/odp_packet.c index 4f523c9..8dde404 100644 --- a/platform/linux-generic/odp_packet.c +++ b/platform/linux-generic/odp_packet.c @@ -49,19 +49,11 @@ void packet_parse_reset(odp_packet_hdr_t *pkt_hdr) static void packet_init(pool_entry_t *pool, odp_packet_hdr_t *pkt_hdr, size_t size, int parse) { - /* - * Reset parser metadata. Note that we clear via memset to make - * this routine indepenent of any additional adds to packet metadata. - */ - const size_t start_offset = ODP_FIELD_SIZEOF(odp_packet_hdr_t, buf_hdr); - uint8_t *start; - size_t len; + pkt_hdr->input_flags.all = 0; + pkt_hdr->output_flags.all = 0; + pkt_hdr->error_flags.all = 0; - start = (uint8_t *)pkt_hdr + start_offset; - 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 = 0; pkt_hdr->l3_offset = ODP_PACKET_OFFSET_INVALID; pkt_hdr->l4_offset = ODP_PACKET_OFFSET_INVALID; @@ -79,6 +71,8 @@ static void packet_init(pool_entry_t *pool, odp_packet_hdr_t *pkt_hdr, pkt_hdr->tailroom = (pool->s.seg_size * pkt_hdr->buf_hdr.segcount) - (pool->s.headroom + size); + + pkt_hdr->input = ODP_PKTIO_INVALID; } odp_packet_t packet_alloc(odp_pool_t pool_hdl, uint32_t len, int parse)
Using memset to initialize struct odp_packet_hdr_t contents to 0 has a significant overhead. Instead, initialize only the required members of the struct. Signed-off-by: Matias Elo <matias.elo@nokia.com> --- platform/linux-generic/include/odp_packet_internal.h | 13 +++++++++---- platform/linux-generic/odp_packet.c | 18 ++++++------------ 2 files changed, 15 insertions(+), 16 deletions(-)