gnrc_rpl: port to nib by cgundogan · Pull Request #8010 · RIOT-OS/RIOT

cgundogan · 2017-11-11T20:38:06Z

replaces fib with nib/ft

WIP for now. There are still some modifications missing in the tests for gnrc_ipv6_nib_ft_add e.g.
Also, I have to re-evaluate the netif2 changes ..
Will continue tomorrow with that.

This PR is part of the network layer remodelling effort:

depends on #8083

miri64

If you don't change the API you don't have to adapt the tests ;-).

miri64 · 2017-11-11T20:44:18Z

sys/include/net/gnrc/ipv6/nib/ft.h

 */
 int gnrc_ipv6_nib_ft_add(const ipv6_addr_t *dst, unsigned dst_len,
-                         const ipv6_addr_t *next_hop, unsigned iface);
+                         const ipv6_addr_t *next_hop, uint16_t ltime,


This was kept out of the intentionally. The routing protocol should timeout the routes externally (e.g. by storing the context for the timer in the protocol's routing table)

This is weird. Every FIB entry should have a lifetime. RPL is not aware of link failures way down the tree as it does not maintain a children list, only a parent list. So RPL does not know when to prune an entry in that case. FIB maintenance should be in the FIB, not in a routing protocol.

This isn't about FIB maintenance, but about route maintenance IMHO. The lifetime of the route (and its scale) is intrinsic to the routing protocol (if you want a proof for that, see various discussions in our GitHub issue history about which unit should be used with route lifetimes).

Iff [edit]and I'm not saying we should[/edit] we do it like this I have some questions: Why is the route lifetime in seconds (isn't milliseconds a more usual unit for that)? Why did you choose uint16_t as the type for that?

Why is the route lifetime in seconds (isn't milliseconds a more usual unit for that)? Why did you choose uint16_t as the type for that?

We had the discussion for gnrc_fib that milliseconds granularity is overkill and the port to deciseconds was never done.. I guess having a second granularity is okay for our constrained IoT scenario and routing protocols usually are not that fast in noticing routing state changes.

Oh, and I chose uint16_t because that's what the default router struct is also using.

miri64 · 2017-11-11T21:37:14Z

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

-                }
-            }
+    /* add external and RPL FT entries */
+    /* TODO: nib: dropped support for external transit options for now */


What do you mean by that?

In case we don't get this in before your vacation and someone else needs to take over: can you please answer my question?

RPL is able to distribute routing states that were not learned/produced by RPL, but from an external source – like another routing protocol or a manually admnistered route entry. These routing states should be marked with the E flag in DAO transit optoins [1].

External (E): 1-bit flag. The 'E' flag is set to indicate that the
parent router redistributes external targets into the RPL
network. An external Target is a Target that has been learned
through an alternate protocol. The external targets are listed
in the Target options that immediately precede the Transit
Information option. An external Target is not expected to
support RPL messages and options.

In the "old" fib implementation user defined flags could be stored along the forwarding entry. We used that to mark a forwarding entry as "learned by RPL" and used this information to decide whether to set or not set the E flag in DAOs.

This feature is missing in the current nib_ft implementation that's why I put a note about dropped support there.

[1] https://tools.ietf.org/html/rfc6550#section-6.7.8

Okay. I guess we'll need to extend the NIB for something like this later on ;-).

miri64

Where do you start the timer btw?

miri64 · 2017-11-11T21:42:12Z

sys/net/gnrc/network_layer/ipv6/nib/nib_ft.c

-        ptr = _nib_ft_add(next_hop, iface, dst, dst_len);
+        _nib_offl_entry_t *offl = _nib_ft_add(next_hop, iface, dst, dst_len);
+        if (offl) {
+            offl->valid_until = offl->pref_until = ltime * MS_PER_SEC;


Iff we add this feature we need to add a new field to _nib_offl_entry_t. You are risking here to override prefix lifetimes and thus causing potential side effects.

miri64 · 2017-11-12T10:02:55Z

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

-        fib_remove_entry(&gnrc_ipv6_fib_table,
-                         (uint8_t *) ipv6_addr_unspecified.u8,
-                         sizeof(ipv6_addr_t));
+        gnrc_ipv6_nib_ft_del(&ipv6_addr_unspecified, 0);


Note to self: this might cause problems with address registration => investigate.

miri64 · 2017-11-12T18:13:20Z

Ideally there shouldn't be any comment here. But just in case: this is a warning for that ;-).

miri64 · 2017-11-12T18:14:03Z

Worked :-)

miri64 · 2017-11-18T13:36:36Z

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

+    /* TODO: nib: dropped support for external transit options for now */
+    void *ft_state = NULL;
+    gnrc_ipv6_nib_ft_t fte;
+    while(gnrc_ipv6_nib_ft_iter(NULL, dodag->iface, &state, &fte)) {


Should be ft_state right?

cgundogan · 2017-11-19T19:50:11Z

rebased onto #8083

miri64

(Untested) ACK. Please squash. If any errors except the ones related to the NIB you already reported offline, I'll fix them in a follow-up.

miri64 · 2017-11-19T22:55:10Z

sys/net/gnrc/netif/gnrc_netif.c

                      netopt2str(opt->opt));
 #else
-                DEBUG("gnrc_netif: GNRC_NETAPI_MSG_TYPE_SET received. opt=%s\n",
+                DEBUG("gnrc_netif: GNRC_NETAPI_MSG_TYPE_SET received. opt=%d\n",


Unrelated, but okay, as long as it stays its own commit.

miri64 · 2017-11-19T22:55:21Z

sys/net/gnrc/network_layer/ipv6/nib/_nib-arsm.c

                               GNRC_IPV6_NIB_NC_INFO_NUD_STATE_UNREACHABLE);
            }
