gnrc_netif: add gnrc_netif_highlander function by jia200x · Pull Request #13226 · RIOT-OS/RIOT

jia200x · 2020-01-28T20:08:34Z

Contribution description

This PR defines a gnrc_netif_single function that indicates if there's only one GNRC network interface.
It has the same purpose of GNRC_NETIF_NUMOF == 1 but having it as a function makes the migration to #12994 easier.
I also rearranged some functions to use standard ifs instead of macros, so the code is more readable and can be checked by static tests.

Testing procedure

make doc
Check that generates the same binaries when changing GNRC_NETIF_NUMOF

Issues/PRs references

miri64 · 2020-01-28T21:02:39Z

In general 👍. I'm not 100% sure about the name, but maybe some sleep gets me over that. I also want to make some sample checks if the size does not change.

jia200x · 2020-01-29T09:55:05Z

hmmmm maybe gnrc_netif_single_instance()?

miri64 · 2020-01-29T10:02:46Z

hmmmm maybe gnrc_netif_single_instance()?

My problem with both this name and the one proposed in the PR, is that it implies to me that the function returns true if there is a single interface / instance. I.e. with the rework in #12994 I would expect the function to return gnrc_netif_iter(NULL)->next == NULL. This, as far as I understand, can't be the case: The result must be statically dependent on GNRC_NETIF_NUMOF/GNRC_NETIF_SINGLE. Otherwise we lose the optimization advantage.

miri64 · 2020-01-29T10:04:59Z

gnrc_netif_there_can_be_only_one()? gnrc_netif_highlander()? 😅

miri64 · 2020-01-29T10:08:21Z

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

 *
 * @attention   Requires prior locking
- * @note        Assumed to be true, when @ref GNRC_NETIF_NUMOF == 1 and
+ * @note        Assumed to be true, when @ref gnrc_netif_single return true and


Independent of the name

Suggested change

* @note Assumed to be true, when @ref gnrc_netif_single return true and

* @note Assumed to be true, when @ref gnrc_netif_single() return true and

miri64 · 2020-01-29T11:51:33Z

gnrc_netif_there_can_be_only_one()? gnrc_netif_highlander()? sweat_smile

The more I think about it: Does anything speak against this joke (gnrc_netif_highlander())? It wouldn't be the first pop-culture reference in software engineering, it references the fact, that there can be only one interface, and gives enough pause compared to gnrc_netif_single() that a dev will look up the doc if they really are unsure what this function is doing.

jia200x · 2020-01-29T12:23:06Z

The more I think about it: Does anything speak against this joke (gnrc_netif_highlander())? It wouldn't be the first pop-culture reference in software engineering, it references the fact, that there can be only one interface, and gives enough pause compared to gnrc_netif_single() that a dev will look up the doc if they really are unsure what this function is doing.

+1 on my side :P

miri64 · 2020-01-29T12:54:08Z

Then lets go ahead with that :-)

miri64

At a closer glance, apart from the rename, there are some issues other issues:

miri64 · 2020-01-29T15:53:24Z

sys/include/net/gnrc/netif.h

+    if(GNRC_NETIF_NUMOF == 1) {
+        return true;
+    }
+    else {
+        return false;
+    }


Can be boiled down to

return (GNRC_NETIF_NUMOF == 1);

I didn't have enough coffee that day

miri64 · 2020-01-29T15:53:40Z

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

