Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

routing/rpl: adjusted RPL to use the FIB #2765

Closed
wants to merge 6 commits into from

Conversation

BytesGalore
Copy link
Member

Formerly the RPL implementation used internal routing and forwarding tables.
This PR changes it to use the new FIB instead of providing a get_next_hop_provider() to the current fading sixlowpan/ip implementation.
The provider registration and usage is replaced by calling the FIB for a next-hop directly.

@BytesGalore BytesGalore added Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation Area: network Area: Networking Discussion: RFC The issue/PR is used as a discussion starting point about the item of the issue/PR NSTF labels Apr 8, 2015
/**
* @brief receive the set inteface id
*
* @return the set interface id of KERNEL_PID_UNDEF if no interface has been set
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

s/of/or

@cgundogan
Copy link
Member

+137 −294. This is nice (: Need to do more testing and reviewing, though.

@@ -33,6 +33,7 @@
#include "lowpan.h"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does it also make sense to outsource the changes to this file to its own PR? Doesn't seem like that there are any dependencies to your RPL refactorings?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Basically yes, but then no routing protocol will work in mainline RIOT until we merge this PR or adjust the other protocols.

@miri64 miri64 mentioned this pull request Apr 9, 2015
36 tasks
@BytesGalore BytesGalore added CI: needs squashing Commits in this PR need to be squashed; If set, CI systems will mark this PR as unmergable State: waiting for other PR State: The PR requires another PR to be merged first labels Apr 10, 2015
@BytesGalore
Copy link
Member Author

does not depend, but would make sense to merge after #2782 has been merged, since this "compatibility" commit can be removed then.

@@ -279,32 +273,16 @@ void *rpl_process(void *arg)
void _rpl_update_routing_table(void)
{
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since FIB handles the lifetime of routes, would it make sense to remove this _rpl_update_routing_table thread completely and just pass a function callback (EDIT: or another kind of signalling) to the fib, which should be executed when a certain condition holds? (e.g. route lifetime expired) @BytesGalore @OlegHahm

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@cgundogan the FIB works completely without threads. The exact invalidation point in time of a route is fixed and known, but not monitored by the FIB. A route lifetime and its invalidation is only performed on demand when the specific FIB entry is touched.
So if the lifetime of a route exceeds, probably the parent invalidation will not be triggered.

@cgundogan
Copy link
Member

I had some warnings compiling your branch regarding return values in sc_fib.c. But I realized that this is fixed in master. A rebase would be appreciated (:

@BytesGalore
Copy link
Member Author

rebased

@cgundogan
Copy link
Member

@BytesGalore did you rebase to current master? I still get the warning regarding sc_fib.c and rpl_send_opt_target_buf->target still comprises a link-local address.

@BytesGalore
Copy link
Member Author

@cgundogan yes the commits are ontop of this merge [1] 19 hours ago

[1] BytesGalore@1af6126

@cgundogan
Copy link
Member

@BytesGalore could you rebase again please? (: This PR conflicts now with the merged #2607

@BytesGalore
Copy link
Member Author

sure, done :)

rpl_send_opt_target_buf->type = RPL_OPT_TARGET;
rpl_send_opt_target_buf->length = RPL_OPT_TARGET_LEN;
rpl_send_opt_target_buf->flags = 0x00;
rpl_send_opt_target_buf->prefix_length = RPL_DODAG_ID_LEN;
memcpy(&rpl_send_opt_target_buf->target, &rpl_get_routing_table()[i].address,
memcpy(&rpl_send_opt_target_buf->target, &rpl_parents[i].addr,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Taking the address from the parent results in these link-local addresses we see. In the former implementation rpl_get_routing_table()[i].address returned global addresses. I think the proper way would be to consult the fib and get the global destination address for those link-local addresses, and skip all parents without an entry in the fib.

EDIT: Or simply iterate over all entries in the fib istead of the parents, like in the implementation before

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unfortunately its not possible that the FIB replies a global address for a given next-hop address, since a next-hop address can be easily used for a large number of destinations.
Iterating is also not possible due to the separation concept of the FIB. But even if iterating would be possible, the FIB can have more entries than just the next-hops from RPL parents.

Fortunately not all is lost :D we could keep the global address for a parent in rpl_parents[i].addr instead of the local one, and ask the FIB for the global address when needed.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On second thought, iterating over the parents to fill the DAO is not the right way, because DAOs accumulate the routes they encounter up the dodag to the root. By iterating over the parents we can only put their addresses in the DAO but not the addresses the node accumulated through other DAOs (from children). In my opinion there should be a way to iterate over all targets in the fib and write them into the DAO.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This way we loose the downward route information for sub-DODAGS right?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I fear so, yes. How could we approach this issue? Is it possible to implement something like a generic iterator for the fib?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have to think a bit more on it.
Just to clarify for myself, what we need to know here are all destinations addresses reachable from this node, which are constructed (storing MOP assumed) by receiving DIOs and all target options from all received DAOs right?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Correct, a DAO includes the address of the sending node in addition to all addresses it received from its children's DAOs, recursively. (storing MOP)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@cgundogan enhancing the data structures in the FIB or using the type field (uint32_t) for additional indication is no problem.
Maybe (and very related to your proposition), and this is also just thinking out loud, the FIB could return a full set of next-hop/destination tuples based on matching type field or just specific bits of it.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe something like this:

typedef struct entries{
    ng_ipv6_addr_t dst;
    ng_ipv6_addr_t next_hop;
};

/**
* param[in]  bitmask indicate the bits to use in the search
* param[in]  typevalue the query type
* param[out] arr_entries array to store matching entries
* param[in, out] arr_entries_size indicates the storage spce on in and the use count when returned
*/
void fib_get_entry_set(uint32_t bitmask, uint32_t typevalue, entries* arr_entries, size_t* arr_entries_size);

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@cgundogan I've opened #2818 as proposition for a function that could help here

@jnohlgard
Copy link
Member

needs rebase

@BytesGalore
Copy link
Member Author

rebased.

@BytesGalore
Copy link
Member Author

Anyone interested to keep this PR?

@cgundogan
Copy link
Member

@BytesGalore not really (: I will close it as this references the former rpl implementation and the new ng_rpl implementation has already the fib included.

@cgundogan cgundogan closed this Aug 17, 2015
@BytesGalore BytesGalore deleted the rpl_and_fib branch November 2, 2015 07:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: network Area: Networking CI: needs squashing Commits in this PR need to be squashed; If set, CI systems will mark this PR as unmergable Discussion: RFC The issue/PR is used as a discussion starting point about the item of the issue/PR State: waiting for other PR State: The PR requires another PR to be merged first Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants