[PATCH net-next 0/4] RFC 4960 Errata fixes

[PATCH net-next 0/4] RFC 4960 Errata fixes

[Marcelo Ricardo Leitner]

This patchset contains fixes for 4 Errata topics from
https://tools.ietf.org/html/draft-ietf-tsvwg-rfc4960-errata-01
Namely, sections:
 3.12. Order of Adjustments of partial_bytes_acked and cwnd
 3.22. Increase of partial_bytes_acked in Congestion Avoidance
 3.26. CWND Increase in Congestion Avoidance Phase
 3.27. Refresh of cwnd and ssthresh after Idle Period

Tests performed with netperf using net namespaces, with drop rates at
0%, 0.5% and 1% by netem, IPv4 and IPv6, 10 runs for each combination.
I couldn't spot differences on the stats. With and without these patches
the results vary in a similar way in terms of throughput and
retransmissions.

Tests with 20ms delay and 20ms delay + drops at 0.5% and 1% also had
results in a similar way, no noticeable difference.

Looking at cwnd, it was possible to notice slightly lower values being
used while still sustaining same throughput profile.

Marcelo Ricardo Leitner (4):
  sctp: update order of adjustments of partial_bytes_acked and cwnd
  sctp: allow increasing cwnd regardless of ctsn moving or not
  sctp: adjust cwnd increase in Congestion Avoidance phase
  sctp: adjust ssthresh when transport is idle

 net/sctp/transport.c | 54 ++++++++++++++++++++++++++++++++--------------------
 1 file changed, 33 insertions(+), 21 deletions(-)

-- 
2.9.4

[PATCH net-next 1/4] sctp: update order of adjustments of partial_bytes_acked and cwnd

[Marcelo Ricardo Leitner]

RFC4960 Errata 3.12 says RFC4960 is unclear about the order of
adjustments applied to partial_bytes_acked and cwnd in the congestion
avoidance phase, and that the actual order should be:
partial_bytes_acked is reset to (partial_bytes_acked - cwnd). Next, cwnd
is increased by MTU.

We were first increasing cwnd, and then subtracting the new value pba,
which leads to a different result as pba is smaller than what it should
and could cause cwnd to not grow as much.

See-also: https://tools.ietf.org/html/draft-ietf-tsvwg-rfc4960-errata-01#section-3.12
Signed-off-by: Marcelo Ricardo Leitner <marcelo.leitner@gmail.com>
---
 net/sctp/transport.c | 15 ++++++++-------
 1 file changed, 8 insertions(+), 7 deletions(-)

