On Tue, Nov 15, 2016 at 8:13 AM, Savolainen, Petri (Nokia - FI/Espoo) < petri.savolai...@nokia-bell-labs.com> wrote:
> > > From: Bill Fischofer [mailto:bill.fischo...@linaro.org] > Sent: Tuesday, November 15, 2016 3:03 PM > To: Savolainen, Petri (Nokia - FI/Espoo) <petri.savolainen@nokia-bell- > labs.com> > Cc: lng-odp-forward <lng-odp@lists.linaro.org> > Subject: Re: [lng-odp] [API-NEXT PATCH v3 15/19] linux-gen: packet: added > support for segmented packets > > > > On Tue, Nov 15, 2016 at 6:20 AM, Savolainen, Petri (Nokia - FI/Espoo) < > petri.savolai...@nokia-bell-labs.com> wrote: > > > From: Bill Fischofer [mailto:bill.fischo...@linaro.org] > Sent: Monday, November 14, 2016 3:37 AM > To: Savolainen, Petri (Nokia - FI/Espoo) <petri.savolainen@nokia-bell- > labs.com> > Cc: lng-odp-forward <lng-odp@lists.linaro.org> > Subject: Re: [lng-odp] [API-NEXT PATCH v3 15/19] linux-gen: packet: added > support for segmented packets > > > > On Thu, Nov 10, 2016 at 5:07 AM, Petri Savolainen < > petri.savolai...@nokia.com> wrote: > Added support for multi-segmented packets. The first segments > is the packet descriptor, which contains all metadata and > pointers to other segments. > > Signed-off-by: Petri Savolainen <petri.savolai...@nokia.com> > --- > .../include/odp/api/plat/packet_types.h | 6 +- > .../linux-generic/include/odp_buffer_inlines.h | 11 - > .../linux-generic/include/odp_buffer_internal.h | 23 +- > .../linux-generic/include/odp_config_internal.h | 39 +- > .../linux-generic/include/odp_packet_internal.h | 80 +-- > platform/linux-generic/include/odp_pool_internal.h | 3 - > platform/linux-generic/odp_buffer.c | 8 +- > platform/linux-generic/odp_crypto.c | 8 +- > platform/linux-generic/odp_packet.c | 712 > +++++++++++++++++---- > platform/linux-generic/odp_pool.c | 123 ++-- > platform/linux-generic/pktio/netmap.c | 4 +- > platform/linux-generic/pktio/socket.c | 3 +- > 12 files changed, 692 insertions(+), 328 deletions(-) > > > > > diff --git a/platform/linux-generic/odp_packet.c > b/platform/linux-generic/odp_packet.c > index 2eee775..a5c6ff4 100644 > --- a/platform/linux-generic/odp_packet.c > +++ b/platform/linux-generic/odp_packet.c > @@ -20,12 +20,155 @@ > #include <stdio.h> > #include <inttypes.h> > > -/* > - * > - * Alloc and free > - * ******************************************************** > - * > - */ > +static inline odp_packet_t packet_handle(odp_packet_hdr_t *pkt_hdr) > +{ > + return (odp_packet_t)pkt_hdr->buf_hdr.handle.handle; > +} > + > +static inline odp_buffer_t buffer_handle(odp_packet_hdr_t *pkt_hdr) > +{ > + return pkt_hdr->buf_hdr.handle.handle; > +} > + > +static inline uint32_t packet_seg_len(odp_packet_hdr_t *pkt_hdr, > + uint32_t seg_idx) > +{ > + return pkt_hdr->buf_hdr.seg[seg_idx].len; > +} > + > +static inline void *packet_seg_data(odp_packet_hdr_t *pkt_hdr, uint32_t > seg_idx) > > This is more usefully typed as returning uint8_t * rather than void * to > permit easy arithmetic on the result. The uint8_t * converts automatically > to void * returns for the external API calls that use this. > > <reply to HTML mail> > > This is actually the only place packet_seg_data() is called. The API > function returns (void *), so the inline function fits that. > > void *odp_packet_seg_data(odp_packet_t pkt, odp_packet_seg_t seg) > { > odp_packet_hdr_t *pkt_hdr = odp_packet_hdr(pkt); > > if (odp_unlikely(seg >= pkt_hdr->buf_hdr.segcount)) > return NULL; > > return packet_seg_data(pkt_hdr, seg); > } > > Also (void *) can be assigned to any pointer type without cast. For > example: > > uint8_t *data; > data = packet_seg_data(pkt_hdr, seg); > > > So, I think there's no benefit to change. > > </reply to HTML mail> > > I do use this function in the packet reference extensions, and need to > perform arithmetic on the return, which is why having it return uint8_t * > rather than void * is more useful. I can include this change as part of my > patch series if you don't want to make it part of this base patch. > > <2nd reply to HTML mail> > > It's better to change it on a future patch that actually needs the change. > > </2nd reply to HTML mail> > > > > > +{ > + return pkt_hdr->buf_hdr.seg[seg_idx].data; > +} > + > +static inline int packet_last_seg(odp_packet_hdr_t *pkt_hdr) > +{ > + if (CONFIG_PACKET_MAX_SEGS == 1) > + return 0; > + else > + return pkt_hdr->buf_hdr.segcount - 1; > +} > + > +static inline uint32_t packet_first_seg_len(odp_packet_hdr_t *pkt_hdr) > +{ > + return packet_seg_len(pkt_hdr, 0); > +} > + > +static inline uint32_t packet_last_seg_len(odp_packet_hdr_t *pkt_hdr) > +{ > + int last = packet_last_seg(pkt_hdr); > + > + return packet_seg_len(pkt_hdr, last); > +} > + > +static inline void *packet_data(odp_packet_hdr_t *pkt_hdr) > +{ > + return pkt_hdr->buf_hdr.seg[0].data; > > > Given the earlier definition of the packet_seg_data helper this would more > consistently be: > > return packet_seg_data(pkt_hdr, 0); > > > <reply to HTML mail> > > There's no need to change the return type of packet_seg_data() to be able > to use it here. > > Anyway, I decided not to hide buf_hdr.seg[0].data reads behind a function > call, since this way it's easy to find all reads and *writes* of it > throughout the file. > > </reply to HTML mail> > > > Then I'd question why have this function at all if the intent is not to > help hide the particulars of the seg structure contained in the buf_hdr? I > thought this was a useful abstraction so it's use as an internal API should > be encouraged. > > <2nd reply to HTML mail> > > I didn't end up using it due to the reason above. It's internal to the > file and used only once, so I could remove it. Do you want it to be removed > ? > > </2nd reply to HTML mail> > > > +} > + > +static inline void *packet_tail(odp_packet_hdr_t *pkt_hdr) > +{ > + int last = packet_last_seg(pkt_hdr); > + uint32_t seg_len = pkt_hdr->buf_hdr.seg[last].len; > + > + return pkt_hdr->buf_hdr.seg[last].data + seg_len; > +} > + > +static inline void push_head(odp_packet_hdr_t *pkt_hdr, uint32_t len) > +{ > + pkt_hdr->headroom -= len; > + pkt_hdr->frame_len += len; > + pkt_hdr->buf_hdr.seg[0].data -= len; > + pkt_hdr->buf_hdr.seg[0].len += len; > +} > > > > > int odp_packet_num_segs(odp_packet_t pkt) > { > - return odp_packet_hdr(pkt)->buf_hdr.segcount; > + odp_packet_hdr_t *pkt_hdr = odp_packet_hdr(pkt); > + > + return pkt_hdr->buf_hdr.segcount; > } > > odp_packet_seg_t odp_packet_first_seg(odp_packet_t pkt) > { > - return (odp_packet_seg_t)pkt; > + (void)pkt; > + > + return 0; > } > > This should be written as: > > odp_packet_seg_t odp_packet_first_seg(odp_packet_t pkt ODP_UNUSED) > { > return 0; > } > > > <reply to HTML mail> > > This is mostly matter of taste. We use cast to (void) in many places > already. Also (void) is part of C language (I believe), whereas ODP_UNUSED > rely on an __attribute__ that all compilers might not implement (on > function prototype at least). So, actually (void) seems to me more > portable than ODP_UNUSED. > > Maybe API definition of ODP_UNUSED should be changed to ODP_UNUSED(x), > which would not be defined on function prototype but on function body and > would be implemented like this, > > #define ODP_UNUSED(x) (void)(x) > > </reply to HTML mail> > > These discussions were held and settled quite some time ago, which is why > we defined the ODP_UNUSED idiom. Since we have a well-defined and > established mechanism for handling unused parameters, we should use it > consistently. > > <2nd reply to HTML mail> > > Actually, the definition of '#define ODP_UNUSED(x) (void)(x)' > implementation was never discussed. We just wrapped > "__attribute__((__unused__))" of GCC. That's why I'm questioning it here > now. What if your compiler X checks for unused params but does not support > the same or similar attribute *inside* function prototype ? > > We could also remove ODP_UNUSED from the API if cast to (void) is a valid > C language feature. > > Anyway, for this series I'll send a v4 with ODP_UNUSED. > > </2nd reply to HTML mail> > If you wish. Alternately I can fold that into my patch that goes on top of this one and I can review your series as-is since it otherwise looks fine and clearly is something we want. > > > -Petri > >
