gnrc_ipv6_nib: initial import of internal NIB functions by miri64 · Pull Request #6325 · RIOT-OS/RIOT

miri64 · 2017-01-11T11:23:43Z

Reasoning for this PR

To put it mildly: the new NDP implementation will be a cluster fuck of interdependent PRs anyways, so I started with an import of the internal structure needed for a working Address Resolution/Registration (AR) and Neighbor Unreachable Discovery (NUD). Including tests, doc and headers this is already ~2000 loc, but thankfully the implementation itself is only 250 loc long ;-).

Structure

The internal structure of the NIB is made to waste the least amount of RAM considering its complex structure:

Off-link entries are currently omitted from this PR, as they are not required for AR and NUD.

Edit: model was updated and is still WIP https://miri64.github.io/riot-ndp-model/

cgundogan · 2017-01-11T11:55:59Z

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

+#define GNRC_IPV6_NIB_QUEUE_PACKETS         (1)
+#endif
+
+#ifndef GNRC_IPV6_NIB_DO_HANDLE_NBR_SOL


What's the idea of incuding this long list of compile time flags? It should suffice to check their availability with if[n]def where they are needed. Maybe I misunderstand and they are used not as compile time switches, sorry if so, but there is no code using them currently (:

They are used as compile switches, but I did not want them to be #ifndef enabled flags, but #if enabled flags, so you can deactivate them beyond the default config easily, too. This is actually more important than activating them, since e.g. 6Lo-ND doesn't need to handle TLLAOs or redirect messages.

miri64 · 2017-01-13T14:08:44Z

Added the default router selection algorithm from RFC 4861.

miri64 · 2017-03-20T10:28:39Z

Rebased to current master

miri64 · 2017-04-25T14:58:15Z

Rebased to current master and adapted to the new model

miri64 · 2017-04-26T13:07:39Z

Ready for review.

miri64 · 2017-04-26T14:52:48Z

Just found another issue with this PR, when working with the NC component. Will fix asap.

miri64 · 2017-04-28T11:36:49Z

Issue resolved.

cgundogan · 2017-06-09T08:20:08Z

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

