From: Bill Fischofer [mailto:bill.fischo...@linaro.org] Sent: Monday, November 14, 2016 3:37 AM To: Savolainen, Petri (Nokia - FI/Espoo) <petri.savolai...@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> +{ + 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> +} + +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) -Petri </reply to HTML mail> odp_packet_seg_t odp_packet_last_seg(odp_packet_t pkt) { - (void)pkt; + odp_packet_hdr_t *pkt_hdr = odp_packet_hdr(pkt); - /* Only one segment */ - return (odp_packet_seg_t)pkt; + return packet_last_seg(pkt_hdr); }