On 18 May 2016 at 02:17, Elo, Matias (Nokia - FI/Espoo) < [email protected]> wrote:
> > > > > *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]> > wrote: > > > > > > 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 ? > > > > 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 > > [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" > > > > > -- 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
