search

RIOT-OS / RIOT

RIOT - The friendly OS for IoT
https://riot-os.org
GNU Lesser General Public License v2.1
4.94k stars 1.99k forks source link

RFC: A layered approach to netdev #7736

Open miri64 opened 7 years ago

miri64 commented 7 years ago

@haukepetersen and I today talked a little bit about the problems with netdev_t and how to dynamically add features independent from a specific device. Examples for this are

This is what we came up with:

layered_netdev

In addition to actual device drivers there are additional netdev_driver_ts available who also have another netdev_driver_t associated to themselves (I called this construction netdev_layered_t here but I'm not stuck on the name). This allows a network stack to use those features as though they were a normal network device and the whole thing would be very transparent to the network stack (so I wouldn't count this as a major API change ;-)). The benefit of this should be clear:

The questions that are still open are:

miri64 commented 7 years ago

Another thing to consider might be that this could waste a few bytes of memory, since any of these layers could just call the next for certain operations without adding any new functionality (e.g. a netstats counter wouldn't be involved in set() or isr() wasting basically 8 byte of RAM and a bit of ROM for the calldown).

miri64 commented 7 years ago

But that might be premature optimization ;-)

jnohlgard commented 7 years ago

Since many of the layers will only hook into one or two of the functions, another approach could be to change the functions themselves to linked lists, where the next function in the list is called if the argument is not handled by the current function. That would reduce memory usage when the layer implements 3 or less of the 6 API functions, and it would reduce the latency for the functions which have few hooks (netdev_t::isr for example). I don't have any solution to where and how to allocate these list items though.

Another important consideration: How do we decide which order the layers should be linked? Do we need a kind of sorting key or do we just add them in the order they are initialized during application start?

Edit: The memory reduction assumes that a function pointer and a pointer to a struct uses the same number of bytes in memory.

jnohlgard commented 7 years ago

I do like the ideas here, it would certainly be possible to simplify MAC layer implementations if the netstats stuff could be broken out into its own layer, just counting the number of packets passing back and forth. And though it would be working the same way as today, I think it would make netdev_ieee802154_send/recv more visible. The monkey patching of send and recv being done by netdev_ieee802154_init in the current implementation was not at all obvious to me until I saw the call chain in a backtrace in the debugger from inside the send of my netdev driver, but maybe I was just being blind.

