Opened 2 years ago

Closed 23 months ago

#13629 closed task (fixed)

tcp: slow start & congestion avoidance @ rfc 5681 : updated rules for congestion window

Reported by: a-star Owned by: axeld
Priority: normal Milestone: Unscheduled
Component: Network & Internet/TCP Version: R1/Development
Keywords: tcp, gsoc, slow start Cc:
Blocked By: Blocking:
Has a Patch: yes Platform: All

Description

Implementing the following changes specified in rfc 5681:

1) rfc 5681 states that the initial size of the congestion window (IW) be calculated as follows:

If SMSS > 2190 bytes:

IW = 2 * SMSS bytes and MUST NOT be more than 2 segments

If (SMSS > 1095 bytes) and (SMSS <= 2190 bytes):

IW = 3 * SMSS bytes and MUST NOT be more than 3 segments

if SMSS <= 1095 bytes:

IW = 4 * SMSS bytes and MUST NOT be more than 4 segments

2) It defines RMSS (Receiver Maximum Segment Size) to be the value specified in the MSS option sent by the receiver during connection startup and says that this size “does not include the TCP/IP headers and options”.

3) The value of congestion window should be updated by “every incoming ACK that acknowledges new data”.

4) The SYN/ACK and the acknowledgment of the SYN/ACK MUST NOT increase the size of the congestion window. Further, if the SYN or SYN/ACK is lost, the initial window used by a sender after a correctly transmitted SYN MUST be one segment consisting of at most SMSS bytes.

Change History (17)

comment:1 Changed 2 years ago by a-star

Has a Patch: set

comment:2 Changed 2 years ago by phoudoin

Trying to review your patch. Question: where and how do you implement the "MUST NOT be more than X segment" rule as defined above?

comment:3 Changed 2 years ago by a-star

I was actually a bit confused with how to implement that rule without adding a new variable to the TCP control block.

I haven't been able to come up with such an idea. I am submitting another patch that implements that rule by extending the TCP control block with a fSendMaxSegments variable. Should this variable reach a value of zero, the _ShouldSendSegment (called in _SendQueued) will return false.

It would be great if you could give me an idea on how to implement it without adding a new variable or if it's ok to add a new varible.

comment:4 Changed 2 years ago by phoudoin

Adding a new variable pose no problem. This feature plays with both segment max size *and* segment max number, there is no choice but to keep track of segment number left somewhere.

I'm more concerning by the UINT32_MAX magic value that, even if it will take time, could becomes 0. I'm not sure to understand if it's a ticking code bomb or what.

I confess I'm not enough aware of TCP congestion mechanisms yet to do actual check of your patches. Consider it as a candide proof reading atm.

comment:5 Changed 2 years ago by a-star

UINT32_MAX shouldn't be a problem since the congestion window variable, fCongestionWindow, is also uint32.

Till the first ACK after connection establishment is received, fSendMaxSegments will limit the number of packets sent according to the "MUST NOT be more than X" rule specified above. After that, on receipt of every ACK that changes the congestion window, fSendMaxSegments counter is restarted with UINT32_MAX.

Now let's consider the worst case scenario: the user continuously sends packet of 1 byte in length and the other end doesn't send back acknowledgment for a long time. Each of these packets will decrement the value of fSendMaxSegments by 1.

[Note: Packets of zero length, like pure ACK or packets exchanged during handshake do not decrement the value of fSendMaxSegments.]

Now for fSendMaxSegments to reduce to 0, the minimum number of such packets required will be UINT32_MAX. The total amount of data sent will then be UINT32_MAX * 1 byte = UINT32_MAX bytes. This value is also the maximum size of the congestion window. Therefore if indeed this much data is sent, the congestion window will itself withhold any more data from being sent. So we have no problem with fSendMaxSegments getting reduced to zero.

comment:6 Changed 2 years ago by phoudoin

thanks for clearing that up.

comment:7 Changed 2 years ago by axeld

Is there a specific reason you moved the TRACE() in _Retransmit() into some if-block? It should always be printed IMO.

comment:8 Changed 2 years ago by axeld

BTW in what order do I have to apply your commits? After I apply the above (2nd try), I cannot apply the one in #13630 anymore.

comment:9 Changed 2 years ago by a-star

I was working with multiple branches and forgot to rebase. I have fixed the patches now. They can applied in the increasing order of the issue no.

comment:10 Changed 2 years ago by jessicah

This particular patch appears to entirely break TCP. It seems unable to establish a TCP connection from my observations. It seems like connect() gets stuck?

comment:11 Changed 2 years ago by jessicah

I'm wondering if fSendMaxSegments isn't set to a usable value initially. I'll see if can add some tracing, dig a bit deeper after testing a couple other patches.

comment:12 Changed 2 years ago by a-star

My mistake. I was only running tests where Haiku was acting like a server. Therefore I missed out on a check for fState == ESTABLISHED to be used in conjunction with fSendMaxSegments == 0 in _ShouldSendSegment.

Applied the fix and replaced the buggy patch.

comment:13 Changed 2 years ago by a-star

Reverting the change in _SendQueued (point 2 in the description of ticket) to the original implementation which deducted the size of tcp options from fSendMaxSegmentSize.

I inspected the outgoing packets from Ubuntu as well as Windows. Both of them upon receiving a RMSS of 1460 sent out packets of length 1448 only, i.e., subtracted the 12 bytes of TCP timestamp option.

Although the rfc tells us that the size doesn't include the length of options but it is only realistic to subtract the size of options since while negotiating MSS during handshake one cannot know in advance what tcp options will be used. All packets will use timestamp once negotiated but some may carry extra information such as SACK as well.

Should have noted that in the first place. :)

comment:14 Changed 23 months ago by jessicah

Resolution: fixed
Status: newclosed

Applied in hrev51385.

Note: See TracTickets for help on using tickets.