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. > 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 > > [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 > _______________________________________________ lng-odp mailing list [email protected] https://lists.linaro.org/mailman/listinfo/lng-odp
