Page MenuHomeFreeBSD

Update <cc>_after_idle to take initcwnd_segments into account.
AbandonedPublic

Authored by hiren on Jan 29 2016, 10:40 PM.

Details

Reviewers
lstewart
gnn
Summary

Make congestion control algorithms take initcwnd_segments value into account after resuming from idle state while trying to decide the cwnd value.

Updating only newreno one as that gets used by other CCs.

Diff Detail

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

Event Timeline

hiren updated this revision to Diff 12846.Jan 29 2016, 10:40 PM
hiren retitled this revision from to Make congestion control algorithms take initcwnd_segments value into account after resuming from idle state while trying to decide the cwnd value..
hiren updated this object.
hiren edited the test plan for this revision. (Show Details)
hiren added reviewers: gnn, lstewart.
hiren added a subscriber: transport.
hiren retitled this revision from Make congestion control algorithms take initcwnd_segments value into account after resuming from idle state while trying to decide the cwnd value. to Update <cc>_after_idle to take initcwnd_segments into account. .Jan 29 2016, 10:42 PM
hiren updated this object.
lstewart edited edge metadata.Feb 1 2016, 5:27 AM

I would suggest that the code to handle RFC3390 should be merged with the new code i.e. the net.inet.tcp.rfc3390 sysctl should become a SYSCTL_PROC and simply set V_tcp_initcwnd_segments=3 behind the scenes, and return the evaluated result of "V_tcp_initcwnd_segments==3" as the sysctl value.

Oops, that should of course be 4 segments, not 3 (though recall that we really need an initcwnd_bytes variable as well in order to fully capture the spirit of the RFC 3390 and later RFCs - something perhaps you can add as part of this work).

hiren added a comment.Feb 2 2016, 2:19 AM

@lstewart I agree and I think its time to improve the initcwnd handling code. But that'd be a separate commit.

What is your take on the problem at hand? Are you okay with the diffs? I'd like to get this in and possibly MFC for 10.3.

Now, coming to the cleanup/improvement part:
I see what you are suggesting wrt RFC3390 sysctl but I guess bigger win wrt cleanup may come from creating a separate function to calculate initial cwnd (lets say tcp_initcwnd() ) and call that from cc_cong_init() and newreno_after_idle().

tcp_initcwnd() would return snd_cwnd based on different rfcs and what not.

About cwnd_bytes, iirc, we discussed something along the lines of having a similar sysctl named initcwnd_bytes where you can specify initial congestion window in bytes.
max (initcwnd_segsments * tp->t_maxseg, initicwnd_bytes)

Now how should we handle the new sysctls default value? should we initialize it to 14600 as we initialize initcwnd_segments to 10?

I'd like to start discussing this on separate review/commit.

In D5124#109833, @hiren wrote:

@lstewart I agree and I think its time to improve the initcwnd handling code. But that'd be a separate commit.
What is your take on the problem at hand? Are you okay with the diffs? I'd like to get this in and possibly MFC for 10.3.

The diff doesn't make a whole lot of sense to me in the current world order with defaults net.inet.tcp.rfc3390=1 and net.inet.tcp.initcwnd_segments=10 given that they are in effect mutually exclusive, and setting net.inet.tcp.initcwnd_segments=0 in order to get 3390 behaviour is just plain bizarre.

Now, coming to the cleanup/improvement part:
I see what you are suggesting wrt RFC3390 sysctl but I guess bigger win wrt cleanup may come from creating a separate function to calculate initial cwnd (lets say tcp_initcwnd() ) and call that from cc_cong_init() and newreno_after_idle().
tcp_initcwnd() would return snd_cwnd based on different rfcs and what not.
About cwnd_bytes, iirc, we discussed something along the lines of having a similar sysctl named initcwnd_bytes where you can specify initial congestion window in bytes.
max (initcwnd_segsments * tp->t_maxseg, initicwnd_bytes)
Now how should we handle the new sysctls default value? should we initialize it to 14600 as we initialize initcwnd_segments to 10?
I'd like to start discussing this on separate review/commit.

Ok well I'll draw a line under it in this review for the moment until we discuss elsewhere.

hiren abandoned this revision.EditedFeb 3 2016, 7:43 PM

https://reviews.freebsd.org/D5173 is what is in the works now.

I still suggest we should fix 10.3R to not have this particular problem.

I patched our inhouse tree with something like this for stable/10:

--- a/sys/netinet/cc/cc_newreno.c
+++ b/sys/netinet/cc/cc_newreno.c
@@ -166,7 +166,10 @@ newreno_after_idle(struct cc_var *ccv)
 	 *
 	 * See RFC5681 Section 4.1. "Restarting Idle Connections".
 	 */
-	if (V_tcp_do_rfc3390)
+	if (V_tcp_do_initcwnd10)
+		rw = min(10 * CCV(ccv, t_maxseg),
+		    max(2 * CCV(ccv, t_maxseg), 14600));
+	else if (V_tcp_do_rfc3390)
 		rw = min(4 * CCV(ccv, t_maxseg),
 		    max(2 * CCV(ccv, t_maxseg), 4380));
 	else