Skip to content

Conversation

benpicco
Copy link
Contributor

@benpicco benpicco commented Jan 7, 2022

Contribution description

Both GNRC and LWIP support the notion of packet snips/slices: A packet does not have to be in continuous memory.
This is useful when e.g. sending a blockwise CoAP message where the CoAP header can be a small buffer and the CoAP block payload can be just a pointer into a larger buffer (possibly on ROM).

However, the sock API completely throws this out of the window by only allowing a single payload buffer in sock_udp_send().
This means we need another buffer to copy the payload and the header into, artificially doubling memory requirements and introducing an unnecessary copy step.

Fix this by introducing a new sock_udp_sendv() function. This allows to specify an array of payload chunks instead of a single buffer. sock_udp_send() is now simple a wrapper around the new function.

Implemented for

  • GNRC
  • LWIP
  • OpenWSN

Testing procedure

Issues/PRs references

@benpicco benpicco requested review from maribu, chrysn and miri64 January 7, 2022 13:16
@github-actions github-actions bot added Area: network Area: Networking Area: sys Area: System labels Jan 7, 2022
Copy link
Member

@maribu maribu left a comment

Choose a reason for hiding this comment

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

I do like the idea very much. One comment inline.

@github-actions github-actions bot added the Area: pkg Area: External package ports label Jan 7, 2022
@benpicco benpicco marked this pull request as ready for review January 7, 2022 14:45
@benpicco benpicco requested a review from yarrick January 7, 2022 14:46
@github-actions github-actions bot added the Area: tests Area: tests and testing framework label Jan 7, 2022
Copy link
Contributor

@kaspar030 kaspar030 left a comment

Choose a reason for hiding this comment

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

Why an array? There's reasons why gnrc and netdev use iolists instead of arrays. They're much more stack friendly, especially for layered protocols.
And, iolists are used at the lowest level. I don't think gnrc can do it, because it needs pktsnips, but theoretically an application could provide an iolist, and the network stack just has to prepend the udp header and pass it on. Maybe this can be added to gnrc's machinery, with a special "iolist" pktsnip type.
An array would need to be reallocated, potentially multiple times.

@benpicco
Copy link
Contributor Author

benpicco commented Jan 8, 2022

We are on the sock layer here so we can't use network stack native list representations anyway.
I don't see how lists or arrays differ in stack usage (in fact the list will store an additional pointer, so 4 more bytes per element) but lists are more awkward to use:

  • Both GNRC and LWIP expect the last element to be written first. This would either mean that the user has to supply the list in an 'unnatural' order, use double-linked lists or we reverse the list inside the network stack
  • Array can be statically initialized (and be compile time const)

I agree that lists provide additional flexibility in case someone wants to add multiple layers on top of sock though.

@kaspar030
Copy link
Contributor

I don't see how lists or arrays differ in stack usage (in fact the list will store an additional pointer, so 4 more bytes per element)

Think of an application writing some content using protocol X. This is a potential call stack using iolists:

proto_X_send(..., buf, len) {
  iolist_t entry = { .iol_data=buf, iol_len=len }

 proto_X_udp_send(..., &entry);
}

proto_X_udp_send(..., iolist_t entry) {
 buf, len = proto_X_create_hdr(...);
 iolist_t proto_x = {.iol_data = buf, .iol_len=len, iol_next=entry };
udp_sendv(..., &proto_x);
}

udp_sendv(..., &proto_x) {
buf, len = udp_build_hdr(...);
 iolist_t udp = {.iol_data = buf, .iol_len=len, iol_next=proto_x };
ip_send(..., &udp);
}

...

Using arrays, each layer would probably get an array as parameter. to add data, that array either needs to be in reverse order and large enough (who knows how large?), or, be recreated on each layer, but again, how big? variable length?.

IMO, lists save a lot here.

@benpicco
Copy link
Contributor Author

benpicco commented Jan 9, 2022

How about this? I'm not entirely happy with having to reverse the list depending on which network stack is used, but then again this is neatly hidden from the user and the list based interface does indeed provide more flexibility to the application.

@kaspar030
Copy link
Contributor

I still don't understand why the iolist has to be reversed. just to be clear, iolists are in-order, meaning, first entry (list head) contains bytes 0-x, second contains x+1 . ..., and so on, right?

That's how it is passed from the application, and that's how the good old netdev expects (or expected) it.

In the PR, you're reversing that for lwip and openwsn, but not gnrc? Even though lwip writes in-order using netcon_write_partly() and openwsn also copies in-order? does gnrc_pktbuf_add prepend?

It seems to me something is turned around. if gnrc_pktbuf_add prepends, we need something that appends. There's gnrc_pkt_append, that sounds like it could be used (but might need allocation of a pktsnip, even though the data could be just pointed to?