-    return gnrc_netif_is_rtr(netif) && gnrc_netif_is_6ln(netif);
+     /* if flag checkers even evaluate, otherwise just assume their result */
+    if (GNRC_IPV6_NIB_CONF_6LR && (IS_USED(MODULE_GNRC_IPV6_ROUTER) ||
+                                   (GNRC_NETIF_NUMOF > 1) ||


Suggested change

(GNRC_NETIF_NUMOF > 1) ||

(!gnrc_netif_single()) ||

miri64 · 2020-01-29T15:55:08Z

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

+    if((!gnrc_netif_single() &&
+       IS_USED(MODULE_GNRC_SIXLOWPAN)) || \
+       IS_USED(MODULE_GNRC_SIXLOENC)) {
+        switch (netif->device_type) {
+#ifdef MODULE_GNRC_SIXLOENC
+            case NETDEV_TYPE_ETHERNET:
+                return (netif->flags & GNRC_NETIF_FLAGS_6LO);
 #endif
+            case NETDEV_TYPE_IEEE802154:
+            case NETDEV_TYPE_CC110X:
+            case NETDEV_TYPE_BLE:
+            case NETDEV_TYPE_NRFMIN:
+            case NETDEV_TYPE_ESP_NOW:
+                return true;
+            default:
+                return false;
+        }
+    }


Have you checked the memory impact of moving this to static inline? examples/gnrc_border_router should be the application to look at.

yes, it increased. The last commit fixes that problem (gnrc_netif_is_6lo is still static inline but I added a non-static function to check if the device requires 6Lo).

Now gnrc_border_router has the same size with and without this PR :)

jia200x · 2020-03-05T14:28:00Z

@miri64 may I squash?

miri64 · 2020-03-05T14:50:18Z

Yes, please!

jia200x · 2020-03-06T14:21:26Z

done :)

miri64

Tested with make all. The binaries for examples/gnrc_networking / examples/gnrc_border_router do have the same size, but their make objdump output differs (this might have different reasons though). For unification it could make sense to also adapt gnrc_netif_is_6ln() to the new style, but this can also be done in a follow-up cleanup.

jia200x · 2020-03-10T10:39:39Z

There were some errors due to functions defined with macros. Instead of adding "(void)" here and there, I changed those functions to their inline variant.

jia200x · 2020-03-10T11:33:46Z

Let's merge #13605 first

leandrolanzieri · 2020-03-12T09:10:55Z

Let's merge #13605 first

Merged

miri64 · 2020-03-12T09:46:23Z

No need to let Murdock run this twice ;-).

miri64

Some style nits were introduced. Also, one style fix is possible without much notice.

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

sys/net/gnrc/network_layer/ipv6/gnrc_ipv6.c

sys/net/gnrc/routing/rpl/gnrc_rpl_auto_init.c

jia200x · 2020-03-12T14:25:27Z

Done. When I squash, I will also change the commit that introduces the function to reflect the new name of the function.

miri64

Murdock's not happy.

sys/net/gnrc/routing/rpl/gnrc_rpl_auto_init.c

miri64 · 2020-03-12T15:20:57Z

sys/net/gnrc/routing/rpl/gnrc_rpl_auto_init.c

