ipv6/rpl: add hop-by-hop extension handling by BytesGalore · Pull Request #7231 · RIOT-OS/RIOT

BytesGalore · 2017-06-22T14:59:28Z

This PR introduces hop-by-hop header handling for RPL,
which includes an adjustment to extension header processing in gnrc_ipv6.

For receiving packets the adjustment introduces a sequential procedure which enables all interested threads to evaluate an extension and react before the packet is forwarded further.
When sending a packet all interested threads will be called to insert their extension before the packet is sent.

sys/include/net/gnrc/rpl/hop.h

miri64 · 2017-06-23T20:48:37Z

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

+    bool encapsulate = false;
+
+    for ( size_t i = 0; i < sizeof(ext_demux); ++i ) {
+        gnrc_netreg_entry_t *call = gnrc_netreg_lookup(GNRC_NETAPI_MSG_TYPE_SND, ext_demux[i]);


GNRC_NETAPI_MSG_TYPE_SND isn't a NETTYPE

Hm, seems I didn't pushed it. It should be GNRC_NETTYPE_IPV6

Even then this doesn't seem to make sense, since you are not sending here but building headers. Why not just use functions or if you absolutely have to use IPC (which I think would slow-down the sending process majorly), use raw IPC?

I don't like the fact that IPv6 needs to know something about, say RPL.
Additionally it would require some handshaking which is not needed by using IPC.

use raw IPC

I do, I iterate all interested PIDs with gnrc_netreg_lookup() and use plain message passing here.

miri64 · 2017-06-23T20:49:55Z

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

+                encapsulate = true;
+            }
+            msg.content.ptr = pkt;
+            msg_send_receive(&msg, &rcv, call->target.pid);


I'm not sure if you use NETAPI_SND here, because above you use NETAPI_SND in a weird context, but NETAPI_SND uses asynchronous API, so this messes up things.

The idea here is to consecutively send a blocking message to allow each interested thread to add their extension header to the IPv6 packet and inform the IPv6 thread back when its done.
This way all extension headers can be inserted in a single packet.

Trying this asynchronously would result in sending a multitude of copies of the packet, since all threads do a copy-on-write operation to the packet when inserting an extension header.

That's why I stated below, that this is a misuse of NETAPI_SND. You are not sending here, you are building headers, so I think a callback function to the header builders would be more sensible here.

miri64 · 2017-06-23T20:50:31Z

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

