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

[Hugo Lefeuvre <hugo.lefeuvre@xxxxxxxxx>]

Hi Simon,

thanks a lot, comments for 1/2 and 2/2 addressed in the v2.

regards,
Hugo

On Tue, 2020-07-28 at 13:41 +0200, Simon Kuenzer wrote:
> 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");
> >