sys/net/network_layer added a core implementation of a FIB #2211

BytesGalore · 2014-12-17T13:07:37Z

This PR introduces a Forwarding Information Base (FIB) implementation.
It will replace the ip_get_next_hop() call on registered functions from routing protocols (RPs), e.g. here.

It provides management functions that must be used by RPs to add, delete and update next hop entries.
It signals reactive routing protocols whenever the FIB does not provide a next hop towards a destination to start a route discovery.
Underneath it provides a universal address container that allows to use one added entry multiple times.
The FIB is completely event triggered (no threads used)
All access is mutually excluded to allow being operated from asynchronous sources.

miri64 · 2014-12-17T13:15:03Z

Wouldn't sys/net/routing be a more fitting directory?

miri64 · 2014-12-17T13:16:04Z

sys/net/include/fib/fib.h

+ * @return FIB_SUCCESS on success
+ *         FIB_ERROR if the entry cannot be created due to unsufficient RAM
+ */
+int fib_table_add_entry(uint8_t iface_id, uint8_t *glob_dst, size_t glob_dst_size, uint8_t *next_hop, size_t next_hop_size, uint32_t lifetime);


Didn't we talk about this previously, to use kernel_pid_t as type for iface_id?

Yeah it was an attempt to save a byte for each entry.
I change it :)

BytesGalore · 2014-12-17T13:24:23Z

Wouldn't sys/net/routing be a more fitting directory?

Hmm don't know, its more forwarding than routing

haukepetersen · 2015-02-11T20:54:06Z

actually I think network layer is the correct place for this module.

miri64 · 2015-02-11T21:05:02Z

I think it's pretty arbitrary, since the network layer uses it to find the next hop and routing (which is more or less part of the network layer) is filling it.

haukepetersen · 2015-02-12T13:21:04Z

@BytesGalore, @authmillenon: does it makes sense to convert the test application into a unit test?

BytesGalore · 2015-02-12T13:25:45Z

@haukepetersen definitely. I will migrate the tests this afternoon/tomorrow morning

haukepetersen · 2015-02-12T13:26:35Z

nice!

miri64 · 2015-02-13T12:25:29Z

While researching for IPv6's neighbor cache I stumbled upon the data structure Destination Cache which is pretty similar (or probably the same thing, just another term for) the FIB, except that it is suggested to store things like path MTU or round-trip timers. Do we want to add such things to the FIB

[…] the Destination Cache maps
a destination IP address to the IP address of the
next-hop neighbor.  This cache is updated with
information learned from Redirect messages.
Implementations may find it convenient to store
additional information not directly related to
Neighbor Discovery in Destination Cache entries,
such as the Path MTU (PMTU) and round-trip timers
maintained by transport protocols.

OlegHahm · 2015-02-13T13:46:30Z

No. Keep the FIB as simple as possible. And from what I read this destination cache is quite different from a FIB (e.g., it includes off-link destinations, too).

BytesGalore · 2015-02-13T13:51:31Z

Usually I wouldn't expect to ask the FIB for the PMTU, or other path specific information directly.
The FIB is, for my understanding, only enquired to get the next-hop information for a given destination, i.e. next-hop IP and the out interface.
So an individual FIB entry must either provide this information directly to the requester, or for instance the PMTU must be obtained from somewhere else.
FIB Implementations such as in Linux mixes/shares this information between layers [1], and provides large linked data structures with diverse information used to speed up lookups and keep the information close together.
The question is if providing additional information per entry is reasonable for us.
For instance I wouldn't expect that the PMTU changes in 802.15.4 only networks per next-hop.
Also I wouldn't want to rely on measuring/using RTT in LLNs.

But this is just my individual opinion and does not take IPv6 capabilities of RIOT into account.

[1] https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/include/net/ip6_fib.h

OlegHahm · 2015-02-13T13:53:06Z

I fully agree with @BytesGalore here.

miri64 · 2015-03-06T18:11:08Z

If you rebase, I start reviewing this, since I come to the point, were I need it in IPv6 :D

miri64 · 2015-03-06T18:14:09Z

sys/Makefile.include

@@ -73,6 +73,10 @@ ifneq (,$(filter oneway_malloc,$(USEMODULE)))
    USEMODULE_INCLUDES += $(RIOTBASE)/sys/oneway-malloc/include
 endif

+ifneq (,$(filter fib,$(USEMODULE)))
+    USEMODULE_INCLUDES += $(RIOTBASE)/sys/net/include


move fib/fib.h to sys/include/net/ng_fib.h and all its sub-headers to sys/include/net/ng_fib/ and remove this line to make this part of NTSF.

miri64 · 2015-03-31T18:16:40Z

sys/include/net/ng_fib/ng_universal_address.h

+#define UNIVERSAL_ADDRESS_SIZE (16)         /**< size of the used addresses in bytes        */
+#define UNIVERSAL_ADDRESS_SUCCESS (0)       /**< return value for success                   */
+#define UNIVERSAL_ADDRESS_ERROR_MEMORY (-1) /**< error value for invalid output buffer size */
+#define UNIVERSAL_ADDRESS_NO_HIT (-2)       /**< value for a address compare failed         */


ok changed that

miri64 · 2015-03-31T18:38:18Z

Apart from my issue with universal_address_compare() in particular and custom return value defines in general I'm fine with this PR.

miri64 · 2015-03-31T19:44:03Z

sys/include/net/ng_fib/ng_universal_address.h

+ *         NULL if the size is unsufficient for copy
+ */
+uint8_t* universal_address_get_address(universal_address_container_t *entry,
+                                  uint8_t *addr, size_t *addr_size);


