net: tcp: Allow application to manage TCP receive window #1002
Conversation
3ac0403 to
c2e62ed
Compare
|
|
||
| ret = packet_received(conn, pkt, context->tcp->recv_user_data); | ||
|
|
||
| context->tcp->send_ack += data_len; |
There was a problem hiding this comment.
Earlier the value of send_ack was updated before a call to packet_received(), shouldn't we do it like that here too?
There was a problem hiding this comment.
This is generally more correct. ACK sequence should be updated after we (== app) have received the data, and we (== app) received it after return from the receive callback, not before it.
I'll address the other comments.
subsys/net/ip/net_context.c
Outdated
| @@ -1187,10 +1187,18 @@ NET_CONN_CB(tcp_established) | |||
| } | |||
|
|
|||
| set_appdata_values(pkt, IPPROTO_TCP); | |||
| context->tcp->send_ack += net_pkt_appdatalen(pkt); | |||
|
|
|||
| uint16_t data_len = net_pkt_appdatalen(pkt); | |||
There was a problem hiding this comment.
You are introducing data_len here in the middle of function.
subsys/net/ip/net_context.c
Outdated
| return -EPROTOTYPE; | ||
| } | ||
|
|
||
| context->tcp->recv_wnd += delta; |
There was a problem hiding this comment.
Shouldn't we check here that the delta is not too large or small?
There was a problem hiding this comment.
Well, we can, but it will make code much less pretty ;-). Do we really need to do that now? ;-)
There was a problem hiding this comment.
Well, we can, but it will make code much less pretty ;-). Do we really need to do that now? ;-)
I think we need to, otherwise this will be forgotten :)
The recv_wnd is uint16_t (actually it should be u16_t, missed this on my first review), and we just cannot blindly +- some user specified int value.
There was a problem hiding this comment.
recv_wnd is uint16_t (actually it should be u16_t
Sorry, this is all is based on the older patch made before u*_t refactor. Now fixed.
Ok, to do bounds checking right, we should consider: is u16_t the right type for recv_wnd value (as stored in struct net_tcp)?
There was a problem hiding this comment.
Probably u16_t is enough here, I mean would it make sense to have a window size that is larger than 65kb in Zephyr?
There was a problem hiding this comment.
Yes, I think 64K should be quite enough for Zephyr. Ok, I'll use s32_t arithmetics to check bounds of u16_t value.
948553a to
96045ed
Compare
This fixes the existing situation that "if application buffers data, it's the problem of application". It's actually the problem of the stack, as it doesn't allow application to control receive window, and without this control, any buffer will overflow, peer packets will be dropped, peer won't receive acks for them, and will employ exponential backoff, the connection will crawl to a halt. This patch adds net_context_tcp_recved() function which an application must explicitly call when it *processes* data, to advance receive window. Jira: ZEP-1999 Signed-off-by: Paul Sokolovsky <paul.sokolovsky@linaro.org>
As we buffer incoming packets in receive callbacks, we must decrease receive window to avoid situation that incoming stream for one socket uses up all buffers in the system and causes deadlock. Once user app consumes queued data using recv() call, we increase window again. Signed-off-by: Paul Sokolovsky <paul.sokolovsky@linaro.org>
96045ed to
daeeb14
Compare
Signed-off-by: Brian J Jones <brian.j.jones@intel.com>
This is rework of #81. As we didn't agree how TCP recv window management should work on the level of native Zephyr networking API, the new patch makes the management completely optional. By default, all apps continue to work as they did before (where there's static window size). However, apps which queue up data, can both shrink and widen it as needed. This feature is required and used by BSD Sockets implementation to achieve reliable transfers of large amounts of data (megabytes).