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
>
>

Reply via email to