Finding a solution for the allocation issue would mean that we also could potentially move the extra members in gnrc_netdev_t added by preprocessor conditions on MODULE_GNRC_MAC (https://github.com/RIOT-OS/RIOT/blob/master/sys/include/net/gnrc/netdev.h#L119-L163). It breaks ABI compatibility between builds to have public API structs change members depending on the included modules.

kaspar030 commented 7 years ago

I don't see news here. Just bad documentation and its effects.

Netdev2 has been designed to be stackable from the beginning. Just add a "parent" pointer to any netdev_t and add some logic to pass send/recv/get/set calls up to that parents.

Was anyone listening when I said "implement mac layers stacked on top of the device drivers using netdev"? edit sorry, that came out a lot more rude than intended.

jnohlgard commented 7 years ago

@kaspar030 thank you, I think using netdev is a good solution for a MAC layer, but there's still the issue of allocating it somewhere for each network device. Do you have any suggestions on how to tackle that?

kaspar030 commented 7 years ago

there's still the issue of allocating it somewhere for each network device. Do you have any suggestions on how to tackle that?

Not really. If it is not possible to do that statically (in a not completely ugly way), we could consider allowing some kind of oneway-malloc that can be used only in auto-init.

jnohlgard commented 7 years ago

I think 1-way malloc is fine on init during boot. Maybe it would be an idea to add a 1-way malloc which works during boot but after calling a certain function (malloc_finalize or sth) then any calls to that malloc will cause a panic, to prevent inadvertent malloc uses and running out of memory. For systems where you don't need dynamic allocation, and want to be able to ensure stability.

miri64 commented 7 years ago

Added the idea of #4795 to the list above to not loose track of it.

bergzand commented 6 years ago

I've been thinking a bit about this the last few days since I have a few network features in mind which could greatly benefit from "dynamic" allocation per network device. Most notably at the moment #6873 where ETX tracking doesn't make a lot of sense on wired interfaces.

Not really. If it is not possible to do that statically (in a not completely ugly way), we could consider allowing some kind of oneway-malloc that can be used only in auto-init.

Would it be possible to reuse the memory space of a thread for this? Stack starting on one end, "one way heap" on the other end. Maybe even use a thread flag to indicate that this one way malloc is allowed for the thread, as to have a way to enforce restrictions on this. As said before, the malloc could be really simple when assumed that a free() is not necessary.

miri64 commented 6 years ago

Would it be possible to reuse the memory space of a thread for this? Stack starting on one end, "one way heap" on the other end. Maybe even use a thread flag to indicate that this one way malloc is allowed for the thread, as to have a way to enforce restrictions on this. As said before, the malloc could be really simple when assumed that a free() is not necessary.

There are no threads in netdev and not supposed to be, so no.

bergzand commented 6 years ago

For now, I'm trying to ~solve~ get some discussion going on the issue of where to store the netdev_layered_t data if they are not statically allocated at compile time.

There are no threads in netdev and not supposed to be, so no.

Just so that I understand what you're saying here, the netdev radio drivers are not thread aware, so no calls to threading functions right? Somewhere up in the stack there has to be some kind of thread running an event loop controlling the drivers somehow right (gnrc_netif or gnrc_netdev)?

miri64 commented 6 years ago

Just so that I understand what you're saying here, the netdev radio drivers are not thread aware, so no calls to threading functions right? Somewhere up in the stack there has to be some kind of thread running an event loop controlling the drivers somehow right (gnrc_netif or gnrc_netdev)?

Yes, the threading, if required, is provided by the network stack, but netdev's event management (the isr()+event_callback() bootstrap) allows for totally thread-less environments as well.

bergzand commented 6 years ago

Yes, the threading, if required, is provided by the network stack, but netdev's event management (the isr()+event_callback() bootstrap) allows for totally thread-less environments as well.

Makes sense.

This whole idea of (one time dynamic) allocation of the netdev_layered_t structs doesn't necessarily have to happen inside netdev right? What I'm thinking of is a structure where the process/functions controlling a netdev_t instance (gnrc_netif or a different threadless design) also configures it with the required layered structs. How this allocation then happens is the problem of the functions above netdev and netdev doesn't have to care whether the allocation happens either dynamic or static, as long as it receives a valid pointer to a netdev_layered_t struct.

miri64 commented 6 years ago

Sound's like a nice idea. Are you willing maybe to provide a proof-of-concept. Would do it myself, but currently don't have enough time at hand for that. :-/

bergzand commented 6 years ago

Sure, not going to promise anything as my time is limited too, but it sounds like a fun challenge :)

bergzand commented 6 years ago

While implementing this for netdev, I was thinking if this layered approach is possible for netif too. Maybe MAC layer protocols such as LWMAC and GoMacH could benefit from this approach.

miri64 commented 6 years ago

Could be.

bergzand commented 6 years ago

I'd like some input on things concerning this issue. The main problem that I'm currently trying to deal with is the sometimes vague split between gnrc_netif and netdev. Mainly the l2filter and the l2_netstats module have code on both sides. So my question for now: Which functionality should be in gnrc_netif and which functionality should be in netdev?

As I see it, code dealing with the actual interface hardware(L1) should be in netdev and code dealing with the data link layer(L2) should be in gnrc_netif. But this opinion is mostly formed by looking at the current state of the code.

miri64 commented 6 years ago

Most of this issue stems from the fact that there was no "in-between" layer up-until now. Regarding l2filter and l2_netstats, I'd say put everything into the new layer. This way they can be independently used from GNRC. The drawback is, that you most likely will need a filter every link-layer each.

bergzand commented 6 years ago

I'd like to continue the generic discussion about the layered netdev approach with @tcschmidt from #9417 started here.

@bergzand it's not about me, it is the demand for a crystal clear layout of what is concepted here so that current work and future contributions can be measured against.

@tcschmidt I understand the need for clear documentation here, and I agree that it is required for future contributions. What confuses me is that you state that the architecture might not completely clear. There has been quite some discussion accompanied with UML diagrams here and in #8198 to communicate the proposed architecture. Thus my question to you for some concrete steps on how to improve the documentation and design decisions here.

