net: added generic network interface header format by haukepetersen · Pull Request #2449 · RIOT-OS/RIOT

haukepetersen · 2015-02-12T18:32:06Z

This is a first approach to a generic link layer header that can be used throughout the network stack.

The basic idea is create a link layer header format, that all link layers understand and that contains the data from link layers, that upper protocols might be interested in.

I am not too happy with this implementation yet, maybe somebody has a better idea how to deal with varying address sizes?

LudwigKnuepfer · 2015-02-12T18:46:05Z

sys/include/net/ng_llhdr.h

"optional options" ;)

(Seriously - what is this supposed to be?)

that is open for discussion, there 8-bit left in the struct to fill (so it aligns), so I thought some options field might be a good idea...

OlegHahm · 2015-02-13T10:30:48Z

I don't fully understand the purpose of this PR. Why should any layer apart from IP/network layer be interested in the link layer header? Even for the network layer I'm not sure, where it would be needed.

miri64 · 2015-02-13T11:54:52Z

I don't fully understand the purpose of this PR

The purpose is to unify the very heterogeneous link layer headers with a simpler, but common, header format. The drivers translate it then to the format they need / they translate it from their format to the common one.

Why should any layer apart from IP/network layer be interested in the link layer header?

Routing needs sometimes information for their metrics (see #2449 (comment)) + according to @cgundogan their are TCP extensions that use link-layer information.

Even for the network layer I'm not sure, where it would be needed.

Uhm, ARP/NDP needs to now the link layer addresses?

miri64 · 2015-02-13T13:37:01Z

Just stumbled upon this: can you rename this module l2hdr (for layer 2 header)? Otherwise it can get very confusing in IP with LL either meaning link-local or link layer.

OlegHahm · 2015-02-13T13:39:12Z

Routing needs sometimes information for their metrics (see #2449 (comment)) + according to @cgundogan their are TCP extensions that use link-layer information.

As far as I see it, the required information (e.g. RSSI or ETX) are rather PHY layer information. They are usually not part of a link layer header. I complete agree that it makes sense to provide this data to upper layers or routing, but I would not put them into a link-layer header.

Uhm, ARP/NDP needs to now the link layer addresses?

To my understanding (and at least Wikipedia agrees) ARP and NDP are not part of the network layer.

Maybe the general problem is more a terminology problem. This PR seems to provide information like the one that is currently handled by sys/include/radio/types.h and I would vote split it accordingly into link-layer and radio/PHY data structs.

miri64 · 2015-02-13T13:41:23Z

Just stumbled upon this: can you rename this module l2hdr (for layer 2 header)? Otherwise it can get very confusing in IP with LL either meaning link-local or link layer.

We also would then have a really easy scheme to refer to the different layers in abbreviations:

link layer: l2
network layer: l3
transport layer: l4
application layer: l7

miri64 · 2015-02-13T13:41:56Z

Maybe the general problem is more a terminology problem. This PR seems to provide information like the one that is currently handled by sys/include/radio/types.h and I would vote split it accordingly into link-layer and radio/PHY data structs.

And why not KISS and unify this into one header?

miri64 · 2015-02-13T13:43:38Z

To my understanding (and at least Wikipedia agrees) ARP and NDP are not part of the network layer.

Yes, but at least NDP is controlled by a network layer protocol (ICMPv6). That means, that the network layer needs this information to have any kind of network resolution.

OlegHahm · 2015-02-13T13:50:27Z

I would argue that ICMP is not pure network layer protocol (since it uses IP for transport), but that's not the point. I'm not against unifying this information, but strongly against putting all into a structure called link-layer header.

miri64 · 2015-02-13T22:38:01Z

sys/include/net/ng_llhdr.h

I think we need to put the PID for the link-layer interface in here to. While implementing IPv6 I wondered how else a routing protocol might get those to put them in the FIB. Do they need it anyway? @cgundogan @Lotterleben @benpicco or @fnack?

At least for multi-interface support in NHDP and OLSRv2 which are working atop of IPv6 it is necessary that the protocols can determine on which interface a packet was received on. I haven't looked into the whole new network stack structure yet, so I can't assess whether this is the right struct to save this information but generally the information has to be available somewhere for network layer protocols.

haukepetersen · 2015-02-18T16:51:31Z

I agree that link layer header wasn't maybe the best name for this -> I renamed it to interface header for now, somebody has a better idea?

Anyway: The notion behind this PR is, that we need a generic way to speak with link-layers/network interfaces of different type - and this is done through this proposed header format. For sending out data the link layer addresses are given to the link layer using this header. When receiving data, the link layer addresses and additionally all information that is interesting for upper layers is bundled into this header, so it can be accessed generically from layers above (without having them to know any specifics of the underlying link layer technologies...).

haukepetersen · 2015-02-18T17:01:08Z

and some more fixes...

OlegHahm · 2015-02-18T17:53:42Z

I would argue that things like RSSI values are part of any header.

haukepetersen · 2015-02-18T18:04:43Z

true, not in a conventional sense. But how would you make this data available to upper layers in a clean way?

OlegHahm · 2015-02-18T18:12:09Z

Maybe calling the struct something like radio_data_t or packet_info_t and set a pointer in the link layer header?

haukepetersen · 2015-02-18T19:02:00Z

just makes stuff more complicated by introducing additional de-referencing... remember this data is allocated in the packet buffer and every new element in the pktsnip-chain introduces addtional overhead...

OlegHahm · 2015-02-19T07:15:30Z

Then the question is strict layering vs. efficiency, I guess.

miri64 · 2015-02-19T09:35:00Z

I agree that link layer header wasn't maybe the best name for this -> I renamed it to interface header for now, somebody has a better idea?

In general I like the connection to interfaces for the name, but for consistency I would name it ng_netif_hdr and even make it a submodule of ng_netif (as I did e.g. with ng_ipv6 and ng_ipv6_addr)

haukepetersen · 2015-02-19T10:04:48Z

@authmillenon: I think that would make sense. But I probably won't get to it before next week...

haukepetersen · 2015-03-02T15:13:14Z

renamed it to ng_netif_hdr_* and made is sub-module to netif. What do you think?

miri64 · 2015-03-03T15:48:28Z

~~From me that would be ACK~~, but I don't want to brush over @OlegHahm's head ;-)

miri64 · 2015-03-03T15:49:31Z

sys/include/net/ng_netif/hdr.h

Arghs: one last minute thing: %s/IFHDR/NG_NETIF_HDR/g ;-)