+    else {
+        /* XXX this is just a work-around ideally this should be defined in some
+         * run-time interface configuration */
+        DEBUG("auto_init_gnrc_rpl: please specify an interface" \


You are removing a space here. The \ is also not necessary.

Suggested change

DEBUG("auto_init_gnrc_rpl: please specify an interface" \

DEBUG("auto_init_gnrc_rpl: please specify an interface "

leandrolanzieri · 2020-03-16T08:03:20Z

@jia200x this needs rebasing now

jia200x · 2020-03-17T09:54:57Z

squashed and rebased

miri64

ACK. Let's finally merge this! The line noted can also be fixed as a follow-up.

miri64 · 2020-03-17T11:19:30Z

sys/net/gnrc/netif/gnrc_netif.c

+#ifdef MODULE_GNRC_SIXLOENC
+        case NETDEV_TYPE_ETHERNET:
+            return (netif->flags & GNRC_NETIF_FLAGS_6LO);
+#endif


In theory this can also use link-time optimization as well:

case NETDEV_TYPE_ETHERNET: if (IS_USED(MODULE_GNRC_SIXLOENC)) { return (netif->flags & GNRC_NETIF_FLAGS_6LO); } /* intentionally falls through */

jia200x added Area: network Area: Networking Type: cleanup The issue proposes a clean-up / The PR cleans-up parts of the codebase / documentation labels Jan 28, 2020

jia200x requested a review from miri64 January 28, 2020 20:08

jia200x mentioned this pull request Jan 28, 2020

gnrc_netif: implementation of dynamic GNRC_NETIF_NUMOF approach #12994

Merged

miri64 reviewed Jan 29, 2020

View reviewed changes

miri64 requested changes Jan 29, 2020

View reviewed changes

jia200x changed the title ~~gnrc_netif: add gnrc_netif_single function~~ gnrc_netif: add gnrc_netif_highlander function Feb 7, 2020

jia200x force-pushed the pr/gnrc_netif_single branch from 93338a2 to 0c54b77 Compare March 5, 2020 15:26

jia200x requested review from PeterKietzmann, cgundogan and haukepetersen as code owners March 5, 2020 15:26

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

miri64 approved these changes Mar 6, 2020

View reviewed changes

jia200x removed the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Mar 10, 2020

jia200x mentioned this pull request Mar 10, 2020

gnrc_ipv6_nib: remove parenthesis from boolean configs #13605

Merged

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

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

miri64 reviewed Mar 12, 2020

View reviewed changes

leandrolanzieri reviewed Mar 12, 2020

View reviewed changes

sys/net/gnrc/routing/rpl/gnrc_rpl_auto_init.c Outdated Show resolved Hide resolved

miri64 reviewed Mar 12, 2020

View reviewed changes

sys/net/gnrc/routing/rpl/gnrc_rpl_auto_init.c Outdated Show resolved Hide resolved

sys/net/gnrc/routing/rpl/gnrc_rpl_auto_init.c Show resolved Hide resolved

sys/net/gnrc/routing/rpl/gnrc_rpl_auto_init.c Show resolved Hide resolved

miri64 reviewed Mar 12, 2020

View reviewed changes

jia200x mentioned this pull request Mar 12, 2020

gnrc_rpl: add default value to GNRC_RPL_DEFAULT_NETIF #13628

Merged

jia200x added 4 commits March 17, 2020 10:50

gnrc_netif: add function to check if single GNRC netif

121626b

gnrc_netif: use gnrc_netif_single where possible

6ace7b5

gnrc_netif: add function to check if device requires 6Lo

4393822

gnrc_netif: change macros to inline functions

c2e491d

jia200x force-pushed the pr/gnrc_netif_single branch from 7ca3efb to c2e491d Compare March 17, 2020 09:54

miri64 approved these changes Mar 17, 2020

View reviewed changes

miri64 merged commit 171907d into RIOT-OS:master Mar 17, 2020

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

	* @note Assumed to be true, when @ref gnrc_netif_single return true and
	* @note Assumed to be true, when @ref gnrc_netif_single() return true and

	DEBUG("auto_init_gnrc_rpl: please specify an interface" \
	DEBUG("auto_init_gnrc_rpl: please specify an interface "

Conversation

jia200x commented Jan 28, 2020

Contribution description

Testing procedure

Issues/PRs references

Uh oh!

miri64 commented Jan 28, 2020

Uh oh!

jia200x commented Jan 29, 2020

Uh oh!

miri64 commented Jan 29, 2020

Uh oh!

miri64 commented Jan 29, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

miri64 commented Jan 29, 2020

Uh oh!

jia200x commented Jan 29, 2020

Uh oh!

miri64 commented Jan 29, 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!

Choose a reason for hiding this comment

Uh oh!

jia200x commented Mar 5, 2020

Uh oh!

miri64 commented Mar 5, 2020

Uh oh!

jia200x commented Mar 6, 2020

Uh oh!

miri64 left a comment

Choose a reason for hiding this comment

Uh oh!

jia200x commented Mar 10, 2020

Uh oh!

jia200x commented Mar 10, 2020

Uh oh!

leandrolanzieri commented Mar 12, 2020

Uh oh!

miri64 commented Mar 12, 2020

Uh oh!

miri64 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jia200x commented Mar 12, 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!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

leandrolanzieri commented Mar 16, 2020

Uh oh!

jia200x commented Mar 17, 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!

jia200x commented Mar 12, 2020 •

edited

Loading