+
+_nib_onl_entry_t *_nib_onl_iter(const _nib_onl_entry_t *last)
+{
+    for (const _nib_onl_entry_t *node = (last) ? last + 1 : _nodes;


I don't understand the motivation for const here, since the array that is iterated over is also not an array of const elements. IMO there's no need for that and makes the below lines less verbose.

Const correctness is important for preventing coding mistakes and may help the compiler how to optimize.

What @gebart is saying

IMO there's no need for that and makes the below lines less verbose.

How that?

cgundogan · 2017-06-09T08:42:34Z

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

+    assert(cstate != GNRC_IPV6_NIB_NC_INFO_NUD_STATE_PROBE);
+    assert(cstate != GNRC_IPV6_NIB_NC_INFO_NUD_STATE_REACHABLE);
+    _nib_onl_entry_t *node = _nib_onl_alloc(addr, iface);
+    if (node != NULL) {


I would prefer

if (node == NULL) { return _cache_out_onl_entry(addr, iface, cstate);; }

to remove the indentation below. Leads to a much better readability of the code.

miri64 · 2017-06-09T08:43:39Z

I wonder if we could merge the default router list with the off-link entry list.
I understand that separation has the advance of saving ram for :: and a prefix length information.
But maintaining the individual list with special member structs requires its own access and modification functions which rises code and the flash image size.

The members of the default router entry struct would still have to be required be added to the off-link-router entry struct, so we then would also waste memory there (since most of them would only be required for default routers). Since the default router list maintenance functions are one of the least complex I also think that the ROM salvage we get from this isn't as big as the RAM salvage we get (percent-wise). As soon as the whole NIB is in place however we can experiment with that. That's the nice thing about levels of abstraction :-).

miri64 · 2017-06-09T08:48:44Z

Since the default router list maintenance functions are one of the least complex I also think that the ROM salvage we get from this isn't as big as the RAM salvage we get (percent-wise)

Just consider, that for a typical 6LN (where we need to save the most memory) we can assume, that it only has one off-link-entry (a prefix in its prefix list) and one default router, so by joining those to data structure we would double the required RAM, because they have completely orthogonally required fields (disregarding prefix, which we are currently saving by implicitly having it :: for the default router).

cgundogan · 2017-06-09T08:54:27Z

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

+{
+    /* if there isn't already a default router selected or
+     * its reachability is suspect */
+    if ((_prime_def_router == NULL) ||


Can you also change this to an early exit? This function has 3 levels of indentation ..

cgundogan · 2017-06-09T10:05:51Z

tests/unittests/tests-gnrc_ipv6_nib/tests-gnrc_ipv6_nib-internal.c

+    TEST_ASSERT_NULL(_nib_drl_iter(NULL));
+}
+
+


please remove empty line

cgundogan · 2017-06-09T10:07:55Z

tests/unittests/tests-gnrc_ipv6_nib/tests-gnrc_ipv6_nib-internal.c

+    TEST_ASSERT(nib_res == node);
+    TEST_ASSERT_NOT_NULL((nib_res = _nib_drl_get_dr()));
+    TEST_ASSERT(nib_res == node);
+    TEST_ASSERT_NOT_NULL((nib_res = _nib_drl_get_dr()));


testing nib_res == node 2 times is enough ..

cgundogan · 2017-06-09T10:09:42Z

tests/unittests/tests-gnrc_ipv6_nib/tests-gnrc_ipv6_nib-internal.c

+}
+
+/*
+ * Tries to get the default router from a list of two routers


tests with two routers are obsolete when you are testing the permutation of three routers below ..

miri64 · 2017-06-09T10:12:39Z

Addressed latest batch of comments on tests

cgundogan · 2017-06-09T10:15:16Z

ACK from @BytesGalore and me ! please squash.

miri64 · 2017-06-09T11:24:49Z

Squashed and rebased

cgundogan · 2017-06-09T12:38:15Z

please address murdock's findings.

miri64 · 2017-06-09T13:20:44Z

Already on it :-)

miri64 · 2017-06-09T13:44:59Z

Fixed (but not squashed in case you want to review those changes, because they are quite largish)

…S#6988

cgundogan · 2017-06-09T19:49:56Z

The doc fixes look good. Please squash and then let's see what murdock has to say. It's getting real now (:

miri64 · 2017-06-09T19:53:02Z

Squashed

miri64 · 2017-06-09T20:02:00Z

Murdock likes it :-)

miri64 added GNRC Area: network Area: Networking Type: new feature The issue requests / The PR implemements a new feature for RIOT labels Jan 11, 2017

miri64 added this to the Release 2017.01 milestone Jan 11, 2017

miri64 assigned OlegHahm Jan 11, 2017

miri64 requested a review from OlegHahm January 11, 2017 11:23

cgundogan reviewed Jan 11, 2017

View reviewed changes

This was referenced Jan 16, 2017

gnrc_ipv6_nib: Initial API for an IPv6 specific FIB/NIB #5866

Closed

gnrc_ndp: add adapters for NIB-based implementation #6380

Closed

PeterKietzmann modified the milestones: Release 2017.01, Release 2017.04 Jan 26, 2017

miri64 force-pushed the gnrc_ipv6_nib/feat/internal branch from f47e419 to 381f5c3 Compare March 20, 2017 10:28

kaspar030 modified the milestone: Release 2017.04 Apr 21, 2017

miri64 force-pushed the gnrc_ipv6_nib/feat/internal branch from 381f5c3 to 9c3a642 Compare April 25, 2017 14:55

miri64 mentioned this pull request Apr 26, 2017

evtimer: initial import #6767

Merged

miri64 force-pushed the gnrc_ipv6_nib/feat/internal branch 2 times, most recently from 2cdfc2b to bc182a5 Compare April 28, 2017 15:23

miri64 mentioned this pull request May 1, 2017

gnrc_ipv6_nib: add neighbor cache component #6988

Merged

miri64 added a commit to miri64/RIOT that referenced this pull request May 1, 2017

Merge RIOT-OS#6325 into RIOT-OS#6988

eb2806a

miri64 added the CI: needs squashing Commits in this PR need to be squashed; If set, CI systems will mark this PR as unmergable label May 1, 2017

miri64 added a commit to miri64/RIOT that referenced this pull request May 2, 2017

Merge RIOT-OS#6325 updates

bd5c12f

cgundogan reviewed Jun 9, 2017

View reviewed changes

miri64 force-pushed the gnrc_ipv6_nib/feat/internal branch from 65033f0 to 22ec0c1 Compare June 9, 2017 11:24

miri64 added CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR and removed CI: needs squashing Commits in this PR need to be squashed; If set, CI systems will mark this PR as unmergable labels Jun 9, 2017

cgundogan approved these changes Jun 9, 2017

View reviewed changes

miri64 added a commit to miri64/RIOT that referenced this pull request Jun 9, 2017

Merge branch 'gnrc_ipv6_nib/feat/internal' (RIOT-OS#6325) into RIOT-O…

b0d384a

…S#6988

miri64 added 2 commits June 9, 2017 21:52

gnrc_ipv6_nib: initial import of internal NIB functions

e92f97c

tests: provide unittests for internal NIB functions

0bf52f0

miri64 force-pushed the gnrc_ipv6_nib/feat/internal branch from c4a5fd5 to 0bf52f0 Compare June 9, 2017 19:52

cgundogan merged commit 72c3c9c into RIOT-OS:master Jun 9, 2017

miri64 deleted the gnrc_ipv6_nib/feat/internal branch June 9, 2017 20:10

miri64 mentioned this pull request Jun 21, 2017

gnrc_rpl should populate the neighbor cache #5025

Closed

aabadie modified the milestone: Release 2017.07 Jun 26, 2017

Conversation

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

Uh oh!

Reasoning for this PR

Structure

Uh oh!

Choose a reason for hiding this comment

Uh oh!

miri64 Jan 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!

miri64 commented Jan 13, 2017

Uh oh!

miri64 commented Mar 20, 2017

Uh oh!

miri64 commented Apr 25, 2017

Uh oh!

miri64 commented Apr 26, 2017

Uh oh!

miri64 commented Apr 26, 2017

Uh oh!

miri64 commented Apr 28, 2017

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!

cgundogan Jun 9, 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 commented Jun 9, 2017

Uh oh!

miri64 commented Jun 9, 2017

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 commented Jun 9, 2017

Uh oh!

cgundogan commented Jun 9, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

miri64 commented Jun 9, 2017

Uh oh!

cgundogan commented Jun 9, 2017

Uh oh!

miri64 commented Jun 9, 2017

Uh oh!

miri64 commented Jun 9, 2017

Uh oh!

cgundogan commented Jun 9, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

miri64 commented Jun 9, 2017

Uh oh!

miri64 commented Jun 9, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

miri64 commented Jan 11, 2017 •

edited

Loading

miri64 Jan 11, 2017 •

edited

Loading

cgundogan Jun 9, 2017 •

edited

Loading

cgundogan commented Jun 9, 2017 •

edited

Loading

cgundogan commented Jun 9, 2017 •

edited

Loading