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

[Florian Schmidt <Florian.Schmidt@xxxxxxxxx>]

Hi Sharan,

On 4/15/19 3:31 PM, Sharan Santhanam wrote:
> 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.
When I thought about this, I had linux kernel module command line 
parameters in mind. Those work a little bit the same way: in the general 
case, you area expected to set sane default parameters (unless there 
really isn't any sane choice), and then potentially modify them by 
optional command line parameters.
This approach would be similar: every variable should have a sane 
default value; only in cases where this really isn't possible, but then, 
you'd probably need to hard-SELECT LIBUKLIBPARAM, and add error handling 
to boot of the option is not provided.
Cheers,
Florian

> > 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)
> > >   {
--
Dr. Florian Schmidt
フローリアン・シュミット
Research Scientist,
Systems and Machine Learning Group
NEC Laboratories Europe
Kurfürsten-Anlage 36, D-69115 Heidelberg
Tel.     +49 (0)6221 4342-265
Fax:     +49 (0)6221 4342-155
e-mail:  florian.schmidt@xxxxxxxxx
============================================================
Registered at Amtsgericht Mannheim, Germany, HRB728558

_______________________________________________
Minios-devel mailing list
Minios-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/minios-devel