+            gnrc_pktsnip_t* tmp = pkt;
+            pkt = gnrc_ipv6_hdr_build(pkt, &hdr->src, &hdr->dst);
+            if (pkt == NULL) {
+                /* revert but what do we now */


abort sending, I would say.

yup, makes sense.

miri64 · 2017-06-23T20:53:04Z

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

                msg_reply(&msg, &reply);
                break;
-
+            case GNRC_IPV6_EXT_HANDLE_NEXT_HDR:


Why not do all below in gnrc_ipv6_ext_demux()?

This is a direct reply of a thread using plain msg passing when parsing an extension from a received IPv6 packet is finished.
The *handle is used as an iteration message by the thread to point to the next header to be processed, which is eventually dispatched in gnrc_ipv6_ext_demux().
The IPv6 thread needs to wait forwarding the IPv6 packet further until all extensions has been processed by all interested threads.

Ok, but in this case please surround the case with a suitable #ifdef MODULE_...

miri64 · 2017-06-23T20:53:45Z

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

-            _send(reversed_pkt, false);
-            return;
-        }
-        else {


We must prevent to forward the packet before all extension headers have been processed.
Currently I moved the responsibility for preparing the packet as above to the thread that processed the last extension header.
But as you stated, the IPv6 thread is the responsible one for doing it and we should keep it that way.

I migrated the responsibility back to IPv6 [1].
The actual forwarding of the packet is now performed when the demultiplexing is finished [2].

[1] https://github.com/RIOT-OS/RIOT/pull/7231/files#diff-74976d45a42eaa063e3561b5fd36f615R146
[2] https://github.com/RIOT-OS/RIOT/pull/7231/files#diff-74976d45a42eaa063e3561b5fd36f615R255

miri64 · 2017-06-23T20:59:23Z

This whole PR seems very intrusive into gnrc_ipv6 just to handle hop-by-hop headers and additionally seems to strongly misuse GNRC's API for things that can be done in a more straight forward way.

BytesGalore · 2017-06-26T07:38:49Z

just to handle hop-by-hop headers

I disagree,
it allows to process any extension header by other threads and it uses the already present information and mechanics form netreg for interest registration on extension header and their dispatching.
It is not bound to RPL or Hop-by-Hop extensions and such mechanism is required for any cross-layer extension header handling, e.g. RPL source routing headers.

miri64 · 2017-06-26T08:02:00Z

I disagree,
it allows to process any extension header by other threads and it uses the already present information and mechanics form netreg for interest registration on extension header and their dispatching.
It is not bound to RPL or Hop-by-Hop extensions and such mechanism is required for any cross-layer extension header handling, e.g. RPL source routing headers.

But it changes the semantics of that mechanics and their API significantly, which can have unforeseen side-effects and makes potential refactoring work harder. I also don't think that handling IPv6 extension headers in any other thread than the IPv6 handling thread is a good idea. Every synchronous context switch demands time and message queue space (thus blocking the queue for other packages). I'm actually currently trying to reduce those within the stack. If you absolutely want to use netreg for this (but maybe use NETAPI_GET for it with a new NETOPT_EXT_HDR option) consider using the netreg callback type (and extending it for get/set, as it currently only works with snd/rcv) instead of the default type.

BytesGalore · 2017-06-28T07:01:05Z

But it changes the semantics of that mechanics and their API significantly

I don't see your point.
The changes introduced by this PR extend the eventloop to recognize one new message type and doesn't change anything in the API semantics.
It introduces a sequentially ordered processing of extension headers in registered contexts, in which demuxing an extension header passes the responsibility to the informed thread which in turn passes pktsnip and the responsibility back here when it finished processing the header.

Using the netreg callback module as you proposed is an interesting feature, but it would bypass the regular processing order of the called context which could result in unforeseen states.

Could you rephrase your proposition for using NETAPI_GET/SET.
If I understand right, you would propose to use it to replace the direct IPC calls, right?

miri64 · 2017-06-28T09:31:24Z

I was talking about this function, that used to have a NETAPI_SND in it to make synchronous (msg_send_receive()) IPC calls. I now see that this changed and you use the EXT_TYPE as message type. I still think however, that it might be more elegant to use NETAPI_GET/SET. So

Could you rephrase your proposition for using NETAPI_GET/SET.
If I understand right, you would propose to use it to replace the direct IPC calls, right?

Yes.

miri64 · 2017-06-28T12:56:19Z

sys/include/net/netopt.h

+    /**
+     * @brief   Type for sequiential processing of IPv6 extension headers
+     *
+     * On GET it requests a thread to insert an extension header.


Why does get not just return a gnrc_pktsnip_t and the IPv6 thread inserts the header?

(then it still could be used like the other get/set calls ;-)).

ok, I gave it a shot

miri64 · 2017-06-28T13:51:50Z

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

+                     * i.e. in front of hdr->next.
+                     * We bend hdr->next to point to ext.
+                     */
+                    pkt->next = data;


Don't you need to append the old payload of pkt as well?

Ah got it. The handler already does that for you.

It depends how the pktsnip is created.
When we create it with the pkt prepending to pkt->next I would expect that the former content is appended behind the newly created snip.
e.g. gnrc_pktbuf_add(pkt->next, ....);

miri64 · 2017-06-28T13:55:09Z

sys/include/net/netopt.h

+    /**
+     * @brief   Type for sequiential processing of IPv6 extension headers
+     *
+     * On GET it requests a thread to provide an extension header.


~~"and appends it to the the given (IPv6) header"~~ "and prepends it to the given payload of the given (IPv6) packet"

miri64 · 2017-06-28T13:59:30Z

sys/include/net/netopt.h

+     *
+     * When the header is done the thread replies to the call.
+     * The result is the pktsnip_t with the extension header
+     * The result is NULL if no extension header is passed.


You hand over a pointer (not a pointer of a pointer) below, so this should be impossible. Why not use the numeric return-value of NETAPI_GET (which is returned by gnrc_netapi_get())?, e.g. sizeof(gnrc_pktsnip_t) on success, -ENOMEM (or whatever is applicable) on error.

not a pointer of a pointer

true, it would require some wrapping.
You mean exploiting the int return type for pointer passing, why not.

stale · 2020-01-31T16:05:46Z

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. If you want me to ignore this issue, please mark it with the "State: don't stale" label. Thank you for your contributions.

smlng · 2020-02-03T07:18:39Z

ping @BytesGalore, should someone take over?

miri64 · 2020-02-03T11:41:41Z

From what I can tell, parts of RFC ~~6550~~ 6553 are slated to be deprecated by https://datatracker.ietf.org/doc/draft-ietf-roll-useofrplinfo/ (mostly the type field gets a rework). I can take this over, but more on the side, so adapting to current master and the new draft might take a bit.

tcschmidt · 2020-04-01T01:14:13Z

@miri64 don't see any problems taking this over ...

miri64 · 2020-04-01T10:24:06Z

@miri64 don't see any problems taking this over ...

Already in progress. First step would be to merge #13275 ;-).

miri64 · 2020-06-29T16:47:12Z

Started migration in https://github.com/miri64/RIOT/tree/gnrc_rpl_opt/feat/initial.

tinstructor · 2020-10-15T12:18:18Z

From what I can tell, parts of RFC ~~6550~~ 6553 are slated to be deprecated by https://datatracker.ietf.org/doc/draft-ietf-roll-useofrplinfo/ (mostly the type field gets a rework). I can take this over, but more on the side, so adapting to current master and the new draft might take a bit.

@miri64 They might be deprecated yes. However, nobody will use the RPI option uncompressed anyway when RIOT finally implements the 6LoRH specified by RFC 8138. However, I don't think anybody is working on implementing 6LoRH currently, is there?

miri64 · 2020-10-20T08:40:01Z

However, I don't think anybody is working on implementing 6LoRH currently, is there?

AFAIK, no, but we need the option and support for the paging dispatch first, right?

tinstructor · 2020-10-26T13:12:10Z

@miri64 correct, paging dispatch is needed first

stale · 2022-04-17T06:02:27Z

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. If you want me to ignore this issue, please mark it with the "State: don't stale" label. Thank you for your contributions.

mguetschow · 2024-06-11T09:14:08Z

Since this PR has been stale for almost four years, but still contains useful code and discussion for follow-up, I'll convert it to a draft.

Please feel free to remove the draft state if anyone wants to pick this up again.

miri64 · 2026-01-27T17:14:52Z

Maybe could be interesting for @elenaf9?

BytesGalore added Area: network Area: Networking Type: new feature The issue requests / The PR implemements a new feature for RIOT State: WIP State: The PR is still work-in-progress and its code is not in its final presentable form yet labels Jun 22, 2017

BytesGalore assigned cgundogan Jun 22, 2017

BytesGalore force-pushed the rpl_add_data_plane_extension_header_handling branch 4 times, most recently from 22acdac to e61fb52 Compare June 23, 2017 06:23

BytesGalore added the Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation label Jun 23, 2017

BytesGalore mentioned this pull request Jun 23, 2017

RPL: add hop-by-hop header RPL option #3399

Closed

miri64 reviewed Jun 23, 2017

View reviewed changes

sys/include/net/gnrc/rpl/hop.h Show resolved Hide resolved

miri64 requested changes Jun 23, 2017

View reviewed changes

aabadie modified the milestone: Release 2017.07 Jun 26, 2017

miri64 reviewed Jun 28, 2017

View reviewed changes

miri64 requested changes Jun 28, 2017

View reviewed changes

BytesGalore force-pushed the rpl_add_data_plane_extension_header_handling branch 4 times, most recently from 20dab9b to 7326be9 Compare June 30, 2017 14:34

aabadie modified the milestones: Release 2017.07, Release 2017.10 Jun 30, 2017

miri64 self-assigned this Jul 30, 2019

stale bot added the State: stale State: The issue / PR has no activity for >185 days label Jan 31, 2020

smlng added State: don't stale State: Tell state-bot to ignore this issue and removed State: stale State: The issue / PR has no activity for >185 days labels Feb 3, 2020

miri64 mentioned this pull request Feb 3, 2020

gnrc_ipv6_ext_opt: initial import #13275

Merged

miri64 mentioned this pull request Oct 7, 2020

gnrc/rpl: document implementation status #15155

Merged

MrKevinWeiss added this to the Release 2021.07 milestone Jun 21, 2021

MrKevinWeiss removed this from the Release 2021.07 milestone Jul 15, 2021

benpicco removed the State: don't stale State: Tell state-bot to ignore this issue label Oct 13, 2021

stale bot added the State: stale State: The issue / PR has no activity for >185 days label Apr 17, 2022

stale bot closed this Jun 12, 2022

miri64 removed the State: stale State: The issue / PR has no activity for >185 days label Jun 16, 2022

miri64 reopened this Jun 21, 2022

github-actions bot added Area: build system Area: Build system Area: examples Area: Example Applications Area: sys Area: System labels Jun 21, 2022

mguetschow marked this pull request as draft June 11, 2024 09:14

crasbe added the State: needs rebase State: The codebase was changed since the creation of the PR, making a rebase necessary label Jan 28, 2026

Conversation

BytesGalore commented Jun 22, 2017

Uh oh!

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!

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

Uh oh!

BytesGalore commented Jun 26, 2017

Uh oh!

miri64 commented Jun 26, 2017

Uh oh!

BytesGalore commented Jun 28, 2017

Uh oh!

miri64 commented Jun 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

miri64 Jun 28, 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!

stale bot commented Jan 31, 2020

Uh oh!

smlng commented Feb 3, 2020

Uh oh!

miri64 commented Feb 3, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tcschmidt commented Apr 1, 2020

Uh oh!

miri64 commented Apr 1, 2020

Uh oh!

miri64 commented Jun 29, 2020

Uh oh!

tinstructor commented Oct 15, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

miri64 Jun 28, 2017 •

edited

Loading

miri64 commented Feb 3, 2020 •

edited

Loading

tinstructor commented Oct 15, 2020 •

edited

Loading