gnrc_netif: implementation of dynamic GNRC_NETIF_NUMOF approach by jia200x · Pull Request #12994 · RIOT-OS/RIOT

jia200x · 2019-12-19T18:48:11Z

Contribution description

This PR is an implementation of the ideas proposed in #9903

It removes the internal allocation of gnrc_netif_t objects in gnrc_netif and assumes the gnrc_netif_xxx_create caller allocates the network interface. This means the allocation is decentralized and we move to a linked list approach for gnrc_netif_numof and gnrc_netif_iter (as described in #9903)

Some benefits:

No need to do tricks for incrementing the number of interfaces. Just call gnrc_netif_xxx_create and they will be in the linked list :)
Auto init can define the number of interfaces depending of the number of devices (e.g 2 IEEE802.15.4 interfaces if there are 2 AT86RF2XX radios)
A little bit less of ROM and RAM, since it's safe to assume the pointers are always there.

I added a GNRC_NETIF_SINGLE macro (as proposed in #9903) to keep the optimizations when there's a single interface.

Testing procedure

This should be carefully tested. Probably 95% of testing here is just compile tests, but it would be nice to ensure that the single interface optimization (GNRC_NETIF_SINGLE) still works as the original GNRC_NETIF_NUMOF==1.

Also, ensure that the devices still work.

Issues/PRs references

#9903
Alternative to #12308
Fixes stuff like #11979

sys/include/net/gnrc/netif.h

gschorcht · 2019-12-20T06:40:19Z

sys/net/gnrc/netif/gnrc_netif.c

-        if ((netif == NULL) && (_netifs[i].ops == NULL)) {
-            netif = &_netifs[i];
-        }
+    if(DEVELHELP && GNRC_NETIF_SINGLE && netif_iter(NULL)) {


Suggested change

if(DEVELHELP && GNRC_NETIF_SINGLE && netif_iter(NULL)) {

if (DEVELHELP && GNRC_NETIF_SINGLE && netif_iter(NULL)) {

tests/gnrc_netif/main.c

jia200x · 2020-01-06T10:36:07Z

I removed the WIP label

sys/include/net/gnrc/netif.h

sys/include/net/gnrc/netif/ethernet.h

smlng

to me it looks like all *_create functions can be changed from int to void as always 0 is returned. Otherwise, nice work!

sys/include/net/gnrc/netif/ieee802154.h

sys/include/net/gnrc/netif/lorawan_base.h

sys/include/net/gnrc/netif/raw.h

sys/net/gnrc/link_layer/lwmac/lwmac.c

sys/net/gnrc/netif/gnrc_netif.c

jia200x · 2020-01-27T08:51:00Z

@smlng solved. In order to rebase, I will need to squash. May I?

smlng · 2020-01-27T09:05:17Z

yeah, please squash and rebase.

jia200x · 2020-01-27T09:55:47Z

@smlng done!

miri64

IMHO at least examples/gnrc_minimal should use the new macro, better would be if the build system could detect somehow how many network devices there are and set it if it is only one.

miri64 · 2020-01-28T15:46:40Z

sys/include/net/gnrc/netif/internal.h


+#ifndef GNRC_NETIF_SINGLE
+#define GNRC_NETIF_SINGLE (0)
+#endif


This should be exposed in sys/include/net/gnrc/netif/conf.h IMHO! This is an important optimization option that should not be hidden (also I think its usage isn't localized to the gnrc_netif module, no?)

sys/net/gnrc/application_layer/tftp/gnrc_tftp.c

miri64 · 2020-01-28T15:52:21Z

sys/net/gnrc/netif/gnrc_netif.c

-            netif = &_netifs[i];
-        }
+    if (DEVELHELP && GNRC_NETIF_SINGLE && netif_iter(NULL)) {
+        DEBUG("gnrc_netif: GNRC_NETIF_SINGLE set but more than one interface is being registered.");


Make this a LOG_WARNING (also line length!)

miri64 · 2020-01-28T15:53:25Z

sys/net/gnrc/netif/gnrc_netif.c

-        if ((netif == NULL) && (_netifs[i].ops == NULL)) {
-            netif = &_netifs[i];
-        }
+    if (DEVELHELP && GNRC_NETIF_SINGLE && netif_iter(NULL)) {


Suggested change

if (DEVELHELP && GNRC_NETIF_SINGLE && netif_iter(NULL)) {

if (IS_ACTIVE(DEVELHELP) && GNRC_NETIF_SINGLE && netif_iter(NULL)) {

done! I also added IS_ACTIVE to GNRC_NETIF_SINGLE

hmmm maybe is cleaner to change here that GNRC_NETIF_SINGLE to the gnrc_netif_highlander call.

miri64 · 2020-01-28T15:54:55Z

sys/net/gnrc/netif/gnrc_netif.c

-        }
+    if (DEVELHELP && GNRC_NETIF_SINGLE && netif_iter(NULL)) {
+        DEBUG("gnrc_netif: GNRC_NETIF_SINGLE set but more than one interface is being registered.");
+        assert(false);


Suggested change

assert(false);

assert(netif_iter(NULL) == NULL);

sys/net/gnrc/netif/gnrc_netif.c

miri64 · 2020-01-28T15:59:09Z

tests/gnrc_netif/common.c

 #include "od.h"

-static netdev_test_t _devs[GNRC_NETIF_NUMOF];
+static netdev_test_t _devs[4];


This needs a new internal define in common.h!

I forgot to add it during the PR cleanup phase

jia200x · 2020-01-28T20:09:11Z

Let's merge #13226 first

jia200x · 2020-03-27T11:40:17Z

strange... do you have the GNRC_NETIF_SINGLE option enabled?

jia200x · 2020-03-27T11:41:48Z

anyway, I will investigate

benpicco · 2020-03-27T11:42:48Z

Indeed when I set CFLAGS += -DGNRC_NETIF_SINGLE=1 I can ping without specifying the interface.

jia200x · 2020-03-27T11:45:46Z

ok, then I think I know what happened. The gnrc_netif_highlander() function is wrong.
It should be something like this:

if (IS_ACTIVE(GNRC_NETIF_SINGLE)) {
    return true;
}
else {
    return gnrc_netif_numof() == 1;
}

jia200x · 2020-03-27T11:46:27Z

could you quickly check if that works for you? If so, I can open a PR (unless you are faster)

benpicco · 2020-03-27T11:47:53Z

Yes this works!

jia200x · 2020-03-27T11:48:11Z

I'm on it

jia200x · 2020-03-27T12:00:57Z

It's there: #13736

leandrolanzieri · 2020-04-03T20:05:49Z

Similarly to the above, the test for emcute is failing, but it works when
CFLAGS += -DGNRC_NETIF_SINGLE=1.

Run test case
{'data_len_end': 503,
 'data_len_start': 0,
 'data_len_step': 50,
 'mode': 'sub',
 'qos_level': 0,
 'topic_name': '/test'}

Traceback (most recent call last):
  File "/home/leandro/Work/RIOT/tests/emcute/tests/01-run.py", line 482, in <module>
    sys.exit(run(testfunc, timeout=TIMEOUT, echo=False))
  File "/home/leandro/Work/RIOT/dist/pythonlibs/testrunner/__init__.py", line 30, in run
    testfunc(child)
  File "/home/leandro/Work/RIOT/tests/emcute/tests/01-run.py", line 473, in testfunc
    server.run()
  File "/usr/local/lib/python3.7/dist-packages/scapy/automaton.py", line 994, in run
    six.reraise(c.exc_info[0], c.exc_info[1], c.exc_info[2])
  File "/usr/local/lib/python3.7/dist-packages/scapy/modules/six.py", line 697, in reraise
    raise value
  File "/usr/local/lib/python3.7/dist-packages/scapy/automaton.py", line 836, in _do_control
    state = next(iterator)
  File "/usr/local/lib/python3.7/dist-packages/scapy/automaton.py", line 873, in _do_iter
    result=state_output, state=self.state.state)  # noqa: E501
scapy.automaton.ErrorState: Reached MESSAGE_TIMEOUT: ['\nCONNECT timed out']
make: *** [/home/leandro/Work/RIOT/tests/emcute/../../Makefile.include:733: test] Error 1

'GNRC_NETIF_NUMOF' has been removed in PR RIOT-OS#12994. The solution applied here has been adapted from a patch applied to (deprecated) 'ndn-riot' package [1]. From now on, it is the responsibility of the programmer to indicate how many interfaces are available/usable. [1] https://github.com/RIOT-OS/RIOT/pull/12994/files#diff-5c72dbe2d11f0fdf149c8a398c1175c3

jia200x added Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation Area: network Area: Networking State: WIP State: The PR is still work-in-progress and its code is not in its final presentable form yet labels Dec 19, 2019

jia200x requested review from gschorcht, haukepetersen, kaspar030, maribu and miri64 December 19, 2019 18:48

gschorcht requested changes Dec 20, 2019

View reviewed changes

jia200x changed the title ~~[WIP] gnrc_netif: implementation of dynamic GNRC_NETIF_NUMOF approach~~ gnrc_netif: implementation of dynamic GNRC_NETIF_NUMOF approach Jan 6, 2020

jia200x removed the State: WIP State: The PR is still work-in-progress and its code is not in its final presentable form yet label Jan 6, 2020

jia200x force-pushed the pr/gnrc_netif_desc_alloc branch from 92779d5 to b13c746 Compare January 6, 2020 10:47

smlng reviewed Jan 7, 2020

View reviewed changes

sys/include/net/gnrc/netif.h Outdated Show resolved Hide resolved

smlng reviewed Jan 7, 2020

View reviewed changes

sys/include/net/gnrc/netif/ethernet.h Outdated Show resolved Hide resolved

smlng reviewed Jan 7, 2020

View reviewed changes

jia200x force-pushed the pr/gnrc_netif_desc_alloc branch 3 times, most recently from 98d4c1c to 4a503e9 Compare January 27, 2020 09:55

miri64 previously requested changes Jan 28, 2020

View reviewed changes

jia200x mentioned this pull request Jan 28, 2020

gnrc_netif: add gnrc_netif_highlander function #13226

Merged

jia200x added the State: waiting for other PR State: The PR requires another PR to be merged first label Jan 28, 2020

jia200x force-pushed the pr/gnrc_netif_desc_alloc branch from 4a503e9 to 31be76e Compare March 17, 2020 13:16

jia200x requested review from aabadie, cgundogan and fjmolinas as code owners March 17, 2020 13:16

jia200x mentioned this pull request Mar 27, 2020

gnrc_netif: fix highlander function #13736

Closed

benpicco mentioned this pull request Mar 27, 2020

shell/gnrc_icmpv6_echo: fix incorrect use of gnrc_netif_highlander() #13737

Merged

jia200x mentioned this pull request Mar 31, 2020

[Tracking]: improve GNRC netif module #13763

Open

4 tasks

This was referenced Apr 3, 2020

tests: use start_network instead of ethos directly #13802

Closed

tests: fix for GNRC_NETIF_SINGLE #13835

Merged

This was referenced Apr 14, 2020

Release 2020.04 - RC1 RIOT-OS/Release-Specs#152

Closed

tinydtls: add sock_async support for sock_dtls #12907

Merged

miri64 mentioned this pull request Jun 30, 2020

cord_ep example timed out when registering to a resource directory #14399

Closed

	if(DEVELHELP && GNRC_NETIF_SINGLE && netif_iter(NULL)) {
	if (DEVELHELP && GNRC_NETIF_SINGLE && netif_iter(NULL)) {

	if (DEVELHELP && GNRC_NETIF_SINGLE && netif_iter(NULL)) {
	if (IS_ACTIVE(DEVELHELP) && GNRC_NETIF_SINGLE && netif_iter(NULL)) {

Conversation

jia200x commented Dec 19, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Contribution description

Testing procedure

Issues/PRs references

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jia200x commented Jan 6, 2020

Uh oh!

Uh oh!

Uh oh!

smlng left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jia200x commented Jan 27, 2020

Uh oh!

smlng commented Jan 27, 2020

Uh oh!

jia200x commented Jan 27, 2020

Uh oh!

miri64 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jia200x commented Jan 28, 2020

Uh oh!

jia200x commented Mar 27, 2020

Uh oh!

jia200x commented Mar 27, 2020

Uh oh!

benpicco commented Mar 27, 2020

Uh oh!

jia200x commented Mar 27, 2020

Uh oh!

jia200x commented Mar 27, 2020

jia200x commented Dec 19, 2019 •

edited

Loading