Page MenuHomeFreeBSD

Add cwnd and ssthresh recommendations to RFC 6675 support. While here, unify everything under one sysctl knob.
Needs ReviewPublic

Authored by hiren on Apr 26 2016, 12:34 AM.

Details

Reviewers
rrs
jtl
lstewart
gnn
Group Reviewers
transport
Summary

Follow RFC6675 for setting sshthresh and cwnd at fast retransmit.

As discussed on the transport call, this would let people experiment.

Diff Detail

Repository
rS FreeBSD src repository
Lint
Lint OK
Unit
No Unit Test Coverage
Build Status
Buildable 3843
Build 3886: arc lint + arc unit

Event Timeline

hiren updated this revision to Diff 15609.Apr 26 2016, 12:34 AM
hiren retitled this revision from to Follow RFC6675 for setting sshthresh and cwnd at fast retransmit..
hiren updated this object.
hiren edited the test plan for this revision. (Show Details)
hiren added reviewers: lstewart, rrs, jtl, gnn.
hiren added a subscriber: transport.
hiren updated this object.Apr 26 2016, 12:35 AM
hiren removed a reviewer: transport.
hiren removed a subscriber: imp.

Bump!
anyone?

lstewart edited edge metadata.Apr 29 2016, 1:21 AM

Why isn't there simply a do_rfc6675 knob that supersedes this and the previously committed work?

hiren added a comment.Apr 29 2016, 7:28 AM

Why isn't there simply a do_rfc6675 knob that supersedes this and the previously committed work?

For more granular control. We can merge them all into one later once we are satisfied with the behavior.

But why do we need such finely grained control? I don't get it. Either it works or it doesn't. We shouldn't be doing 6675 piecemeal. We should be doing 6675 in full and enabled by default. Providing any level of minutiae beyond enabled/disabled is not only unnecessary but a bad idea IMO.

hiren added a comment.Apr 29 2016, 8:01 AM
This comment was removed by hiren.
hiren added a comment.Apr 29 2016, 8:07 AM

But why do we need such finely grained control? I don't get it. Either it works or it doesn't. We shouldn't be doing 6675 piecemeal. We should be doing 6675 in full and enabled by default. Providing any level of minutiae beyond enabled/disabled is not only unnecessary but a bad idea IMO.

iirc, @rrs tried the patch and it didn't work well for his workload. I
am just trying to avoid such situations.

Having said that, I know putting it under one knob looks more correct.

In D6105#130864, @hiren wrote:

But why do we need such finely grained control? I don't get it. Either it works or it doesn't. We shouldn't be doing 6675 piecemeal. We should be doing 6675 in full and enabled by default. Providing any level of minutiae beyond enabled/disabled is not only unnecessary but a bad idea IMO.

iirc, @rrs tried the patch and it didn't work well for his workload. I
am just trying to avoid such situations.

That's good reasoning for more testing, not good reasoning for piecemeal implementation and control knobs.

Having said that, I know putting it under one knob looks more correct.

s/looks/is :)

jtl edited edge metadata.Apr 29 2016, 1:01 PM
In D6105#130864, @hiren wrote:

Having said that, I know putting it under one knob looks more correct.

s/looks/is :)

FWIW, I agree with @lstewart; however, I think we should avoid an unqualified "RFC 6675" name until we actually implement all of RFC 6675. Maybe I'll actually get to the rest of it this summer.

Regarding the patch, I'm curious why you chose the formula you did on one 2644-2645. The RFC refers to "Flight Size" from RFC 5681, which is not necessarily the same thing as "pipe". Also, the full formula from RFC 5681 is:

ssthresh = max (FlightSize / 2, 2*SMSS)

It isn't clear to me why the 2*SMSS floor was dropped from RFC 6675's restatement.

hiren added a comment.Apr 29 2016, 3:54 PM
In D6105#130904, @jtl wrote:
In D6105#130864, @hiren wrote:

Having said that, I know putting it under one knob looks more correct.

s/looks/is :)

FWIW, I agree with @lstewart; however, I think we should avoid an unqualified "RFC 6675" name until we actually implement all of RFC 6675. Maybe I'll actually get to the rest of it this summer.

I've been testing with these things and I wanted broader testing from others in the community. If we don't want to go ahead with this, I can abandon this rev and we'd still have patch available with granular knobs for people to test/verify.

Once we have full RFC6675 support, we can come back and do it.

I am fine either-ways. :-)

Regarding the patch, I'm curious why you chose the formula you did on one 2644-2645. The RFC refers to "Flight Size" from RFC 5681, which is not necessarily the same thing as "pipe".

https://mailarchive.ietf.org/arch/msg/tcpm/515zSdL2uD77ezV4-MDnoshDNbI

Also, the full formula from RFC 5681 is:
ssthresh = max (FlightSize / 2, 2*SMSS)
It isn't clear to me why the 2*SMSS floor was dropped from RFC 6675's restatement.

I'd also have to look.

hiren updated this revision to Diff 16595.May 19 2016, 10:11 PM
hiren edited edge metadata.

Add cwnd and ssthresh recommendations to RFC 6675 support. While here, unify everything under one sysctl knob.

hiren retitled this revision from Follow RFC6675 for setting sshthresh and cwnd at fast retransmit. to Add cwnd and ssthresh recommendations to RFC 6675 support. While here, unify everything under one sysctl knob..May 19 2016, 10:12 PM
hiren edited edge metadata.
hiren added a comment.May 24 2016, 8:25 AM

@lstewart , @jtl

Can you guys see if this approach looks okay?

jtl added a comment.May 25 2016, 1:04 AM
In D6105#138167, @hiren wrote:

@lstewart , @jtl
Can you guys see if this approach looks okay?

I don't have time to fully think about this today; perhaps, we can discuss it in Ottawa.

I'm on the fence about the sysctl name. I also need to think further about "flight size" vs. "pipe". I read the IETF mailing list discussion, but I don't think it directly answered the question. (But, it may just be lacking some thought from me.)

hiren added a comment.Jun 9 2016, 3:59 AM

At hackers lounge BSDCan, @jtl and I discussed this and realized that I was incorrectly interpreting rfc.

Let's try to decompose whats being proposed by RFCs:

RFC6675 says:
DupAcks >= DupThresh,
(4.2) ssthresh = cwnd = (FlightSize / 2)
The congestion window (cwnd) and slow start threshold (ssthresh) are reduced to half of FlightSize per [RFC5681].

(4.3) Retransmit the first data segment presumed dropped

(4.4) Set pipe.

Now, looking at RFC5681

FLIGHT SIZE: The amount of data that has been sent but not yet cumulatively acknowledged. i.e. SND.MAX - SND.UNA

In 3.2. Fast Retransmit/Fast Recovery

  1. When the third duplicate ACK is received, a TCP MUST set ssthresh to no more than the value given in equation (4). When [RFC3042] is in use, additional data sent in limited transmit MUST NOT be included in this calculation.

Eq (4) ssthresh = max (FlightSize / 2, 2*SMSS)

  1. The lost segment starting at SND.UNA MUST be retransmitted and cwnd set to ssthresh plus 3*SMSS. This artificially "inflates" the congestion window by the number of segments (three) that have left the network and which the receiver has buffered.

i.e. if you want to follow RFC5681 strictly, it says, on 3rd dupack, set sshthresh and after retransmitting first lost seg, set cwnd. Which differs from what RFC6675 says.

I'll try and update the code tomorrow.