From the start, netdev is an interface and the goal of making it multilayered sounds weird.

The goal here is not to change the netdev API. The approach here is to change only the way RIOT-os implements the netdev API.

bergzand commented 6 years ago

@bergzand could we do a thorough practical evaluation of the performance impacts prior to progressing (e.g., like in https://arxiv.org/abs/1801.02833) ?

@tcschmidt I think this is a good idea. Purely based on the implementation changes that we aim at here I expect the processing time/flash/ram impact to be neglible, maybe a slight increase in RAM usage (as in less than 100 bytes in total), but of course this is measurable to confirm this before merging anything significant.

Most of the technical details have been worked out, the difficult part now is to get into a state where a transition to the layered approach is possible without having a single PR that changes thousands of lines across multiple modules and radio drivers.

I'm saying this after @PeterKietzmann and I have spent life-month of disclosing and debugging insufficiencies of previous versions of netdev...

@tcschmidt Is there a result of this somewhere? I'm very interested if the end result of all the changes happening ATM cover these insufficiencies

tcschmidt commented 6 years ago

@bergzand the problem is not with code size, but with packet processing and overall stack performance. The results reported in the paper should somewhat serve as a reference: this is what the original netdev - after all debugging and optimizing - could do (@PeterKietzmann has the numbers). Future versions should undergo regression tests and be rejected, if not performing. After all, networking is king in the IoT - RIOT should show superior performance in transmitting and receiving packets. Code complexity at this layer is typically the enemy.

bergzand commented 6 years ago

@tcschmidt Yes, as I tried to mention above, I expect the impact on packet processing time to be negligible. The advantage of the approach described above is that it reduces code complexity

tcschmidt commented 6 years ago

@bergzand should not expect, but prove: experiences have been on the contrary. Packet processing is an art.

bergzand commented 6 years ago

@tcschmidt I though we already agreed on getting some measurements before merging, so I'm not completely sure what you're trying to say here.

After all, networking is king in the IoT - RIOT should show superior performance in transmitting and receiving packets. Code complexity at this layer is typically the enemy.

Would you mind opening an issue on this, aimed at how RIOT should measure this performance in transmitting and receiving packets? It would be nice if this issue results in a performance test/benchmark to have a consistent method for taking measurements.

bergzand commented 6 years ago

@tcschmidt By the way, did you think of some improvents for the documentation thing I asked you about here?

tcschmidt commented 6 years ago

@bergzand for the moment I would suggest you just cross-link with @PeterKietzmann to get his testing scripts etc. For the future, we have scheduled work on network testing as part of our HIL initiative - but this won't be finished in a few weeks. So it makes no sense to wait. Regarding documentation, I would love to see a (i) clear problem statement and (ii) well justified solution concept. However, I don't want to hold you responsible for this ...

bergzand commented 6 years ago

For the future, we have scheduled work on network testing as part of our HIL initiative - but this won't be finished in a few weeks. So it makes no sense to wait.

I still think an issue for this would be good as it gives the community a place to discuss and sync the ideas and requirements for a test like this.

Regarding documentation, I would love to see a (i) clear problem statement and (ii) well justified solution concept. However, I don't want to hold you responsible for this ...

The problem statement might not be directly clear from the opening post here, but can be easily distilled from the text. Something like:

"The current netdev implementation does not allow for adding features in a scalable way independent of the network device drivers."

I would say that the architecture of the proposed solution is well discussed and quite clear. I would like to at least add the design concepts to the header documentation when implementing this. What do you think?

tcschmidt commented 6 years ago

@bergzand no problem, I will open an issue about network testing ... just need a little time for that.

Adding the design considerations to the header documentation sounds good to me. Various people work on the code, and when discussing the netdevs (and other stuff) in retrospect for publications it quickly appeared that we all have limited memory lifetime ;)

bergzand commented 6 years ago

@bergzand no problem, I will open an issue about network testing ... just need a little time for that.

Awesome, thank you :)

miri64 commented 6 years ago