diff --git a/net/sctp/transport.c b/net/sctp/transport.c
index 721eeebfcd8a50609877db61ede41575e012606a..04b6dd1a07ded25fe5874518b0944a6d9df4099b 100644
--- a/net/sctp/transport.c
+++ b/net/sctp/transport.c
@@ -452,17 +452,18 @@ void sctp_transport_raise_cwnd(struct sctp_transport *transport,
 		 * chunks acknowledged by the new Cumulative TSN Ack and by
 		 * Gap Ack Blocks.
 		 *
-		 * When partial_bytes_acked is equal to or greater than cwnd
-		 * and before the arrival of the SACK the sender had cwnd or
-		 * more bytes of data outstanding (i.e., before arrival of the
-		 * SACK, flightsize was greater than or equal to cwnd),
-		 * increase cwnd by MTU, and reset partial_bytes_acked to
-		 * (partial_bytes_acked - cwnd).
+		 * When partial_bytes_acked is equal to or greater than
+		 * cwnd and before the arrival of the SACK the sender
+		 * had cwnd or more bytes of data outstanding (i.e.,
+		 * before arrival of the SACK, flightsize was greater
+		 * than or equal to cwnd), partial_bytes_acked is reset
+		 * to (partial_bytes_acked - cwnd). Next, cwnd is
+		 * increased by MTU. (RFC 4960 Errata 3.12)
 		 */
 		pba += bytes_acked;
 		if (pba >= cwnd) {
+			pba = pba - cwnd;
 			cwnd += pmtu;
-			pba = ((cwnd < pba) ? (pba - cwnd) : 0);
 		}
 
 		pr_debug("%s: congestion avoidance: transport:%p, "
-- 
2.9.4

[PATCH net-next 2/4] sctp: allow increasing cwnd regardless of ctsn moving or not

[Marcelo Ricardo Leitner]

As per RFC4960 Errata 3.22, this condition is not needed anymore as it
could cause the partial_bytes_acked to not consider the TSNs acked in
the Gap Ack Blocks although they were received by the peer successfully.

This patch thus drops the check for new Cumulative TSN Ack Point,
leaving just the flight_size < cwnd one.

See-also: https://tools.ietf.org/html/draft-ietf-tsvwg-rfc4960-errata-01#section-3.22
Signed-off-by: Marcelo Ricardo Leitner <marcelo.leitner@gmail.com>
---
 net/sctp/transport.c | 17 ++++++++---------
 1 file changed, 8 insertions(+), 9 deletions(-)

diff --git a/net/sctp/transport.c b/net/sctp/transport.c
index 04b6dd1a07ded25fe5874518b0944a6d9df4099b..9d3589451a967a31ee241a5138a58f2f81a2f2a1 100644
--- a/net/sctp/transport.c
+++ b/net/sctp/transport.c
@@ -406,11 +406,10 @@ void sctp_transport_raise_cwnd(struct sctp_transport *transport,
 		asoc->fast_recovery = 0;
 
 	/* The appropriate cwnd increase algorithm is performed if, and only
-	 * if the cumulative TSN whould advanced and the congestion window is
-	 * being fully utilized.
+	 * if the congestion window is being fully utilized.
+	 * Note that RFC4960 Errata 3.22 removed the other condition.
 	 */
-	if (TSN_lte(sack_ctsn, transport->asoc->ctsn_ack_point) ||
-	    (flight_size < cwnd))
+	if (flight_size < cwnd)
 		return;
 
 	ssthresh = transport->ssthresh;
@@ -446,11 +445,11 @@ void sctp_transport_raise_cwnd(struct sctp_transport *transport,
 			 flight_size, pba);
 	} else {
 		/* RFC 2960 7.2.2 Whenever cwnd is greater than ssthresh,
-		 * upon each SACK arrival that advances the Cumulative TSN Ack
-		 * Point, increase partial_bytes_acked by the total number of
-		 * bytes of all new chunks acknowledged in that SACK including
-		 * chunks acknowledged by the new Cumulative TSN Ack and by
-		 * Gap Ack Blocks.
+		 * upon each SACK arrival, increase partial_bytes_acked
+		 * by the total number of bytes of all new chunks
+		 * acknowledged in that SACK including chunks
+		 * acknowledged by the new Cumulative TSN Ack and by Gap
+		 * Ack Blocks. (updated by RFC4960 Errata 3.22)
 		 *
 		 * When partial_bytes_acked is equal to or greater than
 		 * cwnd and before the arrival of the SACK the sender
-- 
2.9.4

[PATCH net-next 3/4] sctp: adjust cwnd increase in Congestion Avoidance phase

[Marcelo Ricardo Leitner]

RFC4960 Errata 3.26 identified that at the same time RFC4960 states that
cwnd should never grow more than 1*MTU per RTT, Section 7.2.2 was
underspecified and as described could allow increasing cwnd more than
that.

This patch updates it so partial_bytes_acked is maxed to cwnd if
flight_size doesn't reach cwnd, protecting it from such case.

See-also: https://tools.ietf.org/html/draft-ietf-tsvwg-rfc4960-errata-01#section-3.26
Signed-off-by: Marcelo Ricardo Leitner <marcelo.leitner@gmail.com>
---
 net/sctp/transport.c | 26 ++++++++++++++++++--------
 1 file changed, 18 insertions(+), 8 deletions(-)

diff --git a/net/sctp/transport.c b/net/sctp/transport.c
index 9d3589451a967a31ee241a5138a58f2f81a2f2a1..e3ebf04ddbd092a1ba04962c8fbe85a58588771a 100644
--- a/net/sctp/transport.c
+++ b/net/sctp/transport.c
@@ -405,13 +405,6 @@ void sctp_transport_raise_cwnd(struct sctp_transport *transport,
 	    TSN_lte(asoc->fast_recovery_exit, sack_ctsn))
 		asoc->fast_recovery = 0;
 
-	/* The appropriate cwnd increase algorithm is performed if, and only
-	 * if the congestion window is being fully utilized.
-	 * Note that RFC4960 Errata 3.22 removed the other condition.
-	 */
-	if (flight_size < cwnd)
-		return;
-
 	ssthresh = transport->ssthresh;
 	pba = transport->partial_bytes_acked;
 	pmtu = transport->asoc->pathmtu;
@@ -434,6 +427,14 @@ void sctp_transport_raise_cwnd(struct sctp_transport *transport,
 		if (asoc->fast_recovery)
 			return;
 
+		/* The appropriate cwnd increase algorithm is performed
+		 * if, and only if the congestion window is being fully
+		 * utilized.  Note that RFC4960 Errata 3.22 removed the
+		 * other condition on ctsn moving.
+		 */
+		if (flight_size < cwnd)
+			return;
+
 		if (bytes_acked > pmtu)
 			cwnd += pmtu;
 		else
@@ -451,6 +452,13 @@ void sctp_transport_raise_cwnd(struct sctp_transport *transport,
 		 * acknowledged by the new Cumulative TSN Ack and by Gap
 		 * Ack Blocks. (updated by RFC4960 Errata 3.22)
 		 *
+		 * When partial_bytes_acked is greater than cwnd and
+		 * before the arrival of the SACK the sender had less
+		 * bytes of data outstanding than cwnd (i.e., before
+		 * arrival of the SACK, flightsize was less than cwnd),
+		 * reset partial_bytes_acked to cwnd. (RFC 4960 Errata
+		 * 3.26)
+		 *
 		 * When partial_bytes_acked is equal to or greater than
 		 * cwnd and before the arrival of the SACK the sender
 		 * had cwnd or more bytes of data outstanding (i.e.,
@@ -460,7 +468,9 @@ void sctp_transport_raise_cwnd(struct sctp_transport *transport,
 		 * increased by MTU. (RFC 4960 Errata 3.12)
 		 */
 		pba += bytes_acked;
-		if (pba >= cwnd) {
+		if (pba > cwnd && flight_size < cwnd)
+			pba = cwnd;
+		if (pba >= cwnd && flight_size >= cwnd) {
 			pba = pba - cwnd;
 			cwnd += pmtu;
 		}
-- 
2.9.4

[PATCH net-next 4/4] sctp: adjust ssthresh when transport is idle

[Marcelo Ricardo Leitner]

RFC 4960 Errata 3.27 identifies that ssthresh should be adjusted to cwnd
because otherwise it could cause the transport to lock into congestion
avoidance phase specially if ssthresh was previously reduced by some
packet drop, leading to poor performance.

The Errata says to adjust ssthresh to cwnd only once, though the same
goal is achieved by updating it every time we update cwnd too. The
caveat is that we could take longer to get back up to speed but that
should be compensated by the fact that we don't adjust on RTO basis (as
RFC says) but based on Heartbeats, which are usually way longer.

See-also: https://tools.ietf.org/html/draft-ietf-tsvwg-rfc4960-errata-01#section-3.27
Signed-off-by: Marcelo Ricardo Leitner <marcelo.leitner@gmail.com>
---
 net/sctp/transport.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/net/sctp/transport.c b/net/sctp/transport.c
index e3ebf04ddbd092a1ba04962c8fbe85a58588771a..7cdd6bcddbc5ad277f525635cf56788f70a4ee30 100644
--- a/net/sctp/transport.c
+++ b/net/sctp/transport.c
@@ -569,6 +569,8 @@ void sctp_transport_lower_cwnd(struct sctp_transport *transport,
 		 */
 		transport->cwnd = max(transport->cwnd/2,
 					 4*asoc->pathmtu);
+		/* RFC 4960 Errata 3.27.2: also adjust sshthresh */
+		transport->ssthresh = transport->cwnd;
 		break;
 	}
 
-- 
2.9.4

Re: [PATCH net-next 0/4] RFC 4960 Errata fixes

[David Miller]

From: Marcelo Ricardo Leitner <marcelo.leitner@gmail.com>
Date: Fri, 23 Jun 2017 19:58:39 -0300

> This patchset contains fixes for 4 Errata topics from
> https://tools.ietf.org/html/draft-ietf-tsvwg-rfc4960-errata-01
> Namely, sections:
>  3.12. Order of Adjustments of partial_bytes_acked and cwnd
>  3.22. Increase of partial_bytes_acked in Congestion Avoidance
>  3.26. CWND Increase in Congestion Avoidance Phase
>  3.27. Refresh of cwnd and ssthresh after Idle Period
> 
> Tests performed with netperf using net namespaces, with drop rates at
> 0%, 0.5% and 1% by netem, IPv4 and IPv6, 10 runs for each combination.
> I couldn't spot differences on the stats. With and without these patches
> the results vary in a similar way in terms of throughput and
> retransmissions.
> 
> Tests with 20ms delay and 20ms delay + drops at 0.5% and 1% also had
> results in a similar way, no noticeable difference.
> 
> Looking at cwnd, it was possible to notice slightly lower values being
> used while still sustaining same throughput profile.

Series applied, thank you.