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

[Simon Kuenzer <simon.kuenzer@xxxxxxxxx>]

On 06.11.20 12:48, Sharan Santhanam wrote:
> Hello Simon,
>
> Thanks for the work. i have a minor comment inline
>
> Reviewed-by: Sharan Santhanam <sharan.santhanam@xxxxxxxxx>
>
> Thanks & Regards
>
> Sharan
>
> On 10/30/20 7:31 PM, Simon Kuenzer wrote:
> > Adds an alignment parameter to `lwip_alloc_netbuf()`. The netbuf
> > buffer area (`m->buf`) will be aligned.
> >
> > 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 ba7d41c..7b689f9 100644
> > --- a/uknetdev.c
> > +++ b/uknetdev.c
> > @@ -99,6 +99,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,
> Maybe a comment why is this align?
Sure, no problem., I will add a comment. With this commit, we do not do 
an packet alignement yet, just introducing the interface. I wanted to 
keep this patch as simple as possible and make use of the updated 
interface with the following patch.
Thanks,

Simon

> >                         lwip_data->rx_headroom);
> >           if (!nb[i]) {
> >               /* we run out of memory */