In general this work aims to reduce code complexity. The layers this issue talks about currently already exist in part and existed in the referenced paper as well (see modules like netdev_ieee802154 and netdev_ethernet). However the calling hierarchy is quite messed up at the moment + private member fields are touched in places where they are not supposed to be touched (and I admit that this is to 98% my fault ;-)). One example: when a radio specific option is retrieved for a network interface via gnrc_netapi, the interface's thread then first asks the radio driver. If the radio driver doesn't have it, it asks the IEEE 802.15.4 netdev layer. This is supposed to be the otherway around. However, to isolate IEEE 802.15.4 specific options (such as addresses, PAN ID, header flags) and to reduce code duplication, the comparably complicated header construction (they have variable length) of the IEEE 802.15.4 header is also moved to that module in #9417. That's all that is happening and shouldn't have much influence on performance (which should be proven of course).

bergzand commented 6 years ago

Okay, I'd like to have some opinions again.

I'm currently looking at the way link layer addresses are initialized. My main issue with the current architecture is that the link layer address generated by the device driver is passed up and down the stack.

Problem

The device driver generates an address based on the luid module. This is written to the netdev_t struct by the device driver. gnrc_netif requests the addres from the device driver where it is then also stored.

My problem here is that 1. there is data directly written between "layers", by the device driver, to the netdev(_ieee802154)_t struct — and 2. In a setup where multiple layers require knowledge of the link layer address, there is no way to guarantee this.

Solution

My current solution would be to have the higher layer (gnrc_netif or the netdev glue layers) generate the link layer address and set it in the device driver. This way it has to traverse all netdev layers, giving all layers explicitly knowledge of the new link layer address.

In this implementation checks would be required to check whether the netdev stack uses a link layer address and to check if the device driver provides a link layer address and then use that address.

miri64 commented 6 years ago

Didn't we "solve" the luid problem already by using luid_custom() with the netdev's pointer instead of luid_get() (see https://github.com/RIOT-OS/RIOT/issues/9656#issuecomment-434248873).

bergzand commented 6 years ago

Didn't we "solve" the luid problem already by using luid_custom() with the netdev's pointer instead of luid_get() (see #9656 (comment)).

That only solves changes to the link layer address when the device driver is reset, but that doesn't solve my problem number 1 and number 2 here (right?) :)

miri64 commented 6 years ago

Well, problem 1 and 2 won't arise, when the reset is idempotent ;-).

miri64 commented 6 years ago

Or am I misunderstanding them? :-/

bergzand commented 6 years ago

As a practical (or less hypothetical) example, the nrf52840 doesn't have hardware filtering. The software filter could be implemented as a netdev layer. This extra filter layer would then require knowledge of both the generated link layer address and the PAN ID. IMHO the easiest way for this layer to get the link layer address would be if it could grab the information from a netdev::set call.

Or am I misunderstanding them? :-/

That just means I didn't explain them good enough :)

I'm trying to remove this write by the device driver to the netdev_ieee802154_t struct. At the same time I'm trying to get to a solution that is usable when in the future multiple layers require knowledge of the link layer address.

A different solution for the second issue might be to do a netdev::get call in the init function of the layer for the link layer address and also listen for any netdev::set call that changes the link layer address. Not sure yet if this also works always though.

bergzand commented 6 years ago

These "blockers" are all cases where data is directly read from a netdev struct at a position where in a layered module the content of this struct can't be guaranteed. List might be expanded in the future

Blockers:

bergzand commented 6 years ago

Link layer address generation:

At the moment I think that the easiest way is to remove the link layer address from the netdev_ieee802154_t struct. This way the behaviour becomes identical to the behaviour of the ethernet drivers.

The main issue is that now the address generated by the device driver has to be synced somehow with the netdev_ieee802154_t struct member. It is not easily possible to have the netdev_ieee802154_t layer generate the address, some radios (socket_zep) have a built in address which should be used instead.

The only place where the link layer address is requested is with the ifconfig shell command, when netif is requesting the l2 address to initialize it's own copy and with a SLAAC failure (after setting the address).

miri64 commented 6 years ago

with a SLAAC failure (after setting the address).

DAD failure ;-).

bergzand commented 6 years ago

DAD failure ;-).

That's what I was thinking, but not what I was writing :)

stale[bot] commented 5 years ago

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.