indentation

miri64 · 2015-03-31T20:45:59Z

So you did not consider to move ng_fib.h directly to sys/include/net? But apart from that (which would be nice, because of consistency and an include always looking like #include "net/ng_fib/ng_fib.h" instead of #include "net/ng_fib.h", but not necessary). ACK for my part. Please squash.

BytesGalore · 2015-04-01T05:38:46Z

ah sorry I did that but forgot to commit the changes 😊

BytesGalore · 2015-04-01T06:08:43Z

squashed

miri64 · 2015-04-01T07:15:01Z

ACK when Travis is happy :)

BytesGalore · 2015-04-01T07:51:20Z

To make travis happy I need to suppress a cppcheck warning due to a buffer I reuse in a unittest twice.
When travis is happy I will squash again.

miri64 · 2015-04-01T07:56:20Z

tests/unittests/tests-fib/tests-fib.c

+    memset(addr_nxt, 0, add_buf_size);
+    memset(addr_lookup, 0, add_buf_size);
+
+    /* cppcheck: addr_lookup is only passed but not required to be read */


s# \*/$#since we test prefix matching \*/#?

miri64 · 2015-04-01T08:14:22Z

Re-ACK, please squash.

BytesGalore · 2015-04-01T08:18:48Z

re-squashed

miri64 · 2015-04-01T08:20:17Z

Waiting for Travis…

miri64 · 2015-04-01T08:25:26Z

tests/unittests/tests-fib/tests-fib.c

+    memset(addr_nxt, 0, add_buf_size);
+    memset(addr_lookup, 0, add_buf_size);
+
+    /* cppcheck: addr_lookup is only passed but not required to be read, 


s/ \+$// ;-)

done and squashed since the change will not destroy anything (hopefully)

(please squash immediately to speed things up ;-))

There is still a whitespace at the end.

I think github tricks us, inspecting the file in raw shows the whitespace is gone :)

You're right... Travis just passed this test.

miri64 · 2015-04-01T09:40:18Z

And go \o/

sys/net/network_layer added a core implementation of a FIB

BytesGalore · 2015-04-01T09:41:23Z

Woohooo 😄
@authmillenon thx for reviewing

BytesGalore added Discussion: RFC The issue/PR is used as a discussion starting point about the item of the issue/PR Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation Area: network Area: Networking labels Dec 17, 2014

miri64 reviewed Dec 17, 2014
View reviewed changes

BytesGalore force-pushed the add_fib branch from affa76d to 1de5d68 Compare January 5, 2015 07:48

OlegHahm added the Community: Hack'n'ACK candidate This PR is a candidate for review and discussion during one of RIOT's monthly Hack'n'ACK parties label Jan 6, 2015

miri64 mentioned this pull request Jan 12, 2015

Network stack refactoring: TODO list #2278

Closed

36 tasks

BytesGalore force-pushed the add_fib branch 2 times, most recently from 873c9bb to e7b924f Compare January 19, 2015 12:02

miri64 modified the milestones: Release NEXT MAJOR, Network Stack Task Force Feb 11, 2015

miri64 added the NSTF label Feb 11, 2015

miri64 assigned haukepetersen Feb 11, 2015

BytesGalore force-pushed the add_fib branch from e08fa2f to 4d55844 Compare February 13, 2015 06:27

miri64 reviewed Mar 6, 2015
View reviewed changes

miri64 reviewed Mar 31, 2015
View reviewed changes

BytesGalore force-pushed the add_fib branch from 27944a4 to af787a2 Compare April 1, 2015 06:08

BytesGalore removed the CI: needs squashing Commits in this PR need to be squashed; If set, CI systems will mark this PR as unmergable label Apr 1, 2015

miri64 reviewed Apr 1, 2015
View reviewed changes

BytesGalore force-pushed the add_fib branch from 4fb1582 to d46c637 Compare April 1, 2015 08:18

net: added a core implementation of a FIB

30c76ad

BytesGalore force-pushed the add_fib branch from d46c637 to 30c76ad Compare April 1, 2015 08:25

miri64 reviewed Apr 1, 2015
View reviewed changes

miri64 added a commit that referenced this pull request Apr 1, 2015

Merge pull request #2211 from BytesGalore/add_fib

9372d95

sys/net/network_layer added a core implementation of a FIB

miri64 merged commit 9372d95 into RIOT-OS:master Apr 1, 2015

BytesGalore deleted the add_fib branch April 27, 2015 06:09

sys/net/network_layer added a core implementation of a FIB #2211

sys/net/network_layer added a core implementation of a FIB #2211

Conversation

BytesGalore commented Dec 17, 2014

miri64 commented Dec 17, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BytesGalore commented Dec 17, 2014

haukepetersen commented Feb 11, 2015

miri64 commented Feb 11, 2015

haukepetersen commented Feb 12, 2015

BytesGalore commented Feb 12, 2015

haukepetersen commented Feb 12, 2015

miri64 commented Feb 13, 2015

OlegHahm commented Feb 13, 2015

BytesGalore commented Feb 13, 2015

OlegHahm commented Feb 13, 2015

miri64 commented Mar 6, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

miri64 commented Mar 31, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

miri64 commented Mar 31, 2015

BytesGalore commented Apr 1, 2015

BytesGalore commented Apr 1, 2015

miri64 commented Apr 1, 2015

BytesGalore commented Apr 1, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

miri64 commented Apr 1, 2015

BytesGalore commented Apr 1, 2015

miri64 commented Apr 1, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

miri64 commented Apr 1, 2015

BytesGalore commented Apr 1, 2015