but, are we clear on the supposed order of the iolists?

(You really don't want to re-use iolist.h?)

@benpicco
Copy link
Contributor Author

Sometimes one does not see the forest due to all the trees 😄
I've added gnrc_pktbuf_add_tail() and reworked the buffer chaining in lwip_sock_sendv() so now we can always use the buffer list in the right order.

I'll split up the commits and force push shortly.

@benpicco benpicco added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Jan 10, 2022
Copy link
Contributor

@yarrick yarrick left a comment

Choose a reason for hiding this comment

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

I am not familiar with the netconn/netbuf API, but it seems the TCP send can be simplified.

@benpicco benpicco removed the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Jan 10, 2022
Copy link
Contributor

@kaspar030 kaspar030 left a comment

Choose a reason for hiding this comment

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

I'm still not convinced that introducing sock_tx_snip_t and sock_rx_snip_t is a good idea.

The main pro for that would be to not have to cast R/O data to non-const, for the case we'd send on-flash data. Also, this makes it clear from an API perspective that the callee won't ever modify the passed in data. IIUC there was also some unhappyness with the iolist member names.

Reasons for just re-using iolist_t:

  1. layers below use iolist_t
  2. other modules might use or produce iolist_t that could be passed to a socket send function.
  3. instead of one type (iolist_t), there's now three (iolist_t, sock_tx_snip_t, sock_rx_snip_t)
  4. if the member names of iolist_t suck, we can change them globally.

I think those three types will cause conversion, type casting, ..., totally outweighing the constness benefit. And I think that constness benefit can be mitigated with clear documentation (like, in the docks for sock_send(..., iolist_t snip): "@note don't modify snip->iol_data!).

I really want this functionality though. Could we, as a compromise, use iolist_t in this PR and postpone the type shedding to later?

@kaspar030
Copy link
Contributor

3. instead of one type (iolist_t), there's now three (iolist_t, sock_tx_snip_t, sock_rx_snip_t)

There's also the point that if having distinct rx/tx snips is actually the preferred choice, we should have that for iolist_t.
But, while I don't think that posix is a good example for API design, they get away with only one iovec_t type. It does reduce complexity.

@benpicco
Copy link
Contributor Author

benpicco commented Feb 21, 2022

Fine, I still maintain that the iolist_t API is fugly, but if you prefer it that way so be it.

@kaspar030
Copy link
Contributor

Fine, I still maintain that the iolist_t API is fugly

you mean because of the member names, or because of the const?

@kaspar030
Copy link
Contributor

From my perspective you can squash this!

@benpicco
Copy link
Contributor Author

you mean because of the member names, or because of the const?

both really, e.g.

@benpicco benpicco force-pushed the sock_udp_sendv branch 3 times, most recently from d0e7a39 to bada975 Compare February 22, 2022 08:17
Copy link
Contributor

@kaspar030 kaspar030 left a comment

Choose a reason for hiding this comment

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

ACK.

Code looks fine. Did some basic UDP testing, all good.

@kaspar030 kaspar030 enabled auto-merge March 1, 2022 13:04
@kaspar030 kaspar030 added Reviewed: 1-fundamentals The fundamentals of the PR were reviewed according to the maintainer guidelines Reviewed: 2-code-design The code design of the PR was reviewed according to the maintainer guidelines Reviewed: 3-testing The PR was tested according to the maintainer guidelines Reviewed: 4-code-style The adherence to coding conventions by the PR were reviewed according to the maintainer guidelines Reviewed: 5-documentation The documentation details of the PR were reviewed according to the maintainer guidelines labels Mar 1, 2022
@kaspar030 kaspar030 merged commit a17ff53 into RIOT-OS:master Mar 1, 2022
@benpicco benpicco deleted the sock_udp_sendv branch March 1, 2022 13:07
@benpicco
Copy link
Contributor Author

benpicco commented Mar 1, 2022

Thank you for the review!

@OlegHahm OlegHahm added this to the Release 2022.04 milestone Apr 25, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: network Area: Networking Area: pkg Area: External package ports Area: sys Area: System Area: tests Area: tests and testing framework CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR CI: run tests If set, CI server will run tests on hardware for the labeled PR Reviewed: 1-fundamentals The fundamentals of the PR were reviewed according to the maintainer guidelines Reviewed: 2-code-design The code design of the PR was reviewed according to the maintainer guidelines Reviewed: 3-testing The PR was tested according to the maintainer guidelines Reviewed: 4-code-style The adherence to coding conventions by the PR were reviewed according to the maintainer guidelines Reviewed: 5-documentation The documentation details of the PR were reviewed according to the maintainer guidelines
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants