Re: [UNIKRAFT PATCH 2/4] lib/uknetdev: netbuf: Metadata at the end of an allocation

[Sharan Santhanam <sharan.santhanam@xxxxxxxxx>]

Thanks for the work

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

On 10/30/20 2:41 PM, Simon Kuenzer wrote:
> Place the netbuf metadata (`struct netbuf`) at the end of an allocation.
> This enables forwarding possible alignments of an underlying allocation
> to the buffer area of a netbuf. The metadata will no longer cause an offset
> of the buffer start address (`m->buf`).
>
> Signed-off-by: Simon Kuenzer <simon.kuenzer@xxxxxxxxx>
> ---
>   lib/uknetdev/include/uk/netbuf.h | 29 +++++-----
>   lib/uknetdev/netbuf.c            | 97 ++++++++++++--------------------
>   2 files changed, 50 insertions(+), 76 deletions(-)
>
> diff --git a/lib/uknetdev/include/uk/netbuf.h b/lib/uknetdev/include/uk/netbuf.h
> index f21d082b..ea803ae8 100644
> --- a/lib/uknetdev/include/uk/netbuf.h
> +++ b/lib/uknetdev/include/uk/netbuf.h
> @@ -57,18 +57,6 @@ typedef void (*uk_netbuf_dtor_t)(struct uk_netbuf *);
>    * The structure can be chained to describe a packet with multiple scattered
>    * buffers.
>    *
> - * NETBUF
> - *                  +----------------------+
> - *                  |   struct uk_netbuf   |
> - *                  |                      |
> - *                  +----------------------+
> - *
> - * PRIVATE META DATA
> - *         *priv -> +----------------------+
> - *                  |      private meta    |
> - *                  |       data area      |
> - *                  +----------------------+
> - *
>    * PACKET BUFFER
>    *          *buf -> +----------------------+ \
>    *                  |      HEAD ROOM       |  |
> @@ -82,6 +70,17 @@ typedef void (*uk_netbuf_dtor_t)(struct uk_netbuf *);
>    *                  |      TAIL ROOM       |  |
>    * *buf + buflen -> +----------------------+ /
>    *
> + * NETBUF
> + *                  +----------------------+
> + *                  |   struct uk_netbuf   |
> + *                  |                      |
> + *                  +----------------------+
> + *
> + * PRIVATE META DATA
> + *         *priv -> +----------------------+
> + *                  |      private meta    |
> + *                  |       data area      |
> + *                  +----------------------+
>    *
>    * The private data area is intended for glue code that wants to embed stack-
>    * specific data to the netbuf (e.g., `struct pbuf` for lwIP). This avoids
> @@ -237,7 +236,8 @@ struct uk_netbuf *uk_netbuf_alloc_indir(struct uk_alloc *a,
>   
>   /**
>    * Allocate and initialize netbuf with data buffer area.
> - * m->len is initialized with 0.
> + * m->len is initialized with 0. Metadata (struct uknetbuf, priv)
> + * is placed at the end of the according allocation.
>    * @param a
>    *   Allocator to be used for allocating `struct uk_netbuf` and the
>    *   corresponding buffer area (single allocation).
> @@ -268,7 +268,8 @@ struct uk_netbuf *uk_netbuf_alloc_buf(struct uk_alloc *a, 
> size_t buflen,
>   
>   /**
>    * Initialize netbuf with data buffer on a user given allocated memory area
> - * m->len is initialized with 0.
> + * m->len is initialized with 0. Metadata (struct uknetbuf, priv)
> + * is placed at the end of the given allocation.
>    * @param mem
>    *   Reference to user provided memory region
>    * @param buflen
> diff --git a/lib/uknetdev/netbuf.c b/lib/uknetdev/netbuf.c
> index 5156e808..c5cf662e 100644
> --- a/lib/uknetdev/netbuf.c
> +++ b/lib/uknetdev/netbuf.c
> @@ -37,7 +37,10 @@
>   
>   /* Used to align netbuf's priv and data areas to `long long` data type */
>   #define NETBUF_ADDR_ALIGNMENT (sizeof(long long))
> -#define NETBUF_ADDR_ALIGN_UP(x) ALIGN_UP((x), NETBUF_ADDR_ALIGNMENT)
> +#define NETBUF_ADDR_ALIGN_UP(x)   ALIGN_UP((__uptr) (x), \
> +                                          NETBUF_ADDR_ALIGNMENT)
> +#define NETBUF_ADDR_ALIGN_DOWN(x) ALIGN_DOWN((__uptr) (x), \
> +                                            NETBUF_ADDR_ALIGNMENT)
>   
>   void uk_netbuf_init_indir(struct uk_netbuf *m,
>                           void *buf, size_t buflen, uint16_t headroom,
> @@ -99,52 +102,34 @@ struct uk_netbuf *uk_netbuf_alloc_buf(struct uk_alloc *a, 
> size_t buflen,
>                                       uint16_t headroom,
>                                       size_t privlen, uk_netbuf_dtor_t dtor)
>   {
> +       void *mem;
> +       size_t alloc_len;
>         struct uk_netbuf *m;
> -       size_t buf_offset = 0;
> -       size_t priv_offset = 0;
> -       size_t headroom_extra = 0;
>   
>   	UK_ASSERT(buflen > 0);
>         UK_ASSERT(headroom <= buflen);
>   
> -	m = uk_malloc(a, NETBUF_ADDR_ALIGN_UP(sizeof(*m))
> -                     + NETBUF_ADDR_ALIGN_UP(privlen)
> -                     + buflen);
> -       if (!m)
> +       alloc_len = NETBUF_ADDR_ALIGN_UP(buflen)
> +                   + NETBUF_ADDR_ALIGN_UP(sizeof(*m) + privlen);
> +       mem = uk_malloc(a, alloc_len);
> +       if (!mem)
>                 return NULL;
>   
> -	/* Place buf right behind `m` or `m->priv` region if privlen > 0.
> -        * In order to keep `m->data - headroom` aligned the padding bytes
> -        *  are added to the headroom.
> -        * We can only do this if the given headroom stays within
> -        *  uint16_t bounds after the operation.
> -        */
> -       if (likely((size_t)(UINT16_MAX - headroom) > NETBUF_ADDR_ALIGNMENT)) {
> -               if (privlen == 0) {
> -                       priv_offset    = 0;
> -                       buf_offset     = sizeof(*m);
> -                       headroom_extra = NETBUF_ADDR_ALIGN_UP(sizeof(*m))
> -                                        - sizeof(*m);
> -               } else {
> -                       priv_offset    = NETBUF_ADDR_ALIGN_UP(sizeof(*m));
> -                       buf_offset     = priv_offset + privlen;
> -                       headroom_extra = NETBUF_ADDR_ALIGN_UP(privlen)
> -                                        - privlen;
> -               }
> +       m = uk_netbuf_prepare_buf(mem,
> +                                 alloc_len,
> +                                 headroom,
> +                                 privlen,
> +                                 dtor);
> +       if (!m) {
> +               uk_free(a, mem);
> +               return NULL;
>         }
>   
> -	uk_netbuf_init_indir(m,
> -                            (void *) m + buf_offset,
> -                            buflen + headroom_extra,
> -                            headroom + headroom_extra,
> -                            privlen > 0 ? ((void *) m + priv_offset) : NULL,
> -                            dtor);
> -
>         /* Save reference to allocator and allocation
>          * that is used for free'ing this uk_netbuf.
>          */
>         m->_a = a;
> -       m->_b = m;
> +       m->_b = mem;
>   
>   	return m;
>   }
> @@ -154,41 +139,29 @@ struct uk_netbuf *uk_netbuf_prepare_buf(void *mem, size_t 
> size,
>                                         size_t privlen, uk_netbuf_dtor_t dtor)
>   {
>         struct uk_netbuf *m;
> -       size_t buf_offset = 0;
> -       size_t priv_offset = 0;
> +       size_t meta_len = 0;
>   
>   	UK_ASSERT(mem);
> -       if ((NETBUF_ADDR_ALIGN_UP(sizeof(*m))
> -            + NETBUF_ADDR_ALIGN_UP(privlen)
> -            + headroom) > size)
> -               return NULL;
>   
> -	/* Place buf right behind `m` or `m->priv` region if privlen > 0.
> -        * In order to keep `m->data - headroom` aligned the padding bytes
> -        *  are added to the headroom.
> -        * We can only do this if the given headroom stays within
> -        *  uint16_t bounds after the operation.
> +       /* Place headroom and buf at the beginning of the allocation,
> +        * This is done in order to forward potential alignments of the
> +        * underlying allocation directly to the netbuf data area.
> +        * `m` (followed by `m->priv` if privlen > 0) will be placed at
> +        * the end of the given memory.
>          */
> -       if (likely((size_t)(UINT16_MAX - headroom) > NETBUF_ADDR_ALIGNMENT)) {
> -               if (privlen == 0) {
> -                       priv_offset = 0;
> -                       buf_offset  = sizeof(*m);
> -                       headroom   += NETBUF_ADDR_ALIGN_UP(sizeof(*m))
> -                                     - sizeof(*m);
> -               } else {
> -                       priv_offset = NETBUF_ADDR_ALIGN_UP(sizeof(*m));
> -                       buf_offset  = priv_offset + privlen;
> -                       headroom   += NETBUF_ADDR_ALIGN_UP(privlen)
> -                                     - privlen;
> -               }
> -       }
> +       meta_len = NETBUF_ADDR_ALIGN_UP(sizeof(*m) + privlen);
> +       if (meta_len > NETBUF_ADDR_ALIGN_DOWN((__uptr) mem + size))
> +               return NULL;
> +
> +       m = (struct uk_netbuf *) (NETBUF_ADDR_ALIGN_DOWN((__uptr) mem + size)
> +                                 - meta_len);
>   
> -	m = (struct uk_netbuf *) mem;
>         uk_netbuf_init_indir(m,
> -                            mem + buf_offset,
> -                            size - buf_offset,
> +                            mem,
> +                            (size_t) ((__uptr) m - (__uptr) mem),
>                              headroom,
> -                            privlen > 0 ? (mem + priv_offset) : NULL,
> +                            privlen > 0 ? (void *) ((__uptr) m+ sizeof(*m))
> +                                        : NULL,
>                              dtor);
>         return m;
>   }