From: Bill Fischofer [mailto:bill.fischo...@linaro.org] Sent: Tuesday, November 15, 2016 3:03 PM 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 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.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> 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> -Petri
