netdev: Initial implementation of a more layered approach to netdev #8198

bergzand · 2017-12-03T21:20:19Z

First approach to have a more layered netdev (#7736). For now I have rebuild the netdev_ieee802154 and netdev_eth files to a layered setup. the l2filter and netstats are not yet implemented in the layered approach and remain implemented as before.

UML

I've adjusted the UML diagram a bit to what I think gives more flexibility in the long run:

I've decided to extend netdev_t instead. This way the callback can also be propagated back through all layers (for example required for a software frame retransmission implementation, or L2 statistics). The context pointer from the netdev_t is reused as the pointer to the layer above. It was already used in this way, only now it points to the previous netdev_layer_t object and for the last layer it points back to the gnrc_netif_t object.

Current implementation allows upper layers to override functionality from lower layers. This could be useful in the case of the software retransmissions where the number of retransmissions implemented by the driver needs to be masked.

Data Allocation

Data is allocated on the stack of the netif thread. I've added a function pointer to the gnrc_netif_create function. This function is first called and is expected to call gnrc_netif_thread. This bootstrap function can push all necessary structs required for the intermediate layer on the stack, add them as layers, add a reference to the top layer to the gnrc_netif_t object and call the gnrc_netif_thread function to start the "real" thread part.
This is the easiest way to add these netdev_layered_t objects in a semi-dynamic (non-configurable) way. I don't consider this the permanent way to allocate these structs, but as a PoC it should suffice.

Configuration

Todo

WIP

Receive feedback
Move L2Filter
Move L2 netstats

miri64 · 2017-12-03T21:24:54Z

I'd like to also get some feedback on this by @haukepetersen and @kaspar030.

jnohlgard · 2017-12-04T12:32:08Z

Why is there a need to distinguish between netdev_t and netdev_layer_t? By having them the same it will be interchangeable to add for example a software CSMA layer, or a duty cycling MAC layer on top of any device without the upper layer caring at all.
(continuing on the discussion opened by @kaspar030 in a different thread, but I can't find the link now...)

miri64 · 2017-12-04T12:35:56Z

Why is there a need to distinguish between netdev_t and netdev_layer_t? By having them the same it will be interchangeable to add for example a software CSMA layer, or a duty cycling MAC layer on top of any device without the upper layer caring at all.

From what I can see from the model (and the implementation): because netdev_layer_t has a next pointer and netdev_t doesn't (and this seems to be the only difference).

miri64 · 2017-12-04T12:37:11Z

Or do you mean, that it could have a benefit for physical devices to have next pointer?

jnohlgard · 2017-12-04T12:58:03Z

I meant why bother having two types to save on RAM for a single pointer? Most systems have one, max two physical interfaces, so it's only a matter of 2-8 bytes depending on platform.

miri64 · 2017-12-04T13:00:52Z

Makefile.dep

 ifneq (,$(filter gnrc_netdev_default,$(USEMODULE)))
  USEMODULE += netdev_default
  USEMODULE += gnrc_netif
 endif

-ifneq (,$(filter netdev_ieee802154,$(USEMODULE)))
+ifneq (,$(filter netdev_layer_ieee802154,$(USEMODULE)))


Independent from the separate type discussion @gebart started: I would argue that a rename of netdev_ieee802154 and netdev_eth isn't really necessary.

Yeah, I agree on that, I came to the same conclusion after I had to rename half of the ifdefs scattered through gnrc_netif and nib

Reverted "new" netdev_layer_(eth|ieee802154) files and reworked netdev_(eth|ieee802154) instead.

miri64 · 2017-12-04T16:05:30Z

I meant why bother having two types to save on RAM for a single pointer? Most systems have one, max two physical interfaces, so it's only a matter of 2-8 bytes depending on platform.

I don't really have an opinion on that, so whatever seems more suitable for the rest: go ahead with it.

miri64 · 2017-12-04T16:08:58Z

I meant why bother having two types to save on RAM for a single pointer? Most systems have one, max two physical interfaces, so it's only a matter of 2-8 bytes depending on platform.

I don't really have an opinion on that, so whatever seems more suitable for the rest: go ahead with it.

I would however prefer the pointer renamed to something other than next. next might imply for the uncareful reader knowledgeable in network architectures, that it refers to the next network device in a list of network devices and not being the next in a stack of filters. Maybe the name parent, @kaspar030 suggested in #7736 might be a better one, but I'm not sure either.

bergzand · 2017-12-04T16:31:32Z

I meant why bother having two types to save on RAM for a single pointer? Most systems have one, max two physical interfaces, so it's only a matter of 2-8 bytes depending on platform.

While I agree that those 2-8 bytes might not matter, I'm not yet sure whether there is a real advantage besides removing some casts to this. Main advantage I can think of now is that it gives a clear indication when the end of the stack is reached (parent == NULL) which allows for iteration over all layers in a predictable way. I'll give it a try this evening.

Maybe the name parent, @kaspar030 suggested in #7736 might be a better one, but I'm not sure either.

parent or child implies a hierarchical relation, I agree that it is a better name than next for this pointer.

jnohlgard · 2017-12-05T14:30:50Z

The more I think about this idea, the more I like it. With this approach it will be possible to insert arbitrary hooks into the packet processing, for example insert a new layer between the netdev and the network layer to add artificial delays or packet drops to allow testing of lossy links on the local computer.

jnohlgard · 2017-12-05T14:47:04Z

Oh, and MAC layers can be made network stack independent by writing them as netdev layers, like @kaspar030 suggested before. Big thumbs up from me for the general idea, I did not look at the implementation details yet.

bergzand · 2017-12-05T15:19:15Z

Oh, and MAC layers can be made network stack independent by writing them as netdev layers, like @kaspar030 suggested before. Big thumbs up from me for the general idea, I did not look at the implementation details yet.

To me, the main problem here is that with netdev, the frame is already in iovec format. I think it is easier to do packet manipulation and inspection while the packet is still handled as a chain of pktsnips.
This is also my current problem with the L2filter and the L2 netstats. Both these structs would easily translate to extra netdev layers, except that the packet needs to be parsed. This would result in different layers (or case statements in a single layer) depending on ethernet or ieee802154.

For now I think it is best to keep these layers to functionality that doesn't work on a specific set of frames, such as the artificial delay or random drop layers proposed by @gebart.

bergzand · 2017-12-05T20:31:36Z

I've removed the netdev_layer_t struct and dependency and added a pointer in the next layer in the netdev_t struct. For now I've decided on calling it child as it points to a layer down in the stack (from the first layer (parent) called until the layer controlling the hardware (child)). The "parent" pointer is recycled from the context pointer which was already used by gnrc_netif as a pointer back "up".

I would like to remove netdev_layer.c too, but I still need to look for a place to put those functions. These functions mainly consists of NULL-like operations (pass the operation to the next layer).

sidenote: the null layer included as example and for testing is not functional at the moment :(

kaspar030 · 2017-12-05T21:10:12Z

the frame is already in iovec format.

... or still. :)

I think it is easier to do packet manipulation and inspection while the packet is still handled as a chain of pktsnips.

That might be. But it is definitely easier to keep a network stack fast and memory efficient without pktsnips.

bergzand · 2017-12-05T21:16:33Z

I think it is easier to do packet manipulation and inspection while the packet is still handled as a chain of pktsnips.

That might be. But it is definitely easier to keep a network stack fast and memory efficient without pktsnips.

What I was trying to say a few posts up is that it might be easier to move the L2 stats and the L2 filter completely to gnrc_netif where the packet is already/still in pktsnip format, I really don't wan't to change netdev from iovec to pktsnips (pktsnips are a gnrc specific thing right?). :)

bergzand · 2017-12-08T19:03:11Z

Added L2 netstats layer. For now only supports ethernet and IEEE802.15.4. I've named it netdev_stats, and made a circular dependency between the original netstats_l2 pseudomodule and this module to have both the old and the new name working (including all remaining "legacy" ifdefs).

As most of the code is shareable between different L2 protocols, I've separated the differences by a case statement (only the multicast/unicast distinction needs it). I still need to add a few ifdefs there to only include the individual cases when the netdev code for that layer is included.

kaspar030 · 2017-12-09T09:32:18Z

It might be beneficial to iron out a concept before trying to keep the implementation up-to-date with the discussion... Makes it easier to only see the differences to the headers while discussing, and saves you the constant updating of actual code... 😉

bergzand · 2017-12-09T10:56:20Z

Updated class diagram and behaviour (my UML skills are not perfect, so I hope it is clear):

Things I don't like:

Every layer must implement all ops, that's why I've build a few NULL-like ops.

Problems I've noticed so far:

gnrc_netif_ieee802154 does some direct access to the netdev struct, mainly to get and update the frame sequence number, but also to check flags. I don't really want to replace this with netopt_t getters and setters, so for now I've solved this with an additional pointer to the "real" hardware device.

It might be beneficial to iron out a concept before trying to keep the implementation up-to-date with the discussion... Makes it easier to only see the differences to the headers while discussing, and saves you the constant updating of actual code... 😉

Of course. I find it useful to play around with the ideas a bit to see if it is realizable and to check if other issues come up that might have been missed in the design.

kaspar030 · 2017-12-09T12:10:20Z

Every layer must implement all ops, that's why I've build a few NULL-like ops.

This brings back an idea I've had for a while: how about refactoring netdev_t to have all function pointers in it's own structure (netdev_ops_t), which would be the first member of netdev_t?
If we also add a "parent" pointer, we could have a couple of pre-made functions that just pass on to the parent, e.g.,

netdev_send_t netdev_send_nop(netdev_t *dev, ...)
{
   return dev->parent->ops->send(dev->parent, ...)
}

The benefits of an extra ops field would be that every layer would only have a base RAM usage of two pointers (ops and parent) in addition to any state, with the ops struct being in ROM.

kaspar030 · 2017-12-09T12:51:24Z

how about refactoring netdev_t to have all function pointers in it's own structure (netdev_ops_t), which would be the first member of netdev_t?

I just relized that this is already the case. ;) Sorry for the noise.

