buglwIP - A Lightweight TCP/IP stack - Bugs: bug #50476, active open, ssthresh is really...

 
 

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

bug #50476: active open, ssthresh is really low with auto-tuning receiver

Submitted by:  Joel Cunningham <jcunningham>
Submitted on:  Tue 07 Mar 2017 05:54:23 AM UTC  
 
Category: TCPSeverity: 3 - Normal
Item Group: Faulty BehaviourStatus: Fixed
Privacy: PublicAssigned to: Joel Cunningham <jcunningham>
Open/Closed: ClosedPlanned Release: None
lwIP version: git head

(Jump to the original submission Jump to the original submission)

Fri 10 Mar 2017 07:17:43 AM UTC, comment #14:

Looks good, I've pushed it. Thanks.

Simon Goldschmidt <goldsimon>
Project Administrator
Thu 09 Mar 2017 10:55:10 PM UTC, comment #13:

I studied test_tcp_fast_rexmit_wraparound and it is making assumptions about how many segments are transmitted upon ACKing the first segment when multiple segments are queued for output (waiting on the in-flight data to decrease). It's not clear to me whether the test relied on tcp_alloc() to keep ssthresh at 0 (enforcing congestion avoidance) or if it was working by accident until my changes.

The attached patch enforces congestion avoidance for this test by setting pcb->ssthresh to pcb->cwnd. The test passes now.

I also looked at the other tests and did not see the same assumption, but some have the txcount checks commented out (see test_tcp_fast_retx_recover) so it's not clear. The others passed without any modification

(file #39955)

Joel Cunningham <jcunningham>
Project MemberIn charge of this item.
Thu 09 Mar 2017 08:55:55 PM UTC, comment #12:

Honestly, I'm not sure what's best as a change here. Git blame sys I did the test, but that was SVN age, so I'm rather certain I committed the code but didn't write it ;-)

Simon Goldschmidt <goldsimon>
Project Administrator
Thu 09 Mar 2017 08:41:16 PM UTC, comment #11:

I debugged the unit test and here's what's going on:

Before the change to set ssthresh in tcp_allloc(), ssthresh is 0 in test_tcp_fast_rexmit_wraparound because we don't perform either an active/passive open, instead just manually setting the pcb state via tcp_set_state()

Then when we call test_tcp_input(), cwnd enters congestion avoidance because it is larger than ssthresh. Congestion avoidance doesn't grow the window large enough for a second segment to be sent in the tcp_output() call at the end of tcp_input()

With my change, we stay in slow start (due to large ssthresh) and cwnd grows large enough for multiple segments to be sent in tcp_output()

This could be fixed by moving ssthresh assignment back to the active/passive open spots, but I'm not sure. Maybe the unit tests should be manually setting ssthresh to meet expectations like it is doing for cwnd

Joel Cunningham <jcunningham>
Project MemberIn charge of this item.
Thu 09 Mar 2017 12:01:36 PM UTC, comment #10:

Joel, this broke the unit test "test_tcp_fast_rexmit_wraparound" (as indicated by https://travis-ci.org/yarrick/lwip-merged)

Simon Goldschmidt <goldsimon>
Project Administrator
Wed 08 Mar 2017 10:50:31 PM UTC, comment #9:

Patch committed in b90a54f989d0edafa36b29bb0c346d1b30e1bf8f

Ambroz thanks for the heads up on RFC 7661, I'll have to take a look at it

Joel Cunningham <jcunningham>
Project MemberIn charge of this item.
Wed 08 Mar 2017 07:44:06 PM UTC, comment #8:

Looks good, Joel!
Ambroz, you're right we don't do that (we just don't handle idle connections). OTOH, our IW calculation relies on RFC2581, equation 1, which might be outdated, too...

Simon Goldschmidt <goldsimon>
Project Administrator
Wed 08 Mar 2017 06:42:08 PM UTC, comment #7:

I would just like to mention RFC 7661 as something to look at for possible future improvements of congestion control (though it is quite complex!). It addresses issues such as CWND growing unreasonably high with a rate-limited sender and using a too high CWND after a reduction of data rate.

I think that lwIP doesn't even implement the idle period restart recommended by RFC 5681:

"Therefore, a TCP SHOULD set cwnd to no more than RW before beginning transmission if the TCP has not sent data in an interval exceeding the retransmission timeout."

Ambroz Bizjak <abizjak>
Wed 08 Mar 2017 03:19:52 AM UTC, comment #6:

Attached patch initializes ssthresh in tcp_alloc() and then removes all other initialize calls. This seemed simplest rather than having an assignment in both active open and passive open logic

Also, I re-read RFC 5681 section 3.1 and my understanding is that the guidance is to set ssthresh arbitrarily high. The RFC provides an example of using the largest advertised receive window, but that's not required. Thus, using TCP_SND_BUF (LwIP's largest effect cwnd) is well within that guidance :)

