Re: [UNIKRAFT/LWIP PATCH v5 4/5] netbuf: Aligned buffers with `lwip_alloc_netbuf()`

[Sharan Santhanam <sharan.santhanam@xxxxxxxxx>]

Hello Simon,

Thanks for the work.

Reviewed-by: Sharan Santhanam <sharan.santhanam@xxxxxxxxx>

Thanks & Regards

Sharan

On 11/9/20 5:06 PM, Simon Kuenzer wrote:
> Adds an alignment parameter to `lwip_alloc_netbuf()` that enables
> alignment of the buffer area (`m->buf`).
>
> Signed-off-by: Simon Kuenzer <simon.kuenzer@xxxxxxxxx>
> ---
>   netbuf.c   | 26 ++++++--------------------
>   netbuf.h   |  2 +-
>   uknetdev.c |  1 +
>   3 files changed, 8 insertions(+), 21 deletions(-)
>
> diff --git a/netbuf.c b/netbuf.c
> index 4878f74..97c129c 100644
> --- a/netbuf.c
> +++ b/netbuf.c
> @@ -50,32 +50,20 @@ static void _netbuf_free(struct pbuf *p)
>   }
>   
>   struct uk_netbuf *lwip_alloc_netbuf(struct uk_alloc *a, size_t alloc_size,
> -                                   size_t headroom)
> +                                   size_t alloc_align, uint16_t headroom)
>   {
> -       void *allocation;
>         struct uk_netbuf *b;
>         struct _netbuf_pbuf *np;
>   
> -	allocation = uk_malloc(a, alloc_size);
> -       if (unlikely(!allocation))
> -               goto err_out;
> -
> -       b = uk_netbuf_prepare_buf(allocation, alloc_size,
> -                                 headroom, sizeof(struct _netbuf_pbuf), NULL);
> +       b = uk_netbuf_alloc_buf(a, alloc_size, alloc_align,
> +                               headroom, sizeof(struct _netbuf_pbuf), NULL);
>         if (unlikely(!b)) {
>                 LWIP_DEBUGF(PBUF_DEBUG,
> -                           ("Failed to initialize netbuf with encapsulated pbuf: requested headroom: 
> %"__PRIsz", alloc_size: %"__PRIsz"\n",
> -                            headroom, alloc_size));
> -               goto err_free_allocation;
> +                           ("Failed to allocate netbuf with encapsulated pbuf: requested headroom: 
> %"__PRIu16", size: %"__PRIsz", alignement: %"__PRIsz"\n",
> +                            headroom, alloc_size, alloc_align));
> +               goto err_out;
>         }
>   
> -	/*
> -        * Register allocator so that uk_netbuf_free() will
> -        * return our memory back to this allocator when free'ing
> -        * this netbuf
> -        */
> -       b->_a = a;
> -
>         /* Fill-out meta data */
>         np = (struct _netbuf_pbuf *) uk_netbuf_get_priv(b);
>         memset(np, 0, sizeof(struct _netbuf_pbuf));
> @@ -97,8 +85,6 @@ struct uk_netbuf *lwip_alloc_netbuf(struct uk_alloc *a, 
> size_t alloc_size,
>                      b, b->buflen, uk_netbuf_headroom(b)));
>         return b;
>   
> -err_free_allocation:
> -       uk_free(a, allocation);
>   err_out:
>         return NULL;
>   }
> diff --git a/netbuf.h b/netbuf.h
> index 1492065..db5adf7 100644
> --- a/netbuf.h
> +++ b/netbuf.h
> @@ -61,7 +61,7 @@ struct _netbuf_pbuf {
>    * before handing over the embedded pbuf to the network stack.
>    */
>   struct uk_netbuf *lwip_alloc_netbuf(struct uk_alloc *a, size_t alloc_size,
> -                                   size_t headroom);
> +                                   size_t alloc_align, uint16_t headroom);
>   
>   /**
>    * Returns the reference of the embedded pbuf of a netbuf
> diff --git a/uknetdev.c b/uknetdev.c
> index 1d8c5e8..881eb60 100644
> --- a/uknetdev.c
> +++ b/uknetdev.c
> @@ -120,6 +120,7 @@ static uint16_t netif_alloc_rxpkts(void *argp, struct 
> uk_netbuf *nb[],
>         for (i = 0; i < count; ++i) {
>                 nb[i] = lwip_alloc_netbuf(lwip_data->pkt_a,
>                                           UKNETDEV_BUFLEN,
> +                                         1 /* no alignment */,
>                                           lwip_data->dev_info.nb_encap_rx);
>                 if (!nb[i]) {
>                         /* we run out of memory */