bergzand · 2017-12-18T09:48:19Z

What's the next step here? For now I've only tried to tackle the how to implement layered netdev here, not (really) the allocation and configuration of layered netdev. Not sure if we want to do that in this PR.

I can start cleaning this up (I think I missed some doxygen here and there), but I like to know if there are more comments on the general design here.

kaspar030 · 2017-12-19T21:17:53Z

drivers/include/net/netdev.h

-#ifdef MODULE_NETSTATS_L2
-    netstats_t stats;                       /**< transceiver's statistics */
-#endif
+    netdev_t *child;                        /**< ptr to the lower netdev layer */


Somehow I'd choose parent as name, don't now why. Any reasoning for "child"?

I think this is due to my mental image of the layered stack as a literal stack of layers, the bottom one being the hardware driver and everything on top adding functionality. A packet being transmitted is handed down starting at the upper most layer to the layer below it, thus, from the parent layer to the child layer.

If you think parent (or upper/lower as stated below) is more clear to other people, I don't mind changing it. The goal here is of course to have something that is as intuitive as possible.

kaspar030 · 2017-12-19T21:18:39Z

drivers/include/net/netdev.h

+ * @return              `< 0` on error, 0 on success
+ */
+int netdev_get_pass(netdev_t *dev, netopt_t opt, void *value, size_t max_len);
+


