Re: [UNIKRAFT PATCH 2/2] lib/ukboot: initialize ukallocregion

[Simon Kuenzer <simon.kuenzer@xxxxxxxxx>]

Besides few minor comments, this patch looks very good. Thanks a lot!

On 22.06.20 16:23, Hugo Lefeuvre wrote:
> Add menuconfig bindings to select a system-wide allocator.
> Initialize the selected allocator in ukboot.
>
> Signed-off-by: Hugo Lefeuvre <hugo.lefeuvre@xxxxxxxxx>
> ---
>   lib/ukboot/Config.uk | 26 ++++++++++++++++++++++----
>   lib/ukboot/boot.c    | 19 +++++++++++++------
>   2 files changed, 35 insertions(+), 10 deletions(-)
>
> diff --git a/lib/ukboot/Config.uk b/lib/ukboot/Config.uk
> index 841a876..e4b356b 100644
> --- a/lib/ukboot/Config.uk
> +++ b/lib/ukboot/Config.uk
> @@ -17,8 +17,26 @@ if LIBUKBOOT
>         int "Maximum number of arguments (max. size of argv)"
>         default 60
>   
> -	config LIBUKBOOT_INITALLOC
> -       bool "Initialize ukallocbbuddy as allocator"
> -       default y
> -       select LIBUKALLOCBBUDDY
> +       choice LIBUKBOOT_INITALLOC
> +       prompt "Default memory allocator"
How about calling this "Initialize memory allocator" instead. I think 
this would make it a bit more obvious to a user what this option is 
doing behind the scenes.

> +       default LIBUKBOOT_INITBBUDDY
> +
> +               config LIBUKBOOT_INITBBUDDY
> +               bool "Binary buddy allocator"
> +               default y
Yes, remove these options 'defaul y/n', as you pointed out.

> +               select LIBUKALLOCBBUDDY
> +
> +               config LIBUKBOOT_INITREGION
> +               bool "Region allocator"
> +               default n
> +               select LIBUKALLOCREGION
> +               help
> +                 Satisfy allocation as fast as possible. No support for free().
> +                 Refer to help in ukallocregion for more information.
> +
> +               config LIBUKBOOT_NOALLOC
> +               bool "No memory allocator"
If you change the prompt of the choice as I mentioned, you can call this 
bool "None" instead of "No memory allocator". This makes it a bit shorter.
> +               default n
> +
> +       endchoice
>   endif
> diff --git a/lib/ukboot/boot.c b/lib/ukboot/boot.c
> index e8a2ac7..4e749aa 100644
> --- a/lib/ukboot/boot.c
> +++ b/lib/ukboot/boot.c
> @@ -41,8 +41,10 @@
>   #include <stdio.h>
>   #include <errno.h>
>   
> -#if CONFIG_LIBUKALLOC && CONFIG_LIBUKALLOCBBUDDY && CONFIG_LIBUKBOOT_INITALLOC
> +#if CONFIG_LIBUKBOOT_INITBBUDDY
>   #include <uk/allocbbuddy.h>
> +#elif CONFIG_LIBUKBOOT_INITREGION
> +#include <uk/allocregion.h>
>   #endif
>   #if CONFIG_LIBUKSCHED
>   #include <uk/sched.h>
> @@ -178,7 +180,7 @@ void ukplat_entry(int argc, char *argv[])
>   #if CONFIG_LIBUKALLOC
>         struct uk_alloc *a = NULL;
>   #endif
> -#if CONFIG_LIBUKALLOC && CONFIG_LIBUKALLOCBBUDDY && CONFIG_LIBUKBOOT_INITALLOC
> +#if !CONFIG_LIBUKBOOT_NOALLOC
>         struct ukplat_memregion_desc md;
>   #endif
>   #if CONFIG_LIBUKSCHED
> @@ -205,9 +207,9 @@ void ukplat_entry(int argc, char *argv[])
>         }
>   #endif /* CONFIG_LIBUKLIBPARAM */
>   
> -#if CONFIG_LIBUKALLOC && CONFIG_LIBUKALLOCBBUDDY && CONFIG_LIBUKBOOT_INITALLOC
> +#if !CONFIG_LIBUKBOOT_NOALLOC
>         /* initialize memory allocator
> -        * FIXME: ukallocbbuddy is hard-coded for now
> +        * FIXME: allocators are hard-coded for now
>          */
>         uk_pr_info("Initialize memory allocator...\n");
>         ukplat_memregion_foreach(&md, UKPLAT_MEMRF_ALLOCATABLE) {
> @@ -226,10 +228,15 @@ void ukplat_entry(int argc, char *argv[])
>                  * As soon we have an allocator, we simply add every
>                  * subsequent region to it
>                  */
> -               if (unlikely(!a))
> +               if (!a) {
> +#if CONFIG_LIBUKBOOT_INITBBUDDY
>                         a = uk_allocbbuddy_init(md.base, md.len);
> -               else
> +#elif CONFIG_LIBUKBOOT_INITREGION
> +                       a = uk_allocregion_init(md.base, md.len);
> +#endif
> +               } else {
>                         uk_alloc_addmem(a, md.base, md.len);
> +               }
>         }
>         if (unlikely(!a))
>                 uk_pr_warn("No suitable memory region for memory allocator. Continue 
> without heap\n");