Add support for TCP ABE draft-khademi-tcpm-alternativebackoff-ecn

[thj]

Summary

This patch adds support Alternative Backoff ECN for TCP with the New Reno
congestion control module. TCP ABE is described in
https://tools.ietf.org/html/draft-khademi-tcpm-alternativebackoff-ecn-01

ABE updates the TCP sender-side reaction to congestion notification received
via Explicit Congestion Notifiction (ECN) marks. From the Abstract in the
draft:

The updated method reduces FlightSize in Congestion Avoidance by a
smaller amount than the TCP reaction to loss. The intention is to
achieve good throughput when the queue at the bottleneck is smaller
than the bandwidth-delay-product of the connection. This is more
likely when an Active Queue Management (AQM) mechanism has used ECN
to CE-mark a packet, than when a packet was lost.

This patch adds 3 new sysctl values, one to control the use of ABE and two new
cc controls under net.inet.tcp.cc.newreno to control the loss and ecn response
factors. These controls are intended to aid future experimentation and research
in this area.

net.inet.tcp.abe:

Toggles whether a transport uses the new abe conditions. Currently only
implemented for New Reno.

net.inet.tcp.cc.newreno.beta_ecn:

The factor used to change FlightSize in response to an ecn mark, defaults
to 0.8 as described in the draft.

net.inet.tcp.cc.newreno.beta_loss:

The factor used to change FlightSize in response to a loss event, defaults
to 0.5 as described in the draft.

[lstewart]

I'm pretty sure this wouldn't compile as proposed. Please remember to build test patches against FreeBSD's svn head branch, as all work always gets comitted to head first before potentially being backported later.

sys/netinet/cc/cc_newreno.c

52

If this is supposed to be an actual copyright attribution, you should add the appropriate year to the existing set attributed to Swinburne University in the main copyright block, and then extend the second informative comment block with these additional author attribution details.

That being said, given the size of the change it probably doesn't warrant copyright changes, so merging this in to the informative comment block is probably all that's needed.

80

This #ifndef block is not needed

91

What about treating these as permanently defined variables which default to non-ABE behaviour i.e. 50 and 50, and toggling ABE simply explicitly sets them to draft-specified values? Perhaps renaming newreno_beta_loss to newreno_beta would also make sense, which specifies the default beta that gets overridden by other variables on a case by case basis.

198

Let's avoid gratuitous code duplication, and perhaps reinstate this line as something like:

factor = (type == CC_ECN ? V_newreno_beta_ecn : V_newreno_beta_loss);
cwin = max((cwin * factor) / (100 * mss), 2) * mss;

290

This can change to sysctl_handle_int()

[thj]

Address Copyright comment. Bump Swinburne copyright year, keep the comment from Grenville about changes.

Adjusts the descriptive text for sysctls to make it clearer and more rfc friendly.

Address comments from lstewart

[thj]

Address potential overflow when computing cwnd.

Add infrastructure to allow setting of beta values from either a sysctl or via a setsockopt call.

[lstewart]

This looks good barring a couple of nits as noted. Please update tcp(4) and cc_newreno(4) appropriately.

sys/netinet/cc/cc_newreno.c

374

Please note in the sysctl description that net.inet.tcp.abe must be 1 for this variable to be settable.

sys/netinet/tcp_input.c

187

Perhaps include name of ABE draft in description, or at minimum in the file as a comment.

[thj]

Make sysctl comments clearer.

[thj]

Update tcp(4) and cc_newreno(4)

[lstewart]

share/man/man4/cc_newreno.4

40 ↗

(On Diff #31257)

Please do the following:

• Bump the document date .Dd field.

• Add a "SEE ALSO" section with a reference to the ABE draft (as an example, you can copy from cc_cubic for draft-rhee-tcpm-cubic-02.txt). While you're at it, perhaps also add a normative reference to RFC 5681 as the first ref in SEE ALSO.

• Add a new ".Ss Socket options" sub-section above the "MIB Variables" sub-section to document "struct cc_newreno_opts" (see mod_cc(9) for an example of documenting a struct in a man page) and to document the 2 socket options added. You should also reference TCP_CCALGOOPT from tcp(4) and mod_cc(4).

FYI, "man mdoc" has details on all the mdoc macros you might need.

48 ↗

(On Diff #31257)

You should use the longest MIB variable name here for width sizing i.e. beta_ecn

51 ↗

(On Diff #31257)

I wonder where these were copied from... please be mindful of paying more attention to detail.

How about (note man page style is to start new sentences on a new line):

Multiplicative window decrease factor, specified as a percentage, applied to the congestion window in response to a congestion signal per: cwnd = (cwnd * beta) / 100.
Default is 50.

56 ↗

(On Diff #31257)

ABE should be explicitly referenced here. Perhaps something like:

Multiplicative window decrease factor, specified as a percentage, applied to the congestion window in response to an ECN congestion signal when
.Va net.inet.tcp.abe=1
per: cwnd = (cwnd * beta_ecn) / 100.
Default is 80.

59 ↗

(On Diff #31257)

Feel free to give yourself/the funding project some kudos in the "HISTORY" and/or "AUTHORS" sections for adding ABE support if you wish.

share/man/man4/tcp.4

494 ↗

(On Diff #31257)

Perhaps be a bit more descriptive and point people in the right direction:

..., which alters the window decrease factor applied to the congestion window in response to an ECN congestion signal.
Refer to
.Xr 4 mod_cc
and individual congestion control man pages to determine if they implement support for ABE and for configuration details.

sys/netinet/cc/cc_newreno.h

3

If this is sponsored work, you may need to check whether you retain copyright, or the funding project/institution does. Normally the body paying for your time owns and should be assigned the copyright, and you should assign yourself author attribution in a comment block below the main copyright block.

37

Nit: spacing looks off here. Make sure there is a hard tab between type and name so that names align on same column.

[thj]

Address nit comments.

Document setting beta values from socket options.

[lstewart]

Thanks Tom, looking good. Will wait a few days to see if any further feedback materialises.

[thj]

Fix changes cause by r321480

[thj]

Capture the struct newreno point in cc_var.

In ctl_output:

• make sure newreno isn't null before continuing
• break from SOPT case so we don't return EINVAL

[thj]

add hook for cb_destroy to recover struct newreno allocation in cb_init

[wblock]

share/man/man4/cc_newreno.4

58 ↗

(On Diff #32966)

socket options use this structure defined in

59 ↗

(On Diff #32966)

<sys/netinet/cc/cc_newreno.h>:

79 ↗

(On Diff #32966)

s/the following/these/

[thj]

Fix merge conflict

Address man page review comments from @wblock
Bump date on man page
Bump date on cc_newreno.h