Skip to content

sock: amend with zero-copy receive functions #13616

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 2 commits into from
Mar 24, 2020
Merged

sock: amend with zero-copy receive functions #13616

merged 2 commits into from
Mar 24, 2020

Conversation

miri64
Copy link
Member

@miri64 miri64 commented Mar 11, 2020

Contribution description

This change amends the sock API by a set of functions to sock that allow provisioning of stack-internal buffers to the caller on receive.

This allows to cover two use-cases:

  1. Zero-copy systems: if the stacks supported the buffer space provided by these functions can be the same that was filled in the link-layer
  2. asynchronous receive within a wrapping sock layer (e.g. sock_dtls wrapping sock_udp): to receive packets of the lower level protocol asynchronously, the wrapping implementation layer would currently need to allocate its own buffer space, introducing a third buffer space in addition to the one of the application and the network stack. For a wrapping layer this is undesirable.

While there are security considerations exposing stack internal memory space to the caller, I believe they are minor, as in the end the application developer is the person in control of the node.

This change only amends the sock API and does not change it, so even though it is an API change, it should only require one ACK.

Testing procedure

Only API amendments here, no functional implementation (yet). So just check if

make doc

succeeds. All sock tests should still compile (Murdock can do that).

Issues/PRs references

See #12907 (comment)

@miri64 miri64 added Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation Area: network Area: Networking Discussion: RFC The issue/PR is used as a discussion starting point about the item of the issue/PR Process: API change Integration Process: PR contains or issue proposes an API change. Should be handled with care. labels Mar 11, 2020
@miri64 miri64 mentioned this pull request Mar 11, 2020
Merged
@miri64
Copy link
Member Author

miri64 commented Mar 17, 2020

@kaspar030 ping?

@benpicco
Copy link
Contributor

I suppose you want to make sure the API is sound before adding the implementation?

@miri64
Copy link
Member Author

miri64 commented Mar 23, 2020

I suppose you want to make sure the API is sound before adding the implementation?

Not just sound, but also under rough consensus. Also, since it is not just "the implementation" but several, I think its best to put those in separate PRs as we usually do with sock.

@miri64 miri64 added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Mar 23, 2020
@benpicco
Copy link
Contributor

benpicco commented Mar 23, 2020 •
edited
Loading

From a first glance the parameters look fine.
Just some minor bikeshedding: wouldn't calling the functions sock_*_recv_async() make their intention clearer?

@miri64
Copy link
Member Author

miri64 commented Mar 23, 2020 •
edited
Loading

Just some minor bikeshedding: wouldn't calling the functions sock_*_recv_async() make their intention clearer?

Why? Asynchronous receive is just one use case for which these functions are needed for (see use-cases above). Also, the name should rather reflect the functionality (provide the buffer with the receive), not the use case.

benpicco
benpicco previously approved these changes Mar 24, 2020
View reviewed changes
Copy link
Contributor

@benpicco benpicco left a comment

Choose a reason for hiding this comment

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

Looks good to me.

leandrolanzieri
leandrolanzieri reviewed Mar 24, 2020
View reviewed changes
leandrolanzieri
leandrolanzieri reviewed Mar 24, 2020
View reviewed changes
@kaspar030
Copy link
Contributor

Ok, I see some difficulty: As is, the application might not free the buffer. That means the stack is forced to allocate this somewhere where it is not blocking the rest of the stack, or block itself until the buffer is freed.

In gnrc, everything is allocated anyways, so this makes sense and removes the need for another copy of the payload.

In a hypothetical single-RX-buffer stack, this is more difficult. It would probably move the allocation to within the stack.

Would it make sense to limit this to sock_async, and make the returned buffer only valid within the async callback? That would also simplify the API, as buf_context could be dropped.

@benpicco benpicco dismissed their stale review March 24, 2020 11:54

pending comments

@miri64
Copy link
Member Author

miri64 commented Mar 24, 2020

Would it make sense to limit this to sock_async, and make the returned buffer only valid within the async callback? That would also simplify the API, as buf_context could be dropped.

I don't think so. As the event handling for sock_async is not implemented in stack we would need an externally available free function. This would still open the API for misuse.

@miri64
Copy link
Member Author

miri64 commented Mar 24, 2020

In a hypothetical single-RX-buffer stack, this is more difficult. It would probably move the allocation to within the stack.

Then for those stacks zero-copy would not be possible?

@miri64
Copy link
Member Author

miri64 commented Mar 24, 2020

As is, the application might not free the buffer.

This would be faulty usage anyway.

@kaspar030
Copy link
Contributor

This would be faulty usage anyway.

Well, not freeing at all would probably be faulty. But freeing at an arbitrary time would be valid, as proposed.

@kaspar030
Copy link
Contributor

