On 16 May 2016 at 18:52, Bill Fischofer <[email protected]> wrote:
> > > On Mon, May 16, 2016 at 9:25 AM, Mike Holmes <[email protected]> > wrote: > >> On 16 May 2016 at 05:50, Matias Elo <[email protected]> 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 <[email protected]> >> > --- >> > 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 >> > [email protected] >> > 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 >> [email protected] >> 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 [email protected] https://lists.linaro.org/mailman/listinfo/lng-odp