haukepetersen · 2015-03-03T18:11:03Z

addressed comments.

miri64 · 2015-03-03T18:29:47Z

sys/include/net/ng_netif/hdr.h

I did not want the @ingroup ng_netif to go :(

haukepetersen · 2015-03-03T19:05:52Z

addresses more comments.

OlegHahm · 2015-03-03T20:49:05Z

ACK - squash please

haukepetersen · 2015-03-03T21:01:34Z

squashed and go.

net: added generic network interface header format

haukepetersen added NSTF Discussion: RFC The issue/PR is used as a discussion starting point about the item of the issue/PR Area: network Area: Networking labels Feb 12, 2015

haukepetersen assigned miri64 Feb 12, 2015

haukepetersen added this to the Network Stack Task Force milestone Feb 12, 2015

LudwigKnuepfer reviewed Feb 12, 2015
View reviewed changes

miri64 mentioned this pull request Feb 13, 2015

ng_ipv6: initial import #2454

Merged

miri64 reviewed Feb 13, 2015
View reviewed changes

haukepetersen changed the title ~~net: added generic link layer header format~~ net: added generic network interface header format Feb 18, 2015

haukepetersen force-pushed the ng_llhdr branch from aa697c4 to eacec1e Compare February 18, 2015 16:52

miri64 mentioned this pull request Feb 22, 2015

Feature clock sync (gtimer and protocols: gtsp,ftsp,pulsesync) #1538

Closed

haukepetersen force-pushed the ng_llhdr branch 2 times, most recently from 555c23c to 8a33d55 Compare March 2, 2015 15:08

miri64 added the CI: needs squashing Commits in this PR need to be squashed; If set, CI systems will mark this PR as unmergable label Mar 3, 2015

miri64 reviewed Mar 3, 2015
View reviewed changes

sys/include/net/ng_netif/hdr.h Outdated

Copy link
Copy Markdown

Member

miri64 Mar 3, 2015

Choose a reason for hiding this comment

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

Arghs: one last minute thing: %s/IFHDR/NG_NETIF_HDR/g ;-)

