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

[Simon Kuenzer <simon.kuenzer@xxxxxxxxx>]

Hi Hugo,

this patch looks good. Could you rename uk_alloc_head to _uk_alloc_head? 
This is to show that this symbol is still internally used by the library 
although it is exported because of performance reasons.
Thanks,

Simon

On 08.04.20 20:24, 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.
>
> 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            | 39 +--------------------------------------
>   lib/ukalloc/exportsyms.uk      |  2 +-
>   lib/ukalloc/include/uk/alloc.h | 10 +++++++---
>   3 files changed, 9 insertions(+), 42 deletions(-)
>
> diff --git a/lib/ukalloc/alloc.c b/lib/ukalloc/alloc.c
> index 214fbd9..11f58dc 100644
> --- a/lib/ukalloc/alloc.c
> +++ b/lib/ukalloc/alloc.c
> @@ -57,7 +57,7 @@
>         (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)
>   {
> @@ -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 594999f..903565d 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
> @@ -14,3 +13,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 73b3a45..3d1f44f 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)
> @@ -108,6 +105,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)
>   {