gnrc_netif: fix highlander function #13736

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

Contribution description

This PR fixes the gnrc_netif_highlander function to provide the correct values when GNRC_NETIF_SINGLE is not defined. There are cases where GNRC_NETIF_SINGLE=0 but there's only one interface, which was not covered by the upstream implementation.

This fixes the issue described in #12994 (comment)

Testing procedure

Using any GNRC example, add this snippet somewhere:

assert((gnrc_netif_numof() != 1 && gnrc_netif_single() == false) || (gnrc_netif_numof() == 1 && gnrc_netif_single() == true));

It should pass for both cases of GNRC_NETIF_SINGLE when there's only one interface. It should also pass when GNRC_NETIF_SINGLE=0 and there's more than one interface.

You should be able to ping now via the link local address if there's only one interface, for both cases of GNRC_NETIF_SINGLE

Issues/PRs references

Fixes #12994 (comment)

benpicco

This fixes the issue I was observing and the logic is sound.

benpicco · 2020-03-27T12:14:35Z

Meh, I now get an assertion error on the border router:

2020-03-27 13:13:06,929 # gnrc_netif: gnrc_netif_highlander() returned true but more than one interface is being registered.
2020-03-27 13:13:06,931 # *** RIOT kernel panic:
2020-03-27 13:13:06,933 # FAILED ASSERTION.

miri64

NACK. IMHO gnrc_netif_highlander() should stay an optimizing function. The tagline even says "there can onyl be one", not "there may only be one" ;-). Also using gnrc_netif_numof() has the potential to bring very long runtimes (as it iterates over the list of all interfaces) if the number of interfaces is greater than 1. IMHO we should fix this rather in ping6. udp send in gnrc_networking is still working.

benpicco · 2020-03-27T12:33:19Z

Not just ping6