miri64 reviewed Mar 3, 2015
View reviewed changes

sys/include/net/ng_netif/hdr.h Outdated

Copy link
Copy Markdown

Member

miri64 Mar 3, 2015

Choose a reason for hiding this comment

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

I did not want the @ingroup ng_netif to go :(

net: added generic network interface header format

be99872

haukepetersen force-pushed the ng_llhdr branch from 4213055 to be99872 Compare March 3, 2015 21:00

haukepetersen added a commit that referenced this pull request Mar 3, 2015

Merge pull request #2449 from haukepetersen/ng_llhdr

4b9eaa9

net: added generic network interface header format

haukepetersen merged commit 4b9eaa9 into RIOT-OS:master Mar 3, 2015

haukepetersen deleted the ng_llhdr branch March 3, 2015 21:01

miri64 mentioned this pull request Mar 4, 2015

Network stack refactoring: TODO list #2278

Closed

36 tasks

Conversation

haukepetersen commented Feb 12, 2015

Uh oh!

LudwigKnuepfer Feb 12, 2015

Choose a reason for hiding this comment

Uh oh!

LudwigKnuepfer Feb 12, 2015

Choose a reason for hiding this comment

Uh oh!

haukepetersen Feb 12, 2015

Choose a reason for hiding this comment

Uh oh!

OlegHahm commented Feb 13, 2015

Uh oh!

miri64 commented Feb 13, 2015

Uh oh!

miri64 commented Feb 13, 2015

Uh oh!

OlegHahm commented Feb 13, 2015

Uh oh!

miri64 commented Feb 13, 2015

Uh oh!

miri64 commented Feb 13, 2015

Uh oh!

miri64 commented Feb 13, 2015

Uh oh!

OlegHahm commented Feb 13, 2015

Uh oh!

miri64 Feb 13, 2015

Choose a reason for hiding this comment

Uh oh!

fnack Feb 14, 2015

Choose a reason for hiding this comment

Uh oh!

haukepetersen commented Feb 18, 2015

Uh oh!

haukepetersen commented Feb 18, 2015

Uh oh!

OlegHahm commented Feb 18, 2015

Uh oh!

haukepetersen commented Feb 18, 2015

Uh oh!

OlegHahm commented Feb 18, 2015

Uh oh!

haukepetersen commented Feb 18, 2015

Uh oh!

OlegHahm commented Feb 19, 2015

Uh oh!

miri64 commented Feb 19, 2015

Uh oh!

haukepetersen commented Feb 19, 2015

Uh oh!

haukepetersen commented Mar 2, 2015

Uh oh!

miri64 commented Mar 3, 2015

Uh oh!

miri64 Mar 3, 2015

Choose a reason for hiding this comment

Uh oh!

haukepetersen commented Mar 3, 2015

Uh oh!

miri64 Mar 3, 2015

Choose a reason for hiding this comment

Uh oh!

haukepetersen commented Mar 3, 2015

Uh oh!

OlegHahm commented Mar 3, 2015

Uh oh!

haukepetersen commented Mar 3, 2015

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants