Re: [UNIKRAFT PATCH v2 1/2] lib/uknetdev: Save nw_stack data in a netdevice

[Simon Kuenzer <simon.kuenzer@xxxxxxxxx>]

Hey Sharan,

On 30.06.20, 12:47, "Minios-devel on behalf of Sharan Santhanam" 
<minios-devel-bounces@xxxxxxxxxxxxxxxxxxxx on behalf of 
Sharan.Santhanam@xxxxxxxxx> wrote:

    We extend the uk_netdev to store network stack specific data into
    uk_netdev. The size of the of this data is passed to network stack
    by setting the CONFIG_UK_NETDEV_SCRATCH_SIZE
    
    Signed-off-by: Sharan Santhanam <sharan.santhanam@xxxxxxxxx>
    ---
     lib/uknetdev/Makefile.rules           | 20 ++++++++++++++++++++
     lib/uknetdev/include/uk/netdev_core.h |  7 +++++++
     support/build/Makefile.rules          |  2 +-
     3 files changed, 28 insertions(+), 1 deletion(-)
     create mode 100644 lib/uknetdev/Makefile.rules
    
    diff --git a/lib/uknetdev/Makefile.rules b/lib/uknetdev/Makefile.rules
    new file mode 100644
    index 00000000..7894e163
    --- /dev/null
    +++ b/lib/uknetdev/Makefile.rules
    @@ -0,0 +1,20 @@
    +UK_SCRATCH_MEM=0
    +UK_SCRATCH_RULE_SET=
    +
    +.SECONDEXPANSION:
    +FORCE:
    +
    +uk_scratch_mem_set: $$(eval 
CFLAGS-y+=-DCONFIG_UK_NETDEV_SCRATCH_SIZE=$$(UK_SCRATCH_MEM))
    +   @:;
    +
    +.PHONY : uk_scratch_mem_set
    +
    +
    +## reserve memory for the network stack
    +## uknetdev_scratch_mem,bytes_of_memory
    +define uknetdev_scratch_mem =
    +$(if $(strip $(1)),\
    +   $(if $(shell test $(1) -gt $(UK_SCRATCH_MEM) && echo "scratch mem 
set"), $(eval UK_SCRATCH_MEM:=$(1))))
    +
    +$(if $(strip $(UK_SCRATCH_RULE_SET)),,$(eval UK_SCRATCH_RULE_SET:=1) 
$(eval UK_PREPARE-$(CONFIG_LIBUKNETDEV)+=uk_scratch_mem_set))
    +endef
    diff --git a/lib/uknetdev/include/uk/netdev_core.h 
b/lib/uknetdev/include/uk/netdev_core.h
    index dba719fc..827f1872 100644
    --- a/lib/uknetdev/include/uk/netdev_core.h
    +++ b/lib/uknetdev/include/uk/netdev_core.h
    @@ -65,6 +65,10 @@
     extern "C" {
     #endif
     
    +#ifndef CONFIG_UK_NETDEV_SCRATCH_SIZE
    +#define CONFIG_UK_NETDEV_SCRATCH_SIZE 0
    +#endif /* CONFIG_UK_NETDEV_SCRATCH_SIZE */
    +
     struct uk_netdev;
     UK_TAILQ_HEAD(uk_netdev_list, struct uk_netdev);
     
    @@ -400,6 +404,9 @@ struct uk_netdev {
        struct uk_netdev_tx_queue   *_tx_queue[CONFIG_LIBUKNETDEV_MAXNBQUEUES];
     
        UK_TAILQ_ENTRY(struct uk_netdev) _list;
    +#if (CONFIG_UK_NETDEV_SCRATCH_SIZE > 0)
    +   char scratch_pad[CONFIG_UK_NETDEV_SCRATCH_SIZE];
    +#endif /* CONFIG_UK_NETDEV_SCRATCH_SIZE */
     };
     
     #ifdef __cplusplus
    diff --git a/support/build/Makefile.rules b/support/build/Makefile.rules
    index 8c096fa5..5f7db8a3 100644
    --- a/support/build/Makefile.rules
    +++ b/support/build/Makefile.rules
    @@ -450,7 +450,7 @@ $(4): $(2) | prepare
                       $($(call vprefix_lib,$(1),CINCLUDES)) $($(call 
vprefix_lib,$(1),CINCLUDES-y)) \
                       $($(call vprefix_src,$(1),$(2),$(3),INCLUDES)) $($(call 
vprefix_src,$(1),$(2),$(3),INCLUDES-y)) \
                       $($(call vprefix_glb,$(3),ARCHFLAGS)) $($(call 
vprefix_glb,$(3),ARCHFLAGS-y)) \
    -                  $(CFLAGS) $(CFLAGS-y) \

Could you add at least a comment about why the `$$`? My concern is that this 
rule is not inline to the others that we have and it is not obvious why. In 
general this looks to me as another reason why I would prefer generating a 
simple header file that gets the value set (as discussed offline). 
Alternatively, would it make sense to change all the build flags in our rules 
to `$$`? What would be the implication?


    +                  $(CFLAGS) $$(CFLAGS-y) \
                       $($(call vprefix_lib,$(1),CFLAGS)) $($(call 
vprefix_lib,$(1),CFLAGS-y)) \
                       $($(call vprefix_src,$(1),$(2),$(3),FLAGS)) $($(call 
vprefix_src,$(1),$(2),$(3),FLAGS-y)) \
                       $(5) \
    -- 
    2.20.1
    
    
Thanks,

Simon