--- a/sys/net/gnrc/routing/rpl/gnrc_rpl_auto_init.c
+++ b/sys/net/gnrc/routing/rpl/gnrc_rpl_auto_init.c
@@ -28,7 +28,7 @@
 
 void auto_init_gnrc_rpl(void)
 {
-    if (gnrc_netif_highlander()) {
+    if (gnrc_netif_numof() == 1) {
         gnrc_netif_t *netif = gnrc_netif_iter(NULL);
         if (netif == NULL) {
             /* XXX this is just a work-around ideally this would happen with
diff --git a/sys/net/gnrc/sock/include/gnrc_sock_internal.h b/sys/net/gnrc/sock/include/gnrc_sock_internal.h
index a91a94ce25..45eb29e9f5 100644
--- a/sys/net/gnrc/sock/include/gnrc_sock_internal.h
+++ b/sys/net/gnrc/sock/include/gnrc_sock_internal.h
@@ -95,7 +95,7 @@ static inline void gnrc_ep_set(sock_ip_ep_t *out, const sock_ip_ep_t *in,
                                size_t in_size)
 {
     memcpy(out, in, in_size);
-    if (gnrc_netif_highlander()) {
+    if (gnrc_netif_numof() == 1) {
         /* set interface implicitly */
         gnrc_netif_t *netif = gnrc_netif_iter(NULL);
 
diff --git a/sys/shell/commands/sc_gnrc_icmpv6_echo.c b/sys/shell/commands/sc_gnrc_icmpv6_echo.c
index 9dddcc8abe..623144333c 100644
--- a/sys/shell/commands/sc_gnrc_icmpv6_echo.c
+++ b/sys/shell/commands/sc_gnrc_icmpv6_echo.c
@@ -187,7 +187,7 @@ static int _configure(int argc, char **argv, _ping_data_t *data)
             if (iface) {
                 data->netif = gnrc_netif_get_by_pid(atoi(iface));
             }
-            else if (gnrc_netif_highlander()) {
+            else if (gnrc_netif_numof() == 1) {
                 data->netif = gnrc_netif_iter(NULL);
             }
 
@@ -371,7 +371,7 @@ static void _print_reply(_ping_data_t *data, gnrc_pktsnip_t *icmpv6,
             data->num_recv++;
             dupmsg += 7;
         }
-        if (gnrc_netif_highlander() || (if_pid == KERNEL_PID_UNDEF) ||
+        if ((gnrc_netif_numof() == 1) || (if_pid == KERNEL_PID_UNDEF) ||
             !ipv6_addr_is_link_local(from)) {
             printf("%u bytes from %s: icmp_seq=%u ttl=%u",
                    (unsigned)icmpv6->size,

miri64 · 2020-03-27T12:40:38Z

Again gnrc_netif_highlander() != gnrc_netif_numof() == 1 (where is this patch coming from btw??!?). gnrc_netif_highlander() is an optimization function that guarantees, that there can be only one interface (compare to GNRC_NETIF_NUMOF == 1 before #12994). So it represents a static configuration that allows for compile-time optimization. gnrc_netif_numof() counts the number of currently initialized interfaces, so it only determines dynamically the number of interfaces. Using gnrc_netif_numof() to check if there is only one interface in a hot path can lead to performance issues, as it potentially iterates over a longer list of interfaces, just to determine if there is only one...

benpicco · 2020-03-27T12:42:28Z

But gnrc_netif_highlander() will return false even if there is only one interface.
In those instances the function is supposed to check for that - not the maximum number of interfaces.

miri64 · 2020-03-27T12:44:35Z

But gnrc_netif_highlander() will return false even if there is only one interface.

Yes, because there can be more (because interfaces are lists now, so dynamically enlargable).

In those instances the function is supposed to check for that - not the maximum number of interfaces.

Again, this function was introduced as a static optimization function and should only be used as such.

miri64 · 2020-03-27T12:45:38Z

But gnrc_netif_highlander() will return false even if there is only one interface.

Yes, because there can be more (because interfaces are lists now, so dynamically enlargable).

highlander == "There can only be one!" ;-) Should rather be "There must only be one"

miri64 · 2020-03-27T12:49:31Z

--- a/sys/net/gnrc/routing/rpl/gnrc_rpl_auto_init.c
+++ b/sys/net/gnrc/routing/rpl/gnrc_rpl_auto_init.c
@@ -28,7 +28,7 @@
 
 void auto_init_gnrc_rpl(void)
 {
-    if (gnrc_netif_highlander()) {
+    if (gnrc_netif_numof() == 1) {
         gnrc_netif_t *netif = gnrc_netif_iter(NULL);
         if (netif == NULL) {
             /* XXX this is just a work-around ideally this would happen with

This is legitimate. There are even warnings printed that configuration is missing in case gnrc_netif_highlander() == false.

diff --git a/sys/net/gnrc/sock/include/gnrc_sock_internal.h b/sys/net/gnrc/sock/include/gnrc_sock_internal.h
index a91a94ce25..45eb29e9f5 100644
--- a/sys/net/gnrc/sock/include/gnrc_sock_internal.h
+++ b/sys/net/gnrc/sock/include/gnrc_sock_internal.h
@@ -95,7 +95,7 @@ static inline void gnrc_ep_set(sock_ip_ep_t *out, const sock_ip_ep_t *in,
                                size_t in_size)
 {
     memcpy(out, in, in_size);
-    if (gnrc_netif_highlander()) {
+    if (gnrc_netif_numof() == 1) {
         /* set interface implicitly */
         gnrc_netif_t *netif = gnrc_netif_iter(NULL);

Dito. An API should not assume the interface that there is more than one interface possible. This was the same behavior before when GNRC_NETIF_NUMOF > 1 and the number of initialized interfaces was < GNRC_NETIF_NUMOF

diff --git a/sys/shell/commands/sc_gnrc_icmpv6_echo.c b/sys/shell/commands/sc_gnrc_icmpv6_echo.c
index 9dddcc8abe..623144333c 100644
--- a/sys/shell/commands/sc_gnrc_icmpv6_echo.c
+++ b/sys/shell/commands/sc_gnrc_icmpv6_echo.c
@@ -187,7 +187,7 @@ static int _configure(int argc, char **argv, _ping_data_t *data)
             if (iface) {
                 data->netif = gnrc_netif_get_by_pid(atoi(iface));
             }
-            else if (gnrc_netif_highlander()) {
+            else if (gnrc_netif_numof() == 1) {
                 data->netif = gnrc_netif_iter(NULL);
             }
 
@@ -371,7 +371,7 @@ static void _print_reply(_ping_data_t *data, gnrc_pktsnip_t *icmpv6,
             data->num_recv++;
             dupmsg += 7;
         }
-        if (gnrc_netif_highlander() || (if_pid == KERNEL_PID_UNDEF) ||
+        if ((gnrc_netif_numof() == 1) || (if_pid == KERNEL_PID_UNDEF) ||
             !ipv6_addr_is_link_local(from)) {
             printf("%u bytes from %s: icmp_seq=%u ttl=%u",
                    (unsigned)icmpv6->size,

Will provide fix

benpicco · 2020-03-27T13:00:06Z

I'm all for whimsical names, but in this case maybe a gnrc_netif_leq_one()/ gnrc_netif_max_one() would be clearer 😆

jia200x · 2020-03-27T14:18:59Z

as discussed offline with @miri64, it's probably a good idea to introduce a gnrc_netif_single() (or gnrc_netif_leq_one() or gnrc_netif_max_one) that returns true or false if there's only one interface. The compile will automatically optimize out all code using that function anyway.
If we add this then we can gradually remove gnrc_netif_highlander. It will not be needed because the optimization is done anyway

miri64 · 2020-03-27T17:40:16Z

Ok, but I think this is a distinct effort from this PR and now that #13737 we can close this PR.

gnrc_netif: fix highlander function

eec69b8

jia200x added Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors) Area: network Area: Networking labels Mar 27, 2020

jia200x requested review from miri64 and benpicco March 27, 2020 12:00

jia200x mentioned this pull request Mar 27, 2020

gnrc_netif: implementation of dynamic GNRC_NETIF_NUMOF approach #12994

Merged

benpicco approved these changes Mar 27, 2020

View reviewed changes

benpicco added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Mar 27, 2020

leandrolanzieri added this to the Release 2020.04 milestone Mar 27, 2020

leandrolanzieri added the Impact: minor The PR is small in size and might only require a quick look of a knowledgeable reviewer label Mar 27, 2020

miri64 requested changes Mar 27, 2020

View reviewed changes

benpicco mentioned this pull request Mar 27, 2020

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

Merged

miri64 closed this Mar 27, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

gnrc_netif: fix highlander function #13736

gnrc_netif: fix highlander function #13736

Uh oh!

jia200x commented Mar 27, 2020

Uh oh!

benpicco left a comment

Uh oh!

benpicco commented Mar 27, 2020 •

edited

Loading

Uh oh!

miri64 left a comment

Uh oh!

benpicco commented Mar 27, 2020

Uh oh!

miri64 commented Mar 27, 2020

Uh oh!

benpicco commented Mar 27, 2020

Uh oh!

miri64 commented Mar 27, 2020 •

edited

Loading

Uh oh!

miri64 commented Mar 27, 2020

Uh oh!

miri64 commented Mar 27, 2020

Uh oh!

benpicco commented Mar 27, 2020

Uh oh!

jia200x commented Mar 27, 2020

Uh oh!

miri64 commented Mar 27, 2020

Uh oh!

Uh oh!

gnrc_netif: fix highlander function #13736

gnrc_netif: fix highlander function #13736

Uh oh!

Conversation

jia200x commented Mar 27, 2020

Contribution description

Testing procedure

Issues/PRs references

Uh oh!

benpicco left a comment

Choose a reason for hiding this comment

Uh oh!

benpicco commented Mar 27, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

miri64 left a comment

Choose a reason for hiding this comment

Uh oh!

benpicco commented Mar 27, 2020

Uh oh!

miri64 commented Mar 27, 2020

Uh oh!

benpicco commented Mar 27, 2020

Uh oh!

miri64 commented Mar 27, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

miri64 commented Mar 27, 2020

Uh oh!

miri64 commented Mar 27, 2020

Uh oh!

benpicco commented Mar 27, 2020

Uh oh!

jia200x commented Mar 27, 2020

Uh oh!

miri64 commented Mar 27, 2020

Uh oh!

Uh oh!

benpicco commented Mar 27, 2020 •

edited

Loading

miri64 commented Mar 27, 2020 •

edited

Loading