From: Bill Fischofer [mailto:[email protected]] Sent: Tuesday, May 17, 2016 9:46 PM To: Mike Holmes <[email protected]> Cc: Elo, Matias (Nokia - FI/Espoo) <[email protected]>; lng-odp <[email protected]> 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 <[email protected]<mailto:[email protected]>> wrote: On 16 May 2016 at 18:52, Bill Fischofer <[email protected]<mailto:[email protected]>> wrote: On Mon, May 16, 2016 at 9:25 AM, Mike Holmes <[email protected]<mailto:[email protected]>> wrote: On 16 May 2016 at 05:50, Matias Elo <[email protected]<mailto:[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]<mailto:[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 ? 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). -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 > [email protected]<mailto:[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]<mailto:[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
