Re: [UNIKRAFT/LIBLWIP PATCH v2 1/3] lib/lwip: Reserve per netdev space for lwip

[Hugo Lefeuvre <hugo.lefeuvre@xxxxxxxxx>]

Hi Sharan,

thanks a lot for this patch. Having a little bit more documentation
would be nice since this is a non-trivial feature. Comments inline.

regards,
Hugo

On Tue, 2020-06-30 at 13:15 +0200, Sharan Santhanam wrote:
> Signed-off-by: Sharan Santhanam <sharan.santhanam@xxxxxxxxx>

I think that a commit message is important here; the purpose of the
scratch pad is not evident. e.g.,

Reserve per-network-device (netdev) space for lwip. This will be later
used to store netdev specific information such as the support of
interrupts. Relying on uknetdev's scratch pad allows us to store this
information contiguously for all devices in the same portion of memory,
thus reducing potential cache and tlb pressure.

> ---
>  Config.uk   | 7 +++++++
>  Makefile.uk | 1 +
>  2 files changed, 8 insertions(+)
> 
> diff --git a/Config.uk b/Config.uk
> index aaaaae0..d9bd2c8 100644
> --- a/Config.uk
> +++ b/Config.uk
> @@ -22,6 +22,13 @@ config LWIP_UKNETDEV
>               In case threaded mode is selected and the underlying
> device
>               driver does not support receive interrupts the
> network
>               interfaces have to be polled manually
> (uknetdev_poll()).
> +
> +config LWIP_UKNETDEV_SCRATCH
> +     int
> +     default 24
> +     help
> +             The network stack reserves space in the uknetdev
> device for its
> +             use.
>  endmenu
>  

While this option is not present in the menuconfig (invisible), users
can still change it manually in their configuration (or rely on
outdated configuration data!). The help should definitely mention
that LWIP_UKNETDEV_SCRATCH needs to remain big enough to store lwip's
internal data, and that reducing this without further adaptations to
the glue code will result in undefined behavior and potentially non-
functional unikernels.

>  config LWIP_AUTOIFACE
> diff --git a/Makefile.uk b/Makefile.uk
> index d3c6c9c..99005a9 100644
> --- a/Makefile.uk
> +++ b/Makefile.uk
> @@ -62,6 +62,7 @@ LIBLWIP_COMMON_INCLUDES-y +=
> -I$(LIBLWIP_BASE)/musl-imported/include
>  LIBLWIP_COMMON_INCLUDES-y += -I$(LIBLWIP_EXTRACTED)/include
>  CINCLUDES-$(CONFIG_LIBLWIP)   += $(LIBLWIP_COMMON_INCLUDES-y)
>  CXXINCLUDES-$(CONFIG_LIBLWIP) += $(LIBLWIP_COMMON_INCLUDES-y)
> +$(eval $(call uknetdev_scratch_mem,$(CONFIG_LWIP_UKNETDEV_SCRATCH)))
>  
>  ####################################################################
> ############
>  # Library flags