buglwIP - A Lightweight TCP/IP stack - Bugs: bug #27771, Silly-window-avoidance decreases...

 
 

You are not allowed to post comments on this tracker with your current authentication level.

bug #27771: Silly-window-avoidance decreases window unnecessarily

Submitter:  Simon Goldschmidt <goldsimon>
Submitted:  Wed 21 Oct 2009 05:23:07 PM UTC
   
 
Category:  TCP Severity:  3 - Normal
Item Group:  None Status:  Invalid
Privacy:  Public Assigned to:  None
Open/Closed:  Closed Planned Release:  None
lwIP version:  1.3.1

Jump to the original submission

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

Simon Goldschmidt <goldsimon>
Group administrator
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.

Kieran Mansley <kieranm>
Group Member
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?

Simon Goldschmidt <goldsimon>
Group administrator
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...

Simon Goldschmidt <goldsimon>
Group administrator
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.

Kieran Mansley <kieranm>
Group Member
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?

Simon Goldschmidt <goldsimon>
Group administrator
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.

Kieran Mansley <kieranm>
Group Member
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)?

Simon Goldschmidt <goldsimon>
Group administrator
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.

Simon Goldschmidt <goldsimon>
Group administrator

 

(Note: upload size limit is set to 16384 kB, after insertion of the required escape characters.)

No files currently attached

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -email is unavailable- added by kieranm (Posted a comment)
  • -email is unavailable- added by goldsimon (Submitted the item)
  •  

    There are 0 votes so far. Votes easily highlight which items people would like to see resolved in priority, independently of the priority of the item set by tracker managers.

     

    Follow 3 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2009-10-23 goldsimon StatusNone Invalid
        Open/ClosedOpen Closed
    2009-10-22 goldsimon SummarySilly-window-avoidance shrinks window unnecessarily Silly-window-avoidance decreases window unnecessarily

    Back to the top

    Powered by Savane 3.13-caa5.
    Corresponding source code