extra newline

Also for the other comments, I'll give this whole thing a large cleanup as soon as I have time, probably not tomorrow, but somewhere this week.

And fixed now :)

kaspar030 · 2017-12-19T21:18:51Z

drivers/include/net/netdev.h

+ */
+void netdev_event_cb_pass(netdev_t *dev, netdev_event_t event);
+
+


extra newline

kaspar030 · 2017-12-19T21:18:59Z

drivers/include/net/netdev.h

+ * @brief   Passthrough event callback function.
+ *
+ * @param[in] dev       network device descriptor
+ * @param[in] event          type of the event


kaspar030 · 2017-12-19T21:19:44Z

drivers/netdev_layer/netdev_layer.c

+#include "net/netdev.h"
+#include "assert.h"
+
+netdev_t *netdev_add_layer(netdev_t *top, netdev_t *netdev) {


here and below: { goes to next line

bergzand · 2018-03-27T20:35:50Z

Reworded the commit message a bit to make it sound less generic

miri64

ACK

The comments and concernes were addressed, so I think this is okay.

kaspar030 · 2018-03-27T20:39:55Z

Nope, please wait.

miri64 · 2018-03-27T20:41:14Z

Apparently GitHub's labeling system is not concurrency-safe ^^

bergzand · 2018-03-27T20:50:52Z

Fixed murdock complaints

miri64

Actually, I found another thing

miri64 · 2018-03-27T20:52:04Z

drivers/netdev_layer/netdev_layer.c

+{
+    netdev_t *cur_dev = (netdev_t *)dev->context;
+
+    cur_dev->event_callback((netdev_t *)cur_dev, event);


dev->context is void * so casting wouldn't be necessary, but you are casting first to netdev_t *, store it in a netdev_t * variable which you then again cast to netdev_t * ;-).

Ack, will fix tomorrow morning

kaspar030

ACK from my side.

Split out this looks nicely unintrusive. ;)

kaspar030 · 2018-03-27T20:59:07Z

drivers/include/net/netdev/layer.h

+ */
+static inline netdev_t *netdev_add_layer(netdev_t *top, netdev_t *dev)
+{
+    assert(top != NULL);


This assert could also check for dev != NULL.

I can't think of a reason why it should ever be NULL, so I've added it.

how about assert(top && dev);?

I think I prefer to keep them splitted, as it makes it slightly easier to determine which case triggered an assertion (dev == NULL or top == NULL). I can of course rewrite it to

assert(top); assert(dev);

ok

edit I mean, as you like, this is nitpicking. :)

bergzand · 2018-03-28T08:40:09Z

Addressed the remarks

kaspar030 · 2018-03-28T08:52:16Z

please squash!

bergzand · 2018-03-28T09:24:04Z

Added the assertion "rewrite" and squashed

miri64

ACK

miri64 · 2018-06-04T13:43:05Z

@bergzand are your old ports for IEEE 802.15.4 and/or Ethernet still available for this somewhere? Are you planning to adapt them in the near future?

bergzand assigned miri64 Dec 3, 2017

miri64 requested changes Dec 4, 2017

View reviewed changes

miri64 mentioned this pull request Dec 5, 2017

gnrc_mac: add timeout module. #5949

Merged

miri64 mentioned this pull request Dec 8, 2017

gnrc_netif_ieee802154: drop duplicate broadcast packets (optionally) #7577

Merged

kaspar030 previously requested changes Dec 19, 2017

View reviewed changes

bergzand force-pushed the wip/netdev_layered branch from 8470fc6 to a9e649d Compare March 27, 2018 20:35

miri64 approved these changes Mar 27, 2018

View reviewed changes

kaspar030 added the Impact: major The PR changes a significant part of the code base. It should be reviewed carefully label Mar 27, 2018

miri64 added the Impact: major The PR changes a significant part of the code base. It should be reviewed carefully label Mar 27, 2018

bergzand force-pushed the wip/netdev_layered branch from a9e649d to 2fd37c0 Compare March 27, 2018 20:50

miri64 requested changes Mar 27, 2018

View reviewed changes

miri64 reviewed Mar 27, 2018

View reviewed changes

kaspar030 approved these changes Mar 27, 2018

View reviewed changes

kaspar030 changed the title ~~[RFC] Initial implementation of a more layered approach to netdev~~ netdev: Initial implementation of a more layered approach to netdev Mar 27, 2018

netdev_layer: initial import of layered netdev API

8a63b88

bergzand force-pushed the wip/netdev_layered branch from 66e402e to 8a63b88 Compare March 28, 2018 09:21

miri64 reviewed Mar 28, 2018

View reviewed changes

miri64 approved these changes Mar 28, 2018

View reviewed changes

miri64 merged commit 76edd4e into RIOT-OS:master Mar 28, 2018

miri64 mentioned this pull request Apr 5, 2018

net: add Skald, a BLE advertising stack #8884

Merged

bergzand mentioned this pull request Jun 12, 2018

gnrc_netif: Add bootstrap function for device setup #9329

Closed

This was referenced Jun 26, 2018

netdev_ieee802154: Use intermediate layer for mac header #9417

Closed

RFC: A layered approach to netdev #7736

Open

		*/
		void netdev_event_cb_pass(netdev_t *dev, netdev_event_t event);

netdev: Initial implementation of a more layered approach to netdev #8198

netdev: Initial implementation of a more layered approach to netdev #8198

Conversation

bergzand commented Dec 3, 2017 • edited Loading

UML

Data Allocation

Configuration

WIP

miri64 commented Dec 3, 2017

jnohlgard commented Dec 4, 2017

miri64 commented Dec 4, 2017

miri64 commented Dec 4, 2017

jnohlgard commented Dec 4, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

miri64 commented Dec 4, 2017

miri64 commented Dec 4, 2017

bergzand commented Dec 4, 2017

jnohlgard commented Dec 5, 2017

jnohlgard commented Dec 5, 2017

bergzand commented Dec 5, 2017

bergzand commented Dec 5, 2017

kaspar030 commented Dec 5, 2017

bergzand commented Dec 5, 2017 • edited Loading

bergzand commented Dec 8, 2017

kaspar030 commented Dec 9, 2017

bergzand commented Dec 9, 2017

kaspar030 commented Dec 9, 2017

kaspar030 commented Dec 9, 2017

bergzand commented Dec 18, 2017

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

Choose a reason for hiding this comment

bergzand commented Mar 27, 2018

miri64 left a comment

Choose a reason for hiding this comment

kaspar030 commented Mar 27, 2018

miri64 commented Mar 27, 2018

bergzand commented Mar 27, 2018

miri64 left a 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

kaspar030 left a 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

kaspar030 Mar 28, 2018 • edited Loading

Choose a reason for hiding this comment

bergzand commented Mar 28, 2018

kaspar030 commented Mar 28, 2018

bergzand commented Mar 28, 2018

miri64 left a comment

Choose a reason for hiding this comment

miri64 commented Jun 4, 2018

bergzand commented Dec 3, 2017 •

edited

Loading

bergzand commented Dec 5, 2017 •

edited

Loading

kaspar030 Mar 28, 2018 •

edited

Loading