net: Introduce iolists #8351

kaspar030 · 2018-01-11T23:19:31Z

Contribution description

netdev used to use struct iovec for its send function. While being the standard for scatter/gather IO (well, we had to add the header as newlib doesn't support it), it is a little clumsy to use, as usually the size of the needed array is not known in stacked environments like a network stack.

This PR introduces "iolists", which are basically a linked list of IO vectors, not unlike GNRC's pktsnips.
netdev is changed to use iolists.

gnrc_pktsnip_t is rearranged so its beginning matches iolist_t, so pktsnips can be directly handed to netdev's send() function.

~~This is WIP, only netdev_tap and at86rf2xx has been adapted, and only gnrc_ethernet and gnrc_ieee802154.~~

Issues/PRs references

Probably solves bullet point #2 of #6299.

Testing checklist

miri64 · 2018-01-12T00:03:18Z

sys/include/net/gnrc/pkt.h

@@ -106,10 +106,11 @@ typedef struct gnrc_pktsnip {
     *
     * @internal
     */
-    unsigned int users;


Maybe move the doc for the field as well ;-)

yup :) done

miri64 · 2018-01-12T00:03:46Z

sys/include/net/gnrc/pkt.h

@@ -106,10 +106,11 @@ typedef struct gnrc_pktsnip {
     *
     * @internal
     */
-    unsigned int users;
+    /* the first three fields *MUST* be identical to iolist_t! */


I think you can group fields with @{ @} as well.

Hm, I thought this to be an implementation detail, thus the comment is not doxygen (/*).

kaspar030 · 2018-01-12T09:50:00Z

updated pkt unittests, and added some checking whether the structs match
added some doc do pkt.h explaining the similarities

kaspar030 · 2018-01-18T20:51:57Z

ready for review
marked major as this touches all netdev drivers and many gnrc components

I don't think this should go into the release, but I'd like to merge it early after as it goes stale rather quick...

kaspar030 · 2018-01-18T20:56:09Z

Help with testing different drivers is much appretiated! If a ping goes through correctly, it should be fine to check a box. ;)

aabadie · 2018-01-18T21:25:11Z

sys/include/iolist.h

+ */
+
+/**
+ * @defgroup    sys_util iolist scatter / gather IO


why not sys_iolist ?

or sys_util_iolist

aabadie · 2018-01-18T21:26:44Z

I can test the LoRa related stuff (sx127x and loramac) but not before next week.

aabadie

I tested it with RN2483 and SX1276 (LoRaWAN) and it works. I briefly tested with XBee and it works (only ifconfig but no send/receive).

aabadie · 2018-02-02T16:08:26Z

This PR is very nice, I think it should be merged soon since it touches many parts of the code and we are at the very beginning of a new release cycle.

miri64 · 2018-02-02T16:20:56Z

I tested it with RN2483 and SX1276 (LoRaWAN) and it works.

Can you please update the list above.

I briefly tested with XBee and it works (only ifconfig but no send/receive).

Since this is touching the send method you can't really call this a test, since ifconfig just uses get/set.

aabadie · 2018-02-02T19:04:37Z

bergzand · 2018-02-19T21:19:55Z

And identical method used for testing tests/lwmac, txtsnd also works for that test.

kaspar030 · 2018-02-19T21:21:42Z

And identical method used for testing tests/lwmac, txtsnd also works for that test.

Nice! btw, feel free to check the boxes. I find it quite satisfying. ;)

bergzand · 2018-02-19T21:30:30Z

And last one for today: openthread also verified by reproducing the steps described in tests/openthread/README.md.

bergzand · 2018-02-20T08:47:33Z

enc28j60 tested and works. 2 more to go!

kaspar030 · 2018-03-06T12:59:32Z

I propose merging as is, so it doesn't get stale and maybe more testing.
Noone seems to have a w5100 around, cc1100 is mor or less broken in master anyways, and only one of the 20+ adapted modules had an issue, so I think the chance that w5100 is broken is slim

@miri64 @bergzand What do you think?

kaspar030 · 2018-03-06T13:01:16Z

I took the liberty to rebase and squash.

miri64 · 2018-03-06T13:10:43Z

I propose merging as is, so it doesn't get stale and maybe more testing.
Noone seems to have a w5100 around, cc1100 is mor or less broken in master anyways, and only one of the 20+ adapted modules had an issue, so I think the chance that w5100 is broken is slim

@miri64 @bergzand What do you think?

Agreed.

kaspar030 · 2018-03-06T15:02:19Z

@bergzand I think your comments have been addressed?

bergzand

ACK. Note that I only tested the code, I haven't done an in depth review of the code.

bergzand · 2018-03-06T20:45:08Z

I propose merging as is, so it doesn't get stale and maybe more testing.
Noone seems to have a w5100 around, cc1100 is mor or less broken in master anyways, and only one of the 20+ adapted modules had an issue, so I think the chance that w5100 is broken is slim

@miri64 @bergzand What do you think?

+1

If there is nobody using the w5100 anymore, maybe it is better to phase it out. But that is something for a different discussion.

aabadie · 2018-03-07T07:30:30Z

If there is nobody using the w5100 anymore

Looking at the driver authorship, the initial contribution is from @haukepetersen. Maybe he knows where to find the hardware for testing ?

kaspar030 · 2018-03-07T08:56:09Z

Thanks a lot for the help with testing!

&go.

bergzand · 2018-03-07T08:56:56Z

🎉

kaspar030 assigned miri64 Jan 11, 2018

miri64 requested changes Jan 12, 2018

View reviewed changes

kaspar030 force-pushed the introduce_iolists branch from 5242784 to 93f1df7 Compare January 18, 2018 13:54

kaspar030 added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Jan 18, 2018

kaspar030 force-pushed the introduce_iolists branch 3 times, most recently from c6915b7 to 9a39582 Compare January 18, 2018 17:42

kaspar030 changed the title ~~WIP: Introduce iolists~~ net: Introduce iolists Jan 18, 2018

kaspar030 removed Discussion: RFC The issue/PR is used as a discussion starting point about the item of the issue/PR State: WIP State: The PR is still work-in-progress and its code is not in its final presentable form yet labels Jan 18, 2018

kaspar030 force-pushed the introduce_iolists branch 2 times, most recently from 3955e58 to 154a882 Compare January 18, 2018 20:37

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

aabadie reviewed Jan 18, 2018

View reviewed changes

aabadie reviewed Feb 2, 2018

View reviewed changes

kaspar030 force-pushed the introduce_iolists branch from 154a882 to dc004b0 Compare February 2, 2018 18:18

aabadie closed this Feb 2, 2018

aabadie reopened this Feb 2, 2018

kaspar030 added 12 commits March 6, 2018 14:00

sys: introduce iolist

dbbdf3f

drivers: netdev: switch to iolists

51dabf9

cpu/native/netdev_tap: adapt to netdev with iolist

ff6b8aa

sys/net/gnrc/pkt: match pktsnip struct start with iolist_t

c935a07

sys/net/gnrc/netif: adapt to netdev with iolist

2f179f2

drivers: net: adapt to iolist-using netdev

23b414b

sys/net: iolist updates

1faa845

pkg/lwip: update to use iolists

403faf2

pkg/emb6: update to use iolists

4e5e500

pkg/openthread: update to use iolists

d396d75

tests/unittests: adapt to iolist pktsnip

da67e3a

pkg/semtech-loramac: adapt to iolist netdev

c4b1fa6

kaspar030 force-pushed the introduce_iolists branch from 75a71bf to c4b1fa6 Compare March 6, 2018 13:01

miri64 approved these changes Mar 6, 2018

View reviewed changes

bergzand approved these changes Mar 6, 2018

View reviewed changes

kaspar030 merged commit d025de3 into RIOT-OS:master Mar 7, 2018

kaspar030 deleted the introduce_iolists branch March 7, 2018 08:56

miri64 mentioned this pull request Jul 1, 2020

sys: net: gnrc: performance regression #6299

Closed

3 tasks

net: Introduce iolists #8351

net: Introduce iolists #8351

Uh oh!

Conversation

kaspar030 commented Jan 11, 2018 • edited by bergzand Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Contribution description

Issues/PRs references

Testing checklist

Uh oh!

miri64 Jan 12, 2018

Choose a reason for hiding this comment

Uh oh!

kaspar030 Jan 12, 2018

Choose a reason for hiding this comment

Uh oh!

miri64 Jan 12, 2018

Choose a reason for hiding this comment

Uh oh!

kaspar030 Jan 12, 2018

Choose a reason for hiding this comment

Uh oh!

kaspar030 commented Jan 12, 2018

Uh oh!

kaspar030 commented Jan 18, 2018

Uh oh!

kaspar030 commented Jan 18, 2018

Uh oh!

aabadie Jan 18, 2018

Choose a reason for hiding this comment

Uh oh!

aabadie Feb 2, 2018 • 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!

kaspar030 Feb 2, 2018

Choose a reason for hiding this comment

Uh oh!

aabadie commented Jan 18, 2018

Uh oh!

aabadie left a comment

Choose a reason for hiding this comment

Uh oh!

aabadie commented Feb 2, 2018

Uh oh!

miri64 commented Feb 2, 2018

Uh oh!

aabadie commented Feb 2, 2018

Uh oh!

bergzand commented Feb 19, 2018

Uh oh!

kaspar030 commented Feb 19, 2018

Uh oh!

bergzand commented Feb 19, 2018

Uh oh!

bergzand commented Feb 20, 2018

Uh oh!

kaspar030 commented Mar 6, 2018

Uh oh!

kaspar030 commented Mar 6, 2018

Uh oh!

miri64 commented Mar 6, 2018

Uh oh!

kaspar030 commented Mar 6, 2018

Uh oh!

bergzand left a comment

Choose a reason for hiding this comment

Uh oh!

bergzand commented Mar 6, 2018

Uh oh!

aabadie commented Mar 7, 2018

Uh oh!

kaspar030 commented Mar 7, 2018

Uh oh!

bergzand commented Mar 7, 2018

Uh oh!

Uh oh!

kaspar030 commented Jan 11, 2018 •

edited by bergzand

Loading

aabadie Feb 2, 2018 •

edited

Loading