Re: [Minios-devel] [UNIKRAFT PATCH 5/8] plat/linuxu: Introduce heap size as a lib parameter

[Sharan Santhanam <sharan.santhanam@xxxxxxxxx>]

On 4/10/19 4:09 PM, Florian Schmidt wrote:
> Hi Sharan,
>
> the idea of the patch looks fine, but I'm not a big fan of strewing 
> #ifdef CONFIG_* all over the code. What if, instead of checking the 
> config parameter everywhere, there were one big #ifdef 
> CONFIG_LIBUKLIBPARAM in libparam.h, which defines everything as it is 
> now if it is defined, and in the #else case defines the public-facing 
> macros such as UK_LIB_PARAM() as empty macros? That way, you don't need 
> to #ifdef-guard all those calls inside the code.
Since it was a base platform library, I did not want to include extra 
libraries. But for other internal libraries I would expect the Config.uk 
to contain "select LIBUKLIBPARAM". So you do not have to use the ifdef.
> Of course, that means the build system needs to provide the include path 
> to libparam.h even if the library itself isn't compiled in, but this 
> could be just done by setting the include directive in libparam's 
> Makefile.uk to be CINCLUDES-y instead of 
> CINCLUDES-$(CONFIG_LIBUKLIBPARAM), but maybe this unconditional addition 
> to the include directories is worth less #ifdef pollution in each source 
> file.
My only concern with this solution is, if the developer misconfigures 
the error would be caught while running the program as we are silently 
ignoring the parameter. In the current implementation it would cause 
compiler error and cannot be ignored.
> Cheers,
> Florian
>
> On 3/15/19 6:06 PM, Sharan Santhanam wrote:
> > In linuxu platform, the user can overwrite the default heap size
> > parameter. In this patch we introduce the heap size argument as an
> > UK_LIB_PARAM which the user can modify at boot time.
> >
> > Signed-off-by: Sharan Santhanam <sharan.santhanam@xxxxxxxxx>
> > ---
> >   plat/linuxu/Makefile.uk |  2 ++
> >   plat/linuxu/memory.c    | 17 ++++++++++++++++-
> >   2 files changed, 18 insertions(+), 1 deletion(-)
> >
> > diff --git a/plat/linuxu/Makefile.uk b/plat/linuxu/Makefile.uk
> > index 2c0de76..281b5c8 100644
> > --- a/plat/linuxu/Makefile.uk
> > +++ b/plat/linuxu/Makefile.uk
> > @@ -8,6 +8,8 @@ $(eval $(call addplat_s,linuxu,$(CONFIG_PLAT_LINUXU)))
> >   ##
> >   $(eval $(call addplatlib,linuxu,liblinuxuplat))
> > +## Adding libparam for the linuxu platform
> > +$(eval $(call addplatlib_paramprefix,linuxu,liblinuxuplat,linuxu))
> >   ##
> >   ## Platform library definitions
> >   ##
> > diff --git a/plat/linuxu/memory.c b/plat/linuxu/memory.c
> > index 35d0d95..6b98acb 100644
> > --- a/plat/linuxu/memory.c
> > +++ b/plat/linuxu/memory.c
> > @@ -33,9 +33,24 @@
> >    * THIS HEADER MAY NOT BE EXTRACTED OR MODIFIED IN ANY WAY.
> >    */
> > +#include <errno.h>
> > +#include <uk/arch/types.h>
> >   #include <linuxu/setup.h>
> > -#include <uk/plat/memory.h>
> > +#include <uk/errptr.h>
> >   #include <uk/assert.h>
> > +#include <linuxu/syscall.h>
> > +#include <uk/plat/memory.h>
> > +#ifdef CONFIG_LIBUKLIBPARAM
> > +#include <uk/libparam.h>
> > +#endif /* CONFIG_LIBUKLIBPARAM */
> > +
> > +#define MB2B        (1024 * 1024)
> > +
> > +static __u32 heap_size = CONFIG_LINUXU_DEFAULT_HEAPMB;
> > +#ifdef CONFIG_LIBUKLIBPARAM
> > +UK_LIB_PARAM(heap_size, __u32);
> > +#endif /* CONFIG_LIBUKLIBPARAM */
> > +
> >   int ukplat_memregion_count(void)
> >   {
_______________________________________________
Minios-devel mailing list
Minios-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/minios-devel