Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

gnrc_netif_t: A dynamic approach to ~~GNRC_NETIF_NUMOF~~. #9903

Closed
miri64 opened this issue Sep 7, 2018 · 14 comments
Closed

gnrc_netif_t: A dynamic approach to ~~GNRC_NETIF_NUMOF~~. #9903

miri64 opened this issue Sep 7, 2018 · 14 comments
Assignees
Labels
Area: network Area: Networking Process: API change Integration Process: PR contains or issue proposes an API change. Should be handled with care. Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation

Comments

@miri64
Copy link
Member

miri64 commented Sep 7, 2018

While there are compile-time benefits to cap the number of interfaces, recent questions by users and contributors opened the question if a statically defined GNRC_NETIF_NUMOF (and thus a predefined array) isn't to intransparent and really the most sensible approach. IMHO it could actually be pretty easy with minor API changes to go to a linked-list-based approach by just adding a next member above ops.

typedef struct {
const gnrc_netif_ops_t *ops; /**< Operations of the network interface */
netdev_t *dev; /**< Network device of the network interface */

would become

typedef struct gnrc_netif gnrc_netif_t;
/* […] */
struct gnrc_netif {
    gnrc_netif_t *next;                     /**< The next interface */
    const gnrc_netif_ops_t *ops;            /**< Operations of the network interface */
    netdev_t *dev;                          /**< Network device of the network interface */

For allocation it could just be allocated on the stack of the network interface's thread.

Of course, gnrc_netif_iter() could be simplified a lot then as well ;-).

The biggest disadvantage I see is that we loose some #ifdef GNRC_NETIF_NUMOF == 1 optimizations through-out the code (which also would require changing, which is why this rework could become quite major though the change to the API is as I said pretty minor).

Let me know what you think.

(@gschorcht this might also be of interest to you as well)

@miri64 miri64 added Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation Area: network Area: Networking Process: API change Integration Process: PR contains or issue proposes an API change. Should be handled with care. GNRC labels Sep 7, 2018
@kaspar030
Copy link
Contributor

How about changing all #ifdef GNRC_NETIF_NUMOF == 1 to ifdef GNRC_SINGLE_NETDEV?
It cannot be set automatically from the Makefiles anymore, but would still be available as optimization (for a not-so edge case...).

Combined with a runtime assert on adding the second netif if GNRC_SINGLE_NETDEV is defined, this shouldn't cause too much harm, but would enabling the use of a linked list for netdevs.

@cladmi
Copy link
Contributor

cladmi commented Sep 7, 2018

What are the use case for GNRC_NETIF_NUMOF > 1 ? Regular boards with one radio have GNRC_NETIF_NUMOF == 1 right ?

@miri64
Copy link
Member Author

miri64 commented Sep 7, 2018

How about changing all #ifdef GNRC_NETIF_NUMOF == 1 to ifdef GNRC_SINGLE_NETDEV?
It cannot be set automatically from the Makefiles anymore, but would still be available as optimization (for a not-so edge case...).

Combined with a runtime assert on adding the second netif if GNRC_SINGLE_NETDEV is defined, this shouldn't cause too much harm, but would enabling the use of a linked list for netdevs.

I had this idea also. It would keep the optimizations but make it optional.

What are the use case for GNRC_NETIF_NUMOF > 1 ? Regular boards with one radio have GNRC_NETIF_NUMOF == 1 right ?

Border router e.g. or like @gschorcht mentioned: boards with more than one network device.

The question is whether it a wished behavior that all network interfaces
of a board are always activated automatically or whether should the
application developer have the control.

I would say yes. If an application developer does not want all devices, they can remove (gnrc_)netdev_default and use the specific devices they want to use instead.

@gschorcht
Copy link
Contributor

I have a ESP32 board that has

  • Ethernet (ESP32 Ethernet MAC)
  • WiFi (ESP32 ESP-NOW)
  • MRF24J40

I have tried the following approach. Board's Makefile.dep could be
extended to determine GNRC_NETIF_NUMOF as following:

ifneq (,$(filter netdev_default gnrc_netdev_default,$(USEMODULE)))
    # avoid multiple definitions when package depenedencies are resolved recursively
    ifndef MODULE_ESP_ETH_ADDED
        MODULE_ESP_ETH_ADDED = 1
        USEMODULE += esp_eth
        $(eval GNRC_NETIF_NUMOF=$(shell echo $$(($(GNRC_NETIF_NUMOF)+1))))
    endif
endif

This approach requires hat the following lines in $(RIOTBASE)/Makefile.include are reordered but this shouldn't have any side effect:

# get number of interfaces straight before resolving ...
GNRC_NETIF_NUMOF ?= 1
ifneq ($(GNRC_NETIF_NUMOF),1)
  CFLAGS += -DGNRC_NETIF_NUMOF=$(GNRC_NETIF_NUMOF)
endif

# process dependencies
include $(RIOTBASE)/Makefile.dep

The question is whether it a wished behavior that all network interfaces of a board are always activated automatically or whether should the application developer have the control.

What I have seen is that most of the boards have only one network interface and they already activate it automatically in their Makefile.dep when module netdev_default is used.

So why not to activate all network interface automatically?

@miri64
Copy link
Member Author

miri64 commented Sep 7, 2018

The question is whether it a wished behavior that all network interfaces of a board are always activated automatically or whether should the application developer have the control.

See my answer to your deleted comment in #9903 (comment)

@miri64
Copy link
Member Author

miri64 commented Sep 7, 2018

How about changing all #ifdef GNRC_NETIF_NUMOF == 1 to ifdef GNRC_SINGLE_NETDEV?
It cannot be set automatically from the Makefiles anymore, but would still be available as optimization (for a not-so edge case...).

Combined with a runtime assert on adding the second netif if GNRC_SINGLE_NETDEV is defined, this shouldn't cause too much harm, but would enabling the use of a linked list for netdevs.

I had this idea also. It would keep the optimizations but make it optional.

This will be hell to test btw.

@miri64
Copy link
Member Author

miri64 commented Sep 7, 2018

(It is already, but by making it optional a test for this should be introduced)

@gschorcht
Copy link
Contributor

@miri64 When I was fixing the esp_wifi netdev driver for ESP32 PR #10762, I was wondering again whether something like the following construct in the makefile is an acceptable solution to determine automatically the number of network interface during compile time.

ifneq (,$(filter esp_now,$(USEMODULE)))
    $(eval GNRC_NETIF_NUMOF=$(shell echo $$(($(GNRC_NETIF_NUMOF)+1))))
    USEMODULE += esp_wifi_any
endif

ifneq (,$(filter esp_wifi,$(USEMODULE)))
    $(eval GNRC_NETIF_NUMOF=$(shell echo $$(($(GNRC_NETIF_NUMOF)+1))))
    USEMODULE += esp_wifi_any
endif

That is, with each enabled netdev driver module, the number of netifs is increased by one. It could be done in same way in driver makefiles or in board definitions.

The problem is that this definition of GNRC_NETIF_NUMOF collide with GNRC_NETIF_NUMOF in example makefiles, e.g. gnrc_boarder_router, why we would have to decide finally where to define it.

BTW, in case of ESP32, I had to add this to cpu/esp32/Makefile.include instead of cpu/esp32/Makefile.dep because of wrong order here.

RIOT/Makefile.include

Lines 282 to 293 in 6a4c764

# get number of interfaces straight before resolving dependencies
GNRC_NETIF_NUMOF ?= 1
ifneq ($(GNRC_NETIF_NUMOF),1)
CFLAGS += -DGNRC_NETIF_NUMOF=$(GNRC_NETIF_NUMOF)
endif
# handle removal of default modules
USEMODULE += $(filter-out $(DISABLE_MODULE), $(DEFAULT_MODULE))
# process dependencies
include $(RIOTBASE)/Makefile.dep

@miri64
Copy link
Member Author

miri64 commented Jan 22, 2019

This seems to be more of a build system specific workaround until we get rid of GNRC_NETIF_NUMOF with the proposal here. @cladmi what do you think about that?

@cladmi
Copy link
Contributor

cladmi commented Jun 3, 2019

Found back about this one. A solution to remove the need to have $(eval GNRC_NETIF_NUMOF=$(shell echo $$(($(GNRC_NETIF_NUMOF)+1)))) would be do have some GNRC_NETIF_INTERFACES += esp_wifi GNRC_NETIF_INTERFACES += esp_now I think in the Makefile.include files and then do GNRC_NETIF_NUMOF = $(words $(GNRC_NETIF_INTERFACES)).

@benpicco
Copy link
Contributor

benpicco commented Aug 8, 2019

You could still have a board that has e.g. two MRF24J40 modules, so you would get two interfaces. Just counting the number of netif modules would not help here.

However, RIOT already iterates over all interfaces for each module in sys/auto_init/netif.
So we would just have to pull out the $module_NUM defines into a common header:

#ifdef MODULE_MRF24J40
#include "mrf24j40_params.h"
#define MRF24J40_NUM ARRAY_SIZE(mrf24j40_params)
#else 
#define MRF24J40_NUM (0)
#endif#define GNRC_NETIF_NUMOF (MRF24J40_NUM + …)

@jia200x
Copy link
Member

jia200x commented Nov 11, 2019

+1 for the linked list approach of interfaces!

How about changing all #ifdef GNRC_NETIF_NUMOF == 1 to ifdef GNRC_SINGLE_NETDEV?
It cannot be set automatically from the Makefiles anymore, but would still be available as optimization (for a not-so edge case...).
Combined with a runtime assert on adding the second netif if GNRC_SINGLE_NETDEV is defined, this shouldn't cause too much harm, but would enabling the use of a linked list for netdevs.

This would certainly do the trick.

A short question: is this intended to be only for GNRC_NETIF_NUMOF, or is it also for other macros (e.g LWIP_NETIF_NUMOF)?

Also note that since #11879 got merged, netif_t structures are linked lists. So it would be consistent.

@miri64
Copy link
Member Author

miri64 commented Nov 12, 2019

I am not aware that lwIP optimizes for number of interfaces. In fact lwIP uses already linked-lists. I don't think that this can or should be generalized for all network stacks. There are network stacks (emb6 or uIP e.g.) that are specifically designed for single-interface use.

@miri64
Copy link
Member Author

miri64 commented Mar 26, 2020

#12994 was merged, so the approach proposed here is now implemented.

@miri64 miri64 closed this as completed Mar 26, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: network Area: Networking Process: API change Integration Process: PR contains or issue proposes an API change. Should be handled with care. Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation
Projects
None yet
Development

No branches or pull requests

10 participants