Re: [UNIKRAFT PATCH v2] lib/ukalloc: make uk_alloc_get_default static inline

[Simon Kuenzer <simon.kuenzer@xxxxxxxxx>]

Thanks a lot for the changes!

Reviewed-by: Simon Kuenzer <simon.kuenzer@xxxxxxxxx>

On 24.06.20 13:42, Hugo Lefeuvre wrote:
> Every allocation done through the libc malloc wrapper triggers a function
> call to uk_alloc_get_default in order to get a pointer to the default
> allocator.  This is not great, because the allocation path is performance
> critical, and we should avoid any superflous overhead there.
>
> This patch removes the capability to change the default allocator once it
> has been set. This never really worked anyways... the libc wrapper would
> forward free() calls to the new allocator, potentially resulting in memory
> corruption if the new allocator doesn't handle these foreign pointers
> correctly, and memory leaks because these pointers won't be freed anymore.
>
> We make the uk_alloc_head variable global and uk_alloc_get_default static
> inline. We rename uk_alloc_head to _uk_alloc_head -- it should preferably
> be accessed through uk_alloc_get_default.
>
> Setting aside compiler optimizations, retrieving the default allocator
> becomes less expensive: a single memory read instead of a function call +
> memory read.
>
> It appears that uk_alloc_set_default was not called anywhere, so this patch
> does not require any additional changes in external libraries.
>
> Signed-off-by: Hugo Lefeuvre <hugo.lefeuvre@xxxxxxxxx>
> ---
>   lib/ukalloc/alloc.c            | 45 ++++--------------------------------------
>   lib/ukalloc/exportsyms.uk      |  2 +-
>   lib/ukalloc/include/uk/alloc.h | 10 +++++++---
>   3 files changed, 12 insertions(+), 45 deletions(-)
>
> diff --git a/lib/ukalloc/alloc.c b/lib/ukalloc/alloc.c
> index 437bc4b..127bc6f 100644
> --- a/lib/ukalloc/alloc.c
> +++ b/lib/ukalloc/alloc.c
> @@ -57,14 +57,14 @@
>         (ALIGN_UP((unsigned long)(size), __PAGE_SIZE) / __PAGE_SIZE)
>   #define page_off(x) ((unsigned long)(x) & (__PAGE_SIZE - 1))
>   
> -static struct uk_alloc *uk_alloc_head;
> +struct uk_alloc *_uk_alloc_head;
>   
>   int uk_alloc_register(struct uk_alloc *a)
>   {
> -       struct uk_alloc *this = uk_alloc_head;
> +       struct uk_alloc *this = _uk_alloc_head;
>   
> -	if (!uk_alloc_head) {
> -               uk_alloc_head = a;
> +       if (!_uk_alloc_head) {
> +               _uk_alloc_head = a;
>                 a->next = NULL;
>                 return 0;
>         }
> @@ -76,43 +76,6 @@ int uk_alloc_register(struct uk_alloc *a)
>         return 0;
>   }
>   
> -struct uk_alloc *uk_alloc_get_default(void)
> -{
> -       return uk_alloc_head;
> -}
> -
> -int uk_alloc_set_default(struct uk_alloc *a)
> -{
> -       struct uk_alloc *head, *this, *prev;
> -
> -       head = uk_alloc_get_default();
> -
> -       if (a == head)
> -               return 0;
> -
> -       if (!head) {
> -               uk_alloc_head = a;
> -               return 0;
> -       }
> -
> -       this = head;
> -       while (this->next) {
> -               prev = this;
> -               this = this->next;
> -               if (a == this) {
> -                       prev->next = this->next;
> -                       this->next = head->next;
> -                       head = this;
> -                       return 0;
> -               }
> -       }
> -
> -       /* a is not registered yet. Add in front of the queue. */
> -       a->next = head;
> -       uk_alloc_head = a;
> -       return 0;
> -}
> -
>   struct metadata_ifpages {
>         unsigned long   num_pages;
>         void            *base;
> diff --git a/lib/ukalloc/exportsyms.uk b/lib/ukalloc/exportsyms.uk
> index 2357501..2c8a90f 100644
> --- a/lib/ukalloc/exportsyms.uk
> +++ b/lib/ukalloc/exportsyms.uk
> @@ -1,6 +1,5 @@
>   uk_alloc_register
>   uk_alloc_get_default
> -uk_alloc_set_default
>   uk_malloc_ifpages
>   uk_free_ifpages
>   uk_realloc_ifpages
> @@ -10,3 +9,4 @@ uk_memalign_compat
>   uk_realloc_compat
>   uk_palloc_compat
>   uk_pfree_compat
> +_uk_alloc_head
> diff --git a/lib/ukalloc/include/uk/alloc.h b/lib/ukalloc/include/uk/alloc.h
> index 8ab41d6..5bfa2f2 100644
> --- a/lib/ukalloc/include/uk/alloc.h
> +++ b/lib/ukalloc/include/uk/alloc.h
> @@ -53,9 +53,6 @@ extern "C" {
>   #define uk_zalloc(a, size)  uk_calloc(a, 1, size)
>   #define uk_do_zalloc(a, size) uk_do_calloc(a, 1, size)
>   
> -struct uk_alloc *uk_alloc_get_default(void);
> -int uk_alloc_set_default(struct uk_alloc *a);
> -
>   typedef void* (*uk_alloc_malloc_func_t)
>                 (struct uk_alloc *a, size_t size);
>   typedef void* (*uk_alloc_calloc_func_t)
> @@ -103,6 +100,13 @@ struct uk_alloc {
>         int8_t priv[];
>   };
>   
> +extern struct uk_alloc *_uk_alloc_head;
> +
> +static inline struct uk_alloc *uk_alloc_get_default(void)
> +{
> +       return _uk_alloc_head;
> +}
> +
>   /* wrapper functions */
>   static inline void *uk_do_malloc(struct uk_alloc *a, size_t size)
>   {