Would it make sense to limit this to sock_async, and make the returned buffer only valid within the async callback? That would also simplify the API, as buf_context could be dropped.

I don't think so. As the event handling for sock_async is not implemented in stack we would need an externally available free function. This would still open the API for misuse.

True. Ok, let's go with it.

@kaspar030
Copy link
Contributor

Then for those stacks zero-copy would not be possible?

I think in the very limited stacks that fit only one packet, it is acceptable to just consider the RX buffer full as long as the application blocks it. The alternative is to have full memory, which has the same effect in practice (dropped packets).

@kaspar030
Copy link
Contributor

ACK from my side, but I think @leandrolanzieri has a point.

@miri64
Copy link
Member Author

miri64 commented Mar 24, 2020

BTW in case anybody is wondering, why there is no TCP equivalent: As TCP needs a buffer to fill anyway, I think it makes little sense to provide this functionality for this protocol.

@miri64 miri64 force-pushed the sock/enh/recv_buf branch from a471343 to 1454e60 Compare March 24, 2020 12:25
@miri64
Copy link
Member Author

miri64 commented Mar 24, 2020

Addressed @leandrolanzieri and squashed directly. I also piggy-backed some long overdue doc fixes ;-).

@miri64 miri64 force-pushed the sock/enh/recv_buf branch from 1454e60 to 50d3ac2 Compare March 24, 2020 12:28
@miri64
Copy link
Member Author

miri64 commented Mar 24, 2020

Oops, removed wrong error doc and forgot to remove it for sock_dtls and sock_ip. Now fixed!

benpicco
benpicco approved these changes Mar 24, 2020
View reviewed changes
Copy link
Contributor

@benpicco benpicco left a comment

Choose a reason for hiding this comment

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

Looks good to me.

@miri64 miri64 added CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR CI: skip compile test If set, CI server will run only non-compile jobs, but no compile jobs or their dependent jobs and removed CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Mar 24, 2020
@miri64
Copy link
Member Author

miri64 commented Mar 24, 2020

Last build passed. Since then only doc was changed. There are only function declarations introduced here, but no implementations. So I think we can skip the compile tests.

miri64 and others added 2 commits March 24, 2020 14:02
@miri64
sock: amend with zero-copy receive functions
a7d4689 
This change amends the `sock` API by a set of functions to `sock` that
allow provisioning of stack-internal buffers to the caller on receive.

This allows to cover two use-cases

1. Zero-copy systems: if the stacks supported the buffer space provided
   by these functions can be the same that was filled in the link-layer
2. asynchronous receive within a wrapping sock layer (e.g. `sock_dtls`
   wrapping `sock_udp`): to receive packets of the lower level protocol
   asynchronously, the wrapping implementation layer would currently
   need to allocate its own buffer space, introducing a third buffer
   space in addition to the one of the application and the network
   stack. For a wrapping layer this is undesirable.

While there are security considerations exposing stack internal memory
space to the caller, I believe they are minor, as in the end the
application developer is the person in control of the node.
@miri64
sock_udp: fix doc of sock objects
2ed7a73
@miri64 miri64 force-pushed the sock/enh/recv_buf branch from 50d3ac2 to 2ed7a73 Compare March 24, 2020 13:02
@miri64
Copy link
Member Author

miri64 commented Mar 24, 2020

Found yet another doc error and squashed directly

@miri64
Copy link
Member Author

miri64 commented Mar 24, 2020

Travis succeeded, but it isn't showed in GitHub somehow. Let's merge this so I can go ahead with the implementation part.

@miri64 miri64 merged commit ba711c5 into RIOT-OS:master Mar 24, 2020
@miri64 miri64 deleted the sock/enh/recv_buf branch March 24, 2020 16:10
This was referenced Mar 24, 2020
Merged
Merged
@leandrolanzieri leandrolanzieri added this to the Release 2020.04 milestone Mar 26, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@leandrolanzieri leandrolanzieri leandrolanzieri left review comments

@benpicco benpicco benpicco approved these changes

@haukepetersen haukepetersen Awaiting requested review from haukepetersen

@kaspar030 kaspar030 Awaiting requested review from kaspar030

@MichelRottleuthner MichelRottleuthner Awaiting requested review from MichelRottleuthner

@pokgak pokgak Awaiting requested review from pokgak

Assignees
No one assigned
Labels
Area: network Area: Networking CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR CI: skip compile test If set, CI server will run only non-compile jobs, but no compile jobs or their dependent jobs Discussion: RFC The issue/PR is used as a discussion starting point about the item of the issue/PR Process: API change Integration Process: PR contains or issue proposes an API change. Should be handled with care. Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation
Projects
None yet
Milestone
Release 2020.04
Development

Successfully merging this pull request may close these issues.
4 participants
@miri64 @benpicco @kaspar030 @leandrolanzieri