-            /* falls through intentionally */
+            /* intentionally falls through */


Dito (also has GCC become so picky??!!? oO)

Don't know if we have a stricter fall through level now, but assuming we have level 3[1],
then the previous string doesn't match the given regex. However, my compiler was able to compile it sometimes, and other times it didn't want to .. with the provided change it worked every time so far.

[1] https://gcc.gnu.org/onlinedocs/gcc/Warning-Options.html

miri64 · 2017-11-19T22:57:16Z

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

+
+                gnrc_ipv6_nib_ft_add(&(target->target), target->prefix_length, src,
+                                     dodag->iface,
+                                     dodag->default_lifetime * dodag->lifetime_unit);


Not part of this PR, but is it really necessary to store the lifetime_unit of a DODAG Oo?

The lifetime and the lifetimeunit are both distributed in DIOs. Need to store that to further distribute it (:

miri64 · 2017-11-19T22:58:00Z

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

-                }
-            }
+    /* add external and RPL FT entries */
+    /* TODO: nib: dropped support for external transit options for now */


Okay. I guess we'll need to extend the NIB for something like this later on ;-).

miri64 · 2017-11-19T23:23:23Z

sys/net/gnrc/routing/rpl/p2p/gnrc_rpl_p2p.c

@@ -346,11 +346,9 @@ void gnrc_rpl_p2p_recv_DRO(gnrc_pktsnip_t *pkt, ipv6_addr_t *src)
    }

    if (gnrc_ipv6_netif_find_by_addr(&me, &addr) == dodag->iface) {


Are you sure, this is the right function as well?

Discussed offline.

miri64

Re-ACK

miri64 · 2017-11-19T23:32:59Z

Can you also please squash 70d0164 and 9873671 They belong together IMHO.

cgundogan · 2017-11-19T23:35:31Z

Can you also please squash 70d0164 and 9873671 They belong together IMHO.

done

miri64 · 2017-11-19T23:42:01Z

Go!

cgundogan · 2017-11-19T23:42:13Z

woohoo

cgundogan added Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation GNRC 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 Nov 11, 2017

cgundogan assigned miri64 Nov 11, 2017

cgundogan requested review from BytesGalore, bergzand and miri64 November 11, 2017 20:38

cgundogan changed the title ~~rpl: port to nib~~ gnrc_rpl: port to nib Nov 11, 2017

miri64 requested changes Nov 11, 2017

View reviewed changes

miri64 reviewed Nov 11, 2017

View reviewed changes

miri64 requested changes Nov 11, 2017

View reviewed changes

miri64 mentioned this pull request Nov 11, 2017

gnrc_netif2: gnrc_ipv6_nib: full integration into GNRC #7925

Merged

28 tasks

miri64 reviewed Nov 12, 2017

View reviewed changes

miri64 mentioned this pull request Nov 17, 2017

gnrc_ipv6_nib: add timeouts to routes #8073

Merged

miri64 requested changes Nov 18, 2017

View reviewed changes

miri64 added the State: waiting for other PR State: The PR requires another PR to be merged first label Nov 18, 2017

cgundogan force-pushed the pr/rpl_nib branch 4 times, most recently from 2bd88b0 to 81b70b8 Compare November 19, 2017 19:49

cgundogan removed the State: WIP State: The PR is still work-in-progress and its code is not in its final presentable form yet label Nov 19, 2017

cgundogan force-pushed the pr/rpl_nib branch 2 times, most recently from d3a1a4c to 63a4bfc Compare November 19, 2017 22:53

miri64 approved these changes Nov 19, 2017

View reviewed changes

miri64 reviewed Nov 19, 2017

View reviewed changes

miri64 removed the State: waiting for other PR State: The PR requires another PR to be merged first label Nov 19, 2017

miri64 approved these changes Nov 19, 2017

View reviewed changes

cgundogan force-pushed the pr/rpl_nib branch from 470ba35 to 3565c04 Compare November 19, 2017 23:32

miri64 added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Nov 19, 2017

cgundogan added 3 commits November 20, 2017 00:35

rpl: replace fib with nib

5b5c479

nib: use correct fall through hint

3be5c9f

gnrc_netif: fix DEBUG output

de5e0c6

cgundogan force-pushed the pr/rpl_nib branch from 3565c04 to de5e0c6 Compare November 19, 2017 23:35

miri64 merged commit 8a79b67 into RIOT-OS:gnrc_netif2_integration/master Nov 19, 2017

cgundogan deleted the pr/rpl_nib branch November 19, 2017 23:42

aabadie added this to the Release 2018.01 milestone Jan 18, 2018

miri64 mentioned this pull request Feb 22, 2023

sys/fib: deprecate the module #19300

Closed

		@@ -346,11 +346,9 @@ void gnrc_rpl_p2p_recv_DRO(gnrc_pktsnip_t pkt, ipv6_addr_t src)
		}

		if (gnrc_ipv6_netif_find_by_addr(&me, &addr) == dodag->iface) {

Conversation

cgundogan commented Nov 11, 2017 • 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!

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!

miri64 Nov 11, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

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!

miri64 commented Nov 12, 2017

Uh oh!

miri64 commented Nov 12, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cgundogan commented Nov 19, 2017

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!

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!

miri64 Nov 19, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

miri64 left a comment

Choose a reason for hiding this comment

Uh oh!

miri64 commented Nov 19, 2017

Uh oh!

cgundogan commented Nov 19, 2017

Uh oh!

miri64 commented Nov 19, 2017

Uh oh!

cgundogan commented Nov 19, 2017

Uh oh!

Reviewers

Assignees

cgundogan commented Nov 11, 2017 •

edited

Loading

miri64 Nov 11, 2017 •

edited

Loading

miri64 Nov 19, 2017 •

edited

Loading