Fri 23 Oct 2009 07:10:15 AM UTC, comment #8:
Closed as invalid after opening bug #27783: Silly window avoidance for small window sizes
|
Thu 22 Oct 2009 03:29:44 PM UTC, comment #7:
No, the test in tcp_update_rcv_ann_wnd should look like this (second clause added):
if (TCP_SEQ_GEQ(new_right_edge, pcb->rcv_ann_right_edge + pcb->mss) ||
TCP_SEQ_GEQ(new_right_edge, pcb->rcv_ann_right_edge + TCP_WND / 2)) {
i.e. if you change pcb->rcv_ann_wnd to TCP_WND you'd be right.
|
Thu 22 Oct 2009 03:20:10 PM UTC, comment #6:
> (iii) we can add a condition to the window update to also
> update if the increment is > WND/2 as suggested by David Empson.
Would that mean (in function tcp_update_rcv_ann_wnd()) to change the line
if (TCP_SEQ_GEQ(new_right_edge, pcb->rcv_ann_right_edge + pcb->mss)) {
to
if (TCP_SEQ_GEQ(new_right_edge, pcb->rcv_ann_right_edge + LWIP_MIN((pcb->rcv_ann_wnd / 2), pcb->mss))) {
?
I did that change and (with MSS==WND==512) it resulted in:
- announced wnd=372
- received 40 bytes
- internal wnd=rcv_ann_wnd=332
- recved 40 bytes -> pcb->wnd = 512
-> window opens by more than (332/2)=166
- announced wnd=512
so even if I though differently, this seems to be OK.
Regarding your last comment, I guess you can argue that way or the other, but the old implementation (window always stays the same when directly calling tcp_recved()) seems indeed outdated and only applicable on local LANs...
I hope you agree with me that this is targeted to 1.3.2, too?
|
Thu 22 Oct 2009 02:17:34 PM UTC, comment #5:
So it seems that both the summary of this bug as well as the bug itself is invalid? Let's wait to close this until we change the algorithm...
|
Thu 22 Oct 2009 10:52:21 AM UTC, comment #4:
Imagine that the 10 bytes was just the first 10 bytes of a longer transfer, and that the sender has in fact got the other 502 possible bytes in flight. If we advertised the 10 bytes of new window after receiving it, the server would then get an ACK that allows it to send 10 more bytes. In reality it might want to send much more, but would be limited by the window, and so send just 10 bytes. This would result in a situation where the network descended to lots of small packets. If instead we batch up the window updates and give it a window updates of 1 MSS or more, the sender will always be able to send a full segment (when it is able to send at all) and so will result in a situation there the network sticks to MSS-sized frames: this is much more efficient.
The critical thing here is not the total amount of window we're advertising (as the receiver, we can't deduce the sender's state, and so can't know how much window is really outstanding) it's the size of the increments to the window that's important.
The downside is that it makes systems where WND = MSS behave a bit worse (the window will decrease to zero before getting an update of 1 MSS) but (i) these systems were never optimal; (ii) we can add a compile time warning; and (iii) we can add a condition to the window update to also update if the increment is > WND/2 as suggested by David Empson.
|
Thu 22 Oct 2009 10:38:19 AM UTC, comment #3:
What's happening in Jan's trace is:
- lwIP announces wnd of 512
- lwIP receives 10 bytes of data
- internal rcv_wnd is now 502
- calls recv callback
- recv callback calls tcp_recved
- internal rcv_wnd is now 512
- recv_ann_wnd is still 502
- lwIP sends ACK with an announced wnd of 502
This means the window is decreasing although it wouldn't have to: if the window size we have was OK to be announced before receiving, it should be OK to be announced after receiving, shouldn't it?
|
Thu 22 Oct 2009 08:28:47 AM UTC, comment #2:
I think the current behaviour is right. I'd like to see a packet capture showing lwIP moving the right edge of the advertised window to give less to the sender. I think at the moment that it will keep this right edge constant instead of increasing it by small amounts, which is the correct behaviour. Once the small amounts add up to a sizeable increase (> 1 MSS) it will advertise that all in one go.
|
Wed 21 Oct 2009 08:44:04 PM UTC, comment #1:
The reason seems to be that tcp_update_rcv_ann_wnd() shrinks the window when called from tcp_receive(), but does not grow it when called from tcp_recved(). The reason is this line:
if (TCP_SEQ_GEQ(new_right_edge, pcb->rcv_ann_right_edge + pcb->mss)) {
together with the line
if (wnd_inflation >= TCP_WND_UPDATE_THRESHOLD)
in tcp_recved(), this looks like the condition to not send updates is doubled: the first time we prevent growing the window at all, the second time we prevent sending a direct ACK. I wonder if we have to remove the ' + pcb->mss' in the first line or if we only have to call tcp_update_rcv_ann_wnd() only once per received packet instead of multiple times (i.e. call it after the recv callback is called)?
|
Wed 21 Oct 2009 05:23:07 PM UTC, original submission:
[This has been reported on lwip-users today (together with summaries "TCP payload is doubled" and "TCP problem" in the last days).]
The algorithm seems to shrink the window even if tcp_recved() is called inside the recv-callback and there hasn't been a shrinked window announced. When calling tcp_recved() for all data inside the recv-callback, no window change should occur at all.
Since this bug was introduced in 1.3.1, it would be a good idea to delay 1.3.2 until this is fixed, I guess.
|