Skip to content
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.

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

sys/net/gcoap: Increase default GCOAP_PDU_BUF_SIZE #16377

Closed

Conversation

chrysn
Copy link
Member

@chrysn chrysn commented Apr 22, 2021

Contribution description

This bumps the default gcoap receive and retransmit buffer size from 128 to about 1k; questions about this come in again and again, quoting IRC:

tobi: does anyone know why the GCOAP_PDU_BUF_SIZE is 128?
Kaspar: This is an FAQ by now. It used to be (and probably is) allocated on stack once or twice by gcoap, so 128 was chosen as a low default.

In most cases, GCOAP_PDU_BUF_SIZE buffers are only ever allocated statically. The one exception is the gcoap shell application where there's a stack allocated buffer as well, and the default stack size has been increased there.

Testing procedure

  • Running the gcoap example, send a large payload eg. using aiocoap-client 'coap://[fe80::3c63:beff:fe85:ca96%tapbr0]/' -m PUT --payload @README.rst; before this, it'll just retransmit and time out; after, it'll just return 4.04 as it should.
    This demonstrates how Gcoap drops long packages instead of gracefully erring out #14167 is worked around.
  • Send a request from gcoap to see it doesn't overflow the stack: coap get -c 2a01:4f8:190:3064::5 5683 /.well-known/core

(But primarily, I'm interested if any builds now overflow the RAM in the CI tests)

Issues/PRs references

While this does not resolve #14167, it mitigates it -- we can now receive reasonably sized packages, ie. those that are sent by a client that does not do path MTU discovery.

It doesn't look like any other resolution is imminent, and the behavior resulting from large requests and small buffers is confusing to newcomers.

Open questions

  • Do we really want to do it this way?
  • Is there a clean way to get the GCOAP_PDU_BUF_SIZE into the main thread size of the gcoap example? (Right now I'm adding the default size in hardcoded numbers, because the default define is not available at the point where the stack size is evaluated.)

The number is from RFC7252 Secion 4.6.

It is increased to ensure that any to-be-expected-sized message can be
received, and to avoid arbitrary limits. Applications can still downsize
the buffer if they run into constraints, provided they know that their
applications will never send such large requests.

Mitigates: RIOT-OS#14167
@chrysn chrysn requested review from miri64 and kaspar030 April 22, 2021 20:02
@chrysn chrysn added the Area: CoAP Area: Constrained Application Protocol implementations label Apr 22, 2021
@kfessel
Copy link
Contributor

kfessel commented Apr 23, 2021

Why don't you just put static in front of that buf in the gcoap example?

May be the cleaner solution would be to ask gcoap for one of its resend_buffers (as they are already buffers to be used for sending and some messages are copied there anyway)

@MrKevinWeiss MrKevinWeiss added this to the Release 2021.07 milestone Jun 22, 2021
@miri64
Copy link
Member

miri64 commented Jul 15, 2021

Ping @chrysn?

@MrKevinWeiss MrKevinWeiss removed this from the Release 2021.07 milestone Jul 15, 2021
@chrysn
Copy link
Member Author

chrysn commented Jul 15, 2021

Going for static in the gcoap example is a viable alternative to increasing the stack size. (I'm not too big a fan of it in examples because it needs a few lines of comments to illustrate why the function does not need to be reentrant -- but if you prefer, I can change it).

@miri64
Copy link
Member

miri64 commented Jul 20, 2021

I guess if it is well documented in the code, it is ok. Better show new contributors how to properly allocate buffers instead of having to tweak the stack size every time we need a bigger payload ;-).

…ocation

examples/gcoap: Allocate the send buffer statically
@github-actions github-actions bot added Area: examples Area: Example Applications Area: network Area: Networking Area: sys Area: System and removed Area: CoAP Area: Constrained Application Protocol implementations labels Aug 24, 2021
@chrysn
Copy link
Member Author

chrysn commented Aug 24, 2021

A static in the example is fine with me; pushed as a squash commit. (Failing check-commits checks can be resolved at rebase time).

I do have a different proposal for resolving #14167 too, but this here is the straightforward one, and the other (which doesn't even have a draft PR yet) can still demo how to use CoAP with smaller buffer sizes by tuning it down -- but unless users implement handlers carefully and with awareness of the limitations, a full message size is the IMO the better default.

@miri64
Copy link
Member

miri64 commented Aug 24, 2021

I do have a different proposal for resolving #14167 too, but this here is the straightforward one, and the other (which doesn't even have a draft PR yet) can still demo how to use CoAP with smaller buffer sizes by tuning it down -- but unless users implement handlers carefully and with awareness of the limitations, a full message size is the IMO the better default.

Mhhhh I am guessing that other proposal could tap into #16715's potential? ^^

@chrysn
Copy link
Member Author

chrysn commented Aug 24, 2021

Kind of. The other proposal would introduce a new memo state ("you got a message, but it's truncated"), which a full #16715 solution would probably need to do proper auto-slicing. But it also depends on sock_udp_recv_buf and that's still experimental.

@chrysn
Copy link
Member Author

chrysn commented Aug 24, 2021

Correction: The other proposal does have a PR, #16378. Github just doesn't manage to find PRs based on the branch name facepalm. (I know gerrit isn't perfect, but).

There has been quite some activity I've failed to follow up on as well, continuing the sprint day there...

@chrysn chrysn added the State: waiting for other PR State: The PR requires another PR to be merged first label Aug 24, 2021
@chrysn
Copy link
Member Author

chrysn commented Aug 24, 2021

Looing at the state of #16378, I think that that one will win.

I'm keeping this PR open and on "waiting for other PR", but expect that it will be closed eventually when #16378 is merged.

@chrysn
Copy link
Member Author

chrysn commented Oct 27, 2021

I'm withdrawing this on grounds of #16378 being merged.

A small buffer will usually be sufficient, applications can now work provided all involved CoAP peers are well-behaved, and increasing this has become purely a performance trade-off thing. (And I personally prefer to have things that break early when the peer is not top notch so it can be fixed, rather than having things fall apart subtly when performance parameters are tuned).

@chrysn chrysn closed this Oct 27, 2021
@chrysn chrysn added State: won't fix State: The issue can not or will not be fixed and removed State: waiting for other PR State: The PR requires another PR to be merged first labels Oct 27, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: examples Area: Example Applications Area: network Area: Networking Area: sys Area: System State: won't fix State: The issue can not or will not be fixed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Gcoap drops long packages instead of gracefully erring out
4 participants