(file #39932)

Joel Cunningham <jcunningham>
Project MemberIn charge of this item.
Tue 07 Mar 2017 06:56:17 PM UTC, comment #5:

Cool. ssthresh can then be set after creating the pcb. Later assignments (apart from lowering it) can be removed.

Simon Goldschmidt <goldsimon>
Project Administrator
Tue 07 Mar 2017 06:19:39 PM UTC, comment #4:

Just to clarify my thoughts from previous post, but I think TCP_SND_BUF is a practical way to implement a good initial ssthresh which will ensure slow start can adequately probe for congestion up to the maximum in-flight data that an LwIP sender supports

Since sending is gated by checking both current receive window and congestion window, if congestion window happens to be larger than the advertised window, that's not a problem

I'll put together a patch and report back

Joel Cunningham <jcunningham>
Project MemberIn charge of this item.
Tue 07 Mar 2017 03:27:09 PM UTC, comment #3:

I think using TCP_SND_BUF would probably work great for LwIP since we won't ever have more data in-flight than what can fit in TCP_SND_BUF, regardless of whatever the receiver's window scales too. This is due to the fact that we don't increase pcb->snd_buf until we receive an ACK.

I believe the wording in RFC 5681 is referring to the window advertised by the receiver and TCP_SND_BUF is a sender side variable, which is not advertised as any window.

I'm wondering if to follow the spirit of the RFC, we should be updating the ssthresh when ever the receiver increases the window (to a new maximum) during auto-tuning, though as mentioned in the first paragraph, LwIP won't be able to use a congestion window larger than TCP_SND_BUF

Joel Cunningham <jcunningham>
Project MemberIn charge of this item.
Tue 07 Mar 2017 12:40:31 PM UTC, comment #2:

I "fixed" that in bug #44023 back in 2015. We used "mss * 10" before, which seemed kind of weird, and I haven't seen your problems back then. However, I'm not sure I tested active open against a window-scaling host...

In addition, lwIP was written before RFC 5681 was presented, and I don't think it has been updated according to it since 2009.

From reading RFC 5681 (and comparing to RFC 2581), what Ambroz suggests sounds OK to me.

Simon Goldschmidt <goldsimon>
Project Administrator
Tue 07 Mar 2017 12:17:11 PM UTC, comment #1:

I see that lwIP sets ssthresh to the advertised window (snd_wnd) when it receives the SYN-ACK. This must be causing lwIP to prematurely enter and remain in congestion avoidance.

(tcp_in.c)
/** Initial slow start threshold value: we use the full window */
#define LWIP_TCP_INITIAL_SSTHRESH(pcb) ((pcb)->snd_wnd)

RFC 5681 says:

"The initial value of ssthresh SHOULD be set arbitrarily high (e.g.,to the size of the largest possible advertised window), but ssthresh MUST be reduced in response to congestion. Setting ssthresh as high as possible allows the network conditions, rather than some arbitrary host limit, to dictate the sending rate."

Note that it says "largest possible advertised window", which is not the same as window advertised at SYN-ACK reception. Maybe the fix is:

#define LWIP_TCP_INITIAL_SSTHRESH(pcb) TCP_SND_BUF

Ambroz Bizjak <abizjak>
Tue 07 Mar 2017 05:54:23 AM UTC, original submission:

I've found through testing for task #14128, that when LwIP performs an active open with a Windows 7 host using auto-tuning receive window, LwIP ends up getting stuck with a 8192 value for ssthresh.

From wireshark captures, the following exchange shows that Windows 7 first advertises a 8192 window in the SYN-ACK, but then bumps up to 65536 (I have window scaling enabled in LwIP) in the first ACK following 3-way handshake. This leaves LwIP with an extremely lower slow-start threshold.

What's also interesting, is that in the passive open case, LwIP ends up with sstresh as 65536. Windows 7 reports a window of 8192 in the SYN and then a window of 65536 in the ACK for LwIP's SYN-ACK

I'm not 100% sure this is buggy behavior, but I do think it warrants investigation since active open ends up with a really low ssthresh in this case.

See the files in task #14128 to see the different ssthresh values for active vs passive open

Joel Cunningham <jcunningham>
Project MemberIn charge of this item.

 

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -unavailable- added by goldsimon (Posted a comment)
  • -unavailable- added by abizjak (Posted a comment)
  • -unavailable- added by jcunningham (Submitted the item)
  •  

    Do you think this task is very important?
    If so, you can click here to add your encouragement to it.
    This task has 0 encouragements so far.

    Only logged-in users can vote.

     

    Please enter the title of George Orwell's famous dystopian book (it's a date):

     

     

    Follow 6 latest changes.

    Date Changed By Updated Field Previous Value => Replaced By
    Thu 09 Mar 2017 10:55:10 PM UTCjcunninghamAttached File-=>Added 0001-Fix-unit-test-with-assumed-congestion-avoidance.patch, #39955
    Wed 08 Mar 2017 10:50:31 PM UTCjcunninghamStatusIn Progress=>Fixed
      Open/ClosedOpen=>Closed
    Wed 08 Mar 2017 03:19:52 AM UTCjcunninghamAttached File-=>Added 0001-bug-50476-initialize-ssthresh-to-TCP_SND_BUF.patch, #39932
    Tue 07 Mar 2017 06:19:39 PM UTCjcunninghamStatusNone=>In Progress
      Assigned toNone=>jcunningham

    Back to the top


    Powered by Savane 3.1-cleanup1