[PATCH] tcp: avoid cwnd moderation in undo

[PATCH] tcp: avoid cwnd moderation in undo

[Yuchung Cheng]

In the current undo logic, cwnd is moderated after it was restored
to the value prior entering fast-recovery. It was moderated first
in tcp_try_undo_recovery then again in tcp_complete_cwr.

Since the undo indicates recovery was false, these moderations
are not necessary. If the undo is triggered when most of the
outstanding data have been acknowledged, the (restored) cwnd is
falsely pulled down to a small value.

This patch removes these cwnd moderations if cwnd is undone during
the fast-recovery.

Signed-off-by: Yuchung Cheng <ycheng@google.com>
---
 net/ipv4/tcp_input.c |   12 +++++++-----
 1 files changed, 7 insertions(+), 5 deletions(-)

diff --git a/net/ipv4/tcp_input.c b/net/ipv4/tcp_input.c
index 08ea735..1bd0f38 100644
--- a/net/ipv4/tcp_input.c
+++ b/net/ipv4/tcp_input.c
@@ -2659,7 +2659,7 @@ static void DBGUNDO(struct sock *sk, const char *msg)
 #define DBGUNDO(x...) do { } while (0)
 #endif
 
-static void tcp_undo_cwr(struct sock *sk, const int undo)
+static void tcp_undo_cwr(struct sock *sk, const int undo_ssthresh)
 {
 	struct tcp_sock *tp = tcp_sk(sk);
 
@@ -2671,14 +2671,13 @@ static void tcp_undo_cwr(struct sock *sk, const int undo)
 		else
 			tp->snd_cwnd = max(tp->snd_cwnd, tp->snd_ssthresh << 1);
 
-		if (undo && tp->prior_ssthresh > tp->snd_ssthresh) {
+		if (undo_ssthresh && tp->prior_ssthresh > tp->snd_ssthresh) {
 			tp->snd_ssthresh = tp->prior_ssthresh;
 			TCP_ECN_withdraw_cwr(tp);
 		}
 	} else {
 		tp->snd_cwnd = max(tp->snd_cwnd, tp->snd_ssthresh);
 	}
-	tcp_moderate_cwnd(tp);
 	tp->snd_cwnd_stamp = tcp_time_stamp;
 }
 
@@ -2822,8 +2821,11 @@ static int tcp_try_undo_loss(struct sock *sk)
 static inline void tcp_complete_cwr(struct sock *sk)
 {
 	struct tcp_sock *tp = tcp_sk(sk);
-	tp->snd_cwnd = min(tp->snd_cwnd, tp->snd_ssthresh);
-	tp->snd_cwnd_stamp = tcp_time_stamp;
+	/* Do not moderate cwnd if it's already undone in cwr or recovery */
+	if (tp->undo_marker && tp->snd_cwnd > tp->snd_ssthresh) {
+		tp->snd_cwnd = tp->snd_ssthresh;
+		tp->snd_cwnd_stamp = tcp_time_stamp;
+	}
 	tcp_ca_event(sk, CA_EVENT_COMPLETE_CWR);
 }
 
-- 
1.7.3.1

Re: [PATCH] tcp: avoid cwnd moderation in undo

[Carsten Wolff]

Hi,

On Saturday 12 March 2011, Yuchung Cheng wrote:
> In the current undo logic, cwnd is moderated after it was restored
> to the value prior entering fast-recovery. It was moderated first
> in tcp_try_undo_recovery then again in tcp_complete_cwr.
> 
> Since the undo indicates recovery was false, these moderations
> are not necessary. If the undo is triggered when most of the
> outstanding data have been acknowledged, the (restored) cwnd is
> falsely pulled down to a small value.
> 
> This patch removes these cwnd moderations if cwnd is undone during
> the fast-recovery.

The moderation is in place to avoid gigantic segment bursts, which could cause 
unnecessary pressure on buffers. In my eyes it's already suboptimal that the 
moderation is weakened in the presence of (detected) reordering, let alone 
removing it completely.

More importantly, the prior ssthresh is restored and not affected by 
moderation. This means, if moderation reduces cwnd to a small value, then cwnd 
< ssthresh and TCP will quickly slow-start back to the previous state, without 
sending a big burst of segments.

Also, you intended to remove cwnd moderation only from an undo during 
recovery, but I think your patch also removes cwnd moderation when the undo is 
caused by D-SACK, i.e. most likely after recovery already ended.

Carsten
-- 
           /\-´-/\
          (  @ @  )
________o0O___^___O0o________

Re: [PATCH] tcp: avoid cwnd moderation in undo

[Yuchung Cheng]

On Mon, Mar 14, 2011 at 3:06 AM, Carsten Wolff <carsten@wolffcarsten.de> wrote:
> The moderation is in place to avoid gigantic segment bursts, which could cause
> unnecessary pressure on buffers. In my eyes it's already suboptimal that the
> moderation is weakened in the presence of (detected) reordering, let alone
> removing it completely.

In the presence of reordering, cwnd is already moderated in Disorder
state before
 entering the (false) recovery.


>
> More importantly, the prior ssthresh is restored and not affected by
> moderation. This means, if moderation reduces cwnd to a small value, then cwnd
> < ssthresh and TCP will quickly slow-start back to the previous state, without
> sending a big burst of segments.
>
> Also, you intended to remove cwnd moderation only from an undo during
> recovery, but I think your patch also removes cwnd moderation when the undo is
> caused by D-SACK, i.e. most likely after recovery already ended.

Thanks. I will update my patch description. But the same principle
applies that cwnd
should not be moderated on false events. Whether it should be moderated on
reordering or other events is another (complex) design issue. But this
patch does not
touch that.

Yuchung

Re: [PATCH] tcp: avoid cwnd moderation in undo

[John Heffner]

On Mon, Mar 14, 2011 at 3:10 PM, Yuchung Cheng <ycheng@google.com> wrote:
> On Mon, Mar 14, 2011 at 3:06 AM, Carsten Wolff <carsten@wolffcarsten.de> wrote:
>> The moderation is in place to avoid gigantic segment bursts, which could cause
>> unnecessary pressure on buffers. In my eyes it's already suboptimal that the
>> moderation is weakened in the presence of (detected) reordering, let alone
>> removing it completely.
>
> In the presence of reordering, cwnd is already moderated in Disorder
> state before
>  entering the (false) recovery.

I've always been somewhat skeptical of the usefulness of cwnd
moderation.  First, I don't know that its behavior is well defined.
When *should* tcp_moderate_cwnd() actually be called, and why?

Second, I've never liked the idea in general.  Reducing cwnd has an
effect lasting many RTTs, so reducing it in response to a transient
event like reordering seems dubious.  And it does not address many
causes of bursts, such as ack compression or stretch acks.

  -John

Re: [PATCH] tcp: avoid cwnd moderation in undo

[Carsten Wolff]

On Monday 14 March 2011, John Heffner wrote:
> On Mon, Mar 14, 2011 at 3:10 PM, Yuchung Cheng <ycheng@google.com> wrote:
> > On Mon, Mar 14, 2011 at 3:06 AM, Carsten Wolff <carsten@wolffcarsten.de> 
wrote:
> >> The moderation is in place to avoid gigantic segment bursts, which could
> >> cause unnecessary pressure on buffers. In my eyes it's already
> >> suboptimal that the moderation is weakened in the presence of
> >> (detected) reordering, let alone removing it completely.
> > 
> > In the presence of reordering, cwnd is already moderated in Disorder
> > state before
> >  entering the (false) recovery.
> 
> I've always been somewhat skeptical of the usefulness of cwnd
> moderation.  First, I don't know that its behavior is well defined.
> When *should* tcp_moderate_cwnd() actually be called, and why?
> 
> Second, I've never liked the idea in general.  Reducing cwnd has an
> effect lasting many RTTs, so reducing it in response to a transient
> event like reordering seems dubious.  And it does not address many
> causes of bursts, such as ack compression or stretch acks.

I think the undo operations are a special case where it absolutely makes 
sense, because the cwnd _already_ has been reduced (down to 1 segment by RTO 
recovery, or, with its special mixture of ratehalving and newreno, Linux 
sometimes reduces cwnd far below half of it even in fast recovery.). The undo 
re-opens the window all at once, which may allow _really huge_ bursts. I don't 
know enough about the other causes of TCPs burstiness, I'm just concerned 
about allowing this special cause.

Carsten
-- 
           /\-´-/\
          (  @ @  )
________o0O___^___O0o________

Re: [PATCH] tcp: avoid cwnd moderation in undo

[John Heffner]

Anecdotally, I've seen more harm than good from cwnd moderation but I
don't have any data to back that up.  It's hard to categorically say
anything.  But I don't know what makes the cases where cwnd moderation
is applied special.

Some other sources of bursts include
- Send-limited applications that stall for some time but less than an
RTO and do a large write
- Receive-limited applications that stall for some time but less than
an RTO and do a large read
- "Ack compression," when acks are queued somewhere in the network and
flood out at line rate
- "Stretch acks," when the receiving TCP does not ack every segment
(for efficiency, etc.), or acks are lost in the network.

Since all of these may occur in the Open state, cwnd moderation is not applied.

While I'm adding to my wish list ;-), I will also say that the undo
logic seems unnecessarily complicated.  If you expect you might have
to undo what you just did, why not wait a bit?  Something like rfc4653
seems like it might be a better approach and might make the cwnd
moderation discussion moot.

Thanks,
  -John


On Tue, Mar 15, 2011 at 6:20 AM, Carsten Wolff <carsten@wolffcarsten.de> wrote:
> On Monday 14 March 2011, John Heffner wrote:
>> On Mon, Mar 14, 2011 at 3:10 PM, Yuchung Cheng <ycheng@google.com> wrote:
>> > On Mon, Mar 14, 2011 at 3:06 AM, Carsten Wolff <carsten@wolffcarsten.de>
> wrote:
>> >> The moderation is in place to avoid gigantic segment bursts, which could
>> >> cause unnecessary pressure on buffers. In my eyes it's already
>> >> suboptimal that the moderation is weakened in the presence of
>> >> (detected) reordering, let alone removing it completely.
>> >
>> > In the presence of reordering, cwnd is already moderated in Disorder
>> > state before
>> >  entering the (false) recovery.
>>
>> I've always been somewhat skeptical of the usefulness of cwnd
>> moderation.  First, I don't know that its behavior is well defined.
>> When *should* tcp_moderate_cwnd() actually be called, and why?
>>
>> Second, I've never liked the idea in general.  Reducing cwnd has an
>> effect lasting many RTTs, so reducing it in response to a transient
>> event like reordering seems dubious.  And it does not address many
>> causes of bursts, such as ack compression or stretch acks.
>
> I think the undo operations are a special case where it absolutely makes
> sense, because the cwnd _already_ has been reduced (down to 1 segment by RTO
> recovery, or, with its special mixture of ratehalving and newreno, Linux
> sometimes reduces cwnd far below half of it even in fast recovery.). The undo
> re-opens the window all at once, which may allow _really huge_ bursts. I don't
> know enough about the other causes of TCPs burstiness, I'm just concerned
> about allowing this special cause.
>
> Carsten
> --
>           /\-´-/\
>          (  @ @  )
> ________o0O___^___O0o________
>

Re: [PATCH] tcp: avoid cwnd moderation in undo

[Alexander Zimmermann]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Hi John,

Am 15.03.2011 um 11:50 schrieb John Heffner:

> 
> While I'm adding to my wish list ;-), I will also say that the undo
> logic seems unnecessarily complicated.  If you expect you might have
> to undo what you just did, why not wait a bit?  Something like rfc4653
> seems like it might be a better approach and might make the cwnd
> moderation discussion moot.
> 

actually, our group (including Carsten) implemented RFC4653 for Linux.
At the moment, we run our final measurements. However, since I leave
the university, the work is not making much progress. Nevertheless I
I hope that we can finish this in the next couple of months. Besides the
Linux patch, we will bring an rfc4653bis as well as a reordering detection
draft to tcpm.

Alex 


//
// Dipl.-Inform. Alexander Zimmermann
// Department of Computer Science, Informatik 4
// RWTH Aachen University
// Ahornstr. 55, 52056 Aachen, Germany
// phone: (49-241) 80-21422, fax: (49-241) 80-22222
// email: zimmermann@cs.rwth-aachen.de
// web: http://www.umic-mesh.net
//

-----BEGIN PGP SIGNATURE-----
Version: GnuPG/MacGPG2 v2.0.17 (Darwin)
Comment: GPGTools - http://gpgtools.org

iEYEARECAAYFAk1/tQsACgkQdyiq39b9uS7cFACeLaNsGVWoiVzAnUjXOnz8Y/Lk
fp0An0IlprotGMbtl1nXC62oyVEHcHv1
=zxT9
-----END PGP SIGNATURE-----

Re: [PATCH] tcp: avoid cwnd moderation in undo

[Carsten Wolff]

Hi,

On Monday 14 March 2011, Yuchung Cheng wrote:
> On Mon, Mar 14, 2011 at 3:06 AM, Carsten Wolff <carsten@wolffcarsten.de> 
wrote:
> > The moderation is in place to avoid gigantic segment bursts, which could
> > cause unnecessary pressure on buffers. In my eyes it's already
> > suboptimal that the moderation is weakened in the presence of (detected)
> > reordering, let alone removing it completely.
> 
> In the presence of reordering, cwnd is already moderated in Disorder
> state before
>  entering the (false) recovery.

Sure, cwnd moderation to in_flight + 1 segment is applied in disorder state, 
because this is implementing a form of extended limited transmit. 
Nevertheless, after a reordering event that caused a spurious fast retransmit, 
there can be an undo of congestion state changes (either after recovery or 
interrupting recovery, depending on the options enabled in the connection). I 
just wanted to point out, that the moderation step happening upon an undo may 
allow a larger burst, if a previous reordering event was detected and caused 
tp->reordering to be increased.

> > More importantly, the prior ssthresh is restored and not affected by
> > moderation. This means, if moderation reduces cwnd to a small value, then
> > cwnd < ssthresh and TCP will quickly slow-start back to the previous
> > state, without sending a big burst of segments.

This is actually the more important point, because it means that the 
moderation does not negate the effects of the undo operation, as suggested by 
your patch-description.

> > Also, you intended to remove cwnd moderation only from an undo during
> > recovery, but I think your patch also removes cwnd moderation when the
> > undo is caused by D-SACK, i.e. most likely after recovery already ended.
> 
> Thanks. I will update my patch description. But the same principle
> applies that cwnd
> should not be moderated on false events. Whether it should be moderated on
> reordering or other events is another (complex) design issue. But this
> patch does not touch that.

False fast retransmits are mostly caused by reordering, spurious RTOs can also 
be caused by delay variations that do not exhibit reordering. Your patch 
touches all cases of spurious events. Anyway, I just mentioned reordering, 
because it is the event in which Linux already allows larger bursts of size 
tp->reordering in the moderation function (i.e. tp->reordering might be 
increased). It's also not important to me if the undo is happening duringor 
after recovery, the important question is, if burst protection in general is 
an important goal, or not (and I think it's there for a reason).

Carsten
-- 
           /\-´-/\
          (  @ @  )
________o0O___^___O0o________

Re: [PATCH] tcp: avoid cwnd moderation in undo

[Yuchung Cheng]

On Tue, Mar 15, 2011 at 3:07 AM, Carsten Wolff <carsten@wolffcarsten.de> wrote:
>
> Hi,
>
> On Monday 14 March 2011, Yuchung Cheng wrote:
> > On Mon, Mar 14, 2011 at 3:06 AM, Carsten Wolff <carsten@wolffcarsten.de>
> wrote:
> > In the presence of reordering, cwnd is already moderated in Disorder
> > state before
> >  entering the (false) recovery.
>
> Sure, cwnd moderation to in_flight + 1 segment is applied in disorder state,

it's in_flight + 3 usually. the moderation first happens
tcp_try_to_open() instead of tcp_cwnd_down()

>
> because this is implementing a form of extended limited transmit.
> Nevertheless, after a reordering event that caused a spurious fast retransmit,
> there can be an undo of congestion state changes (either after recovery or
> interrupting recovery, depending on the options enabled in the connection). I
> just wanted to point out, that the moderation step happening upon an undo may
> allow a larger burst, if a previous reordering event was detected and caused
> tp->reordering to be increased.

Your point is that cwnd should be moderated on reordering (in undo or
other events). Point taken.
 My point is that cwnd does not need to be moderated on false
recoveries. Do you agree?
To implement your design, tcp_update_reordering should do
tcp_cwnd_moderation().
To implement my point, the moderations should be avoided in undo operations.

The two aren't in conflict. But there are cases that have both undo
and reordering.
Are we on the same page?

>
> > > More importantly, the prior ssthresh is restored and not affected by
> > > moderation. This means, if moderation reduces cwnd to a small value, then
> > > cwnd < ssthresh and TCP will quickly slow-start back to the previous
> > > state, without sending a big burst of segments.
>
> This is actually the more important point, because it means that the
> moderation does not negate the effects of the undo operation, as suggested by
> your patch-description.

It's a double-edge sword. Why slow-start if there is no real loss? It
hurts short
request-response type traffic performance badly b/c each undo makes cwnd = 3.
>
> False fast retransmits are mostly caused by reordering, spurious RTOs can also
> be caused by delay variations that do not exhibit reordering. Your patch
> touches all cases of spurious events. Anyway, I just mentioned reordering,
> because it is the event in which Linux already allows larger bursts of size
> tp->reordering in the moderation function (i.e. tp->reordering might be
> increased). It's also not important to me if the undo is happening duringor
> after recovery, the important question is, if burst protection in general is
> an important goal, or not (and I think it's there for a reason).
I am hoping my previous explanation make sense to you (these two points are
not in conflict).

Re: [PATCH] tcp: avoid cwnd moderation in undo

[Carsten Wolff]

[-- Attachment #1: Type: Text/Plain, Size: 5367 bytes --]

Hi again,

On Wednesday 16 March 2011, Yuchung Cheng wrote:
> On Tue, Mar 15, 2011 at 3:07 AM, Carsten Wolff <carsten@wolffcarsten.de> 
wrote:
> > Hi,
> > 
> > On Monday 14 March 2011, Yuchung Cheng wrote:
> > > On Mon, Mar 14, 2011 at 3:06 AM, Carsten Wolff
> > > <carsten@wolffcarsten.de>
> > 
> > wrote:
> > > In the presence of reordering, cwnd is already moderated in Disorder
> > > state before
> > >  entering the (false) recovery.
> > 
> > Sure, cwnd moderation to in_flight + 1 segment is applied in disorder
> > state,
> 
> it's in_flight + 3 usually. the moderation first happens
> tcp_try_to_open() instead of tcp_cwnd_down()

In disorder state, tcp_try_to_open() calls tcp_cwnd_down() which clamps cwnd 
to in_flight + 1 for dupacks (where tcp_packets_in_flight() is not to be 
confused with the IN_FLIGHT variable in IETF documents, which is called 
packets_out in Linux ...). Otherwise, Linux would be violating RFC3042, which 
allows to send one SMSS of data on each dupack before recovery (actually, just 
the first two, but since the DupThresh can be larger than 3 in linux, it 
extends Limited Transmit to more than just the first two dupacks). This is 
mostly equivalent to the aggressive variant of extended limited transmit in 
RFC4653.

> > because this is implementing a form of extended limited transmit.
> > Nevertheless, after a reordering event that caused a spurious fast
> > retransmit, there can be an undo of congestion state changes (either
> > after recovery or interrupting recovery, depending on the options
> > enabled in the connection). I just wanted to point out, that the
> > moderation step happening upon an undo may allow a larger burst, if a
> > previous reordering event was detected and caused tp->reordering to be
> > increased.
> 
> Your point is that cwnd should be moderated on reordering (in undo or
> other events). Point taken.
>  My point is that cwnd does not need to be moderated on false
> recoveries. Do you agree?
> To implement your design, tcp_update_reordering should do
> tcp_cwnd_moderation().
> To implement my point, the moderations should be avoided in undo
> operations.
> 
> The two aren't in conflict. But there are cases that have both undo
> and reordering.
> Are we on the same page?

Unfortunately, no. ;-) My point is, that cwnd should be moderated when the 
congestion state changes are undone after a spurious recovery has been 
detected. Reordering is only one possible reason for a false recovery. And I 
stick to that point because of the thoughts I pointed out in my mail to john, 
i.e. undo typically leading to exceptionally large segment bursts.

As for cwnd moderation upon the detection of a reordering event (that's a 
different thing at a differnt point in time than detection of a false 
recovery!): This wouldn't make sense to me. The detection of the reordering 
event together with a metric that measures the extent of the reordering can be 
used to try and prevent false recoverys in future reordering events, by 
delaying the congestion reaction (i.e. fast retransmit) then.

Reordering can be a cause of spurious recovery. But undo mechanisms and 
mechanisms to prevent false recovery(s) are orthogonal.

Your patch touches all undos, while reordering is just an example for a cause 
of false recovery.

> > > > More importantly, the prior ssthresh is restored and not affected by
> > > > moderation. This means, if moderation reduces cwnd to a small value,
> > > > then cwnd < ssthresh and TCP will quickly slow-start back to the
> > > > previous state, without sending a big burst of segments.
> > 
> > This is actually the more important point, because it means that the
> > moderation does not negate the effects of the undo operation, as
> > suggested by your patch-description.
> 
> It's a double-edge sword. Why slow-start if there is no real loss?

Its a timing thing. I mean, it is an undo operation: the harm has been done, 
some opportunity to send new data has been lost. Trying to send all that data 
at once now without an ACK-clock will cause more harm when buffers are under 
pressure. The undo operation should not try to make up for lost opportunity, 
only try to reduce further loss of opportunity to send new data. For this, the 
segment bursts have to be moderated.

> It
> hurts short
> request-response type traffic performance badly b/c each undo makes cwnd =
> 3.
> 
> > False fast retransmits are mostly caused by reordering, spurious RTOs can
> > also be caused by delay variations that do not exhibit reordering. Your
> > patch touches all cases of spurious events. Anyway, I just mentioned
> > reordering, because it is the event in which Linux already allows larger
> > bursts of size tp->reordering in the moderation function (i.e.
> > tp->reordering might be increased). It's also not important to me if the
> > undo is happening duringor after recovery, the important question is, if
> > burst protection in general is an important goal, or not (and I think
> > it's there for a reason).
> 
> I am hoping my previous explanation make sense to you (these two points are
> not in conflict).

I hope the same for my explanations. :-)

Cheers
Carsten
-- 
           /\-´-/\
          (  @ @  )
________o0O___^___O0o________

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 190 bytes --]

Re: [PATCH] tcp: avoid cwnd moderation in undo

[John Heffner]

On Wed, Mar 16, 2011 at 12:18 PM, Carsten Wolff <carsten@wolffcarsten.de> wrote:
> Unfortunately, no. ;-) My point is, that cwnd should be moderated when the
> congestion state changes are undone after a spurious recovery has been
> detected. Reordering is only one possible reason for a false recovery. And I
> stick to that point because of the thoughts I pointed out in my mail to john,
> i.e. undo typically leading to exceptionally large segment bursts.

There's a more general discussion here as to how much it's worth
avoiding bursts at all.  What research on the subject I'm aware of is
somewhat inconclusive.  It's possible to construct scenarios where
bursting, burst suppression, or pacing each win or lose badly.  If you
can choose only one approach as one-size-fits all it's difficult with
the information at hand to pick only one.  However, I really do wonder
why it's so important to suppress bursts on congestion state undo when
other, likely far more common, sources of bursts are not suppressed.

Carsten, do you have any specific examples of cases you're concerned
about?  FWIW, there are exactly two causes for spurious retransmits:
spurious fast retransmit due to reordering, and spurious timeouts due
to a delay spike.  Are you particularly concerned with one more than
the other?

Thanks,
  -John

Re: [PATCH] tcp: avoid cwnd moderation in undo

[Ilpo Järvinen]

On Wed, 16 Mar 2011, John Heffner wrote:

> On Wed, Mar 16, 2011 at 12:18 PM, Carsten Wolff <carsten@wolffcarsten.de> wrote:
> > Unfortunately, no. ;-) My point is, that cwnd should be moderated when the
> > congestion state changes are undone after a spurious recovery has been
> > detected. Reordering is only one possible reason for a false recovery. And I
> > stick to that point because of the thoughts I pointed out in my mail to john,
> > i.e. undo typically leading to exceptionally large segment bursts.
> 
> There's a more general discussion here as to how much it's worth
> avoiding bursts at all.  What research on the subject I'm aware of is
> somewhat inconclusive.  It's possible to construct scenarios where
> bursting, burst suppression, or pacing each win or lose badly.  If you
> can choose only one approach as one-size-fits all it's difficult with
> the information at hand to pick only one.  However, I really do wonder
> why it's so important to suppress bursts on congestion state undo when
> other, likely far more common, sources of bursts are not suppressed.
> 
> Carsten, do you have any specific examples of cases you're concerned
> about?  FWIW, there are exactly two causes for spurious retransmits:
> spurious fast retransmit due to reordering, and spurious timeouts due
> to a delay spike.  Are you particularly concerned with one more than
> the other?

The latter is handled with FRTO (defaults on in Linux) so nicely that no 
burstiness can be seen.

-- 
 i.

Re: [PATCH] tcp: avoid cwnd moderation in undo

[Yuchung Cheng]

Hi Carsten,

Thanks for the detailed explanation. This funnels down to

a) is cwnd moderation always a good idea?

b) if (a) is true, cwnd should be moderated all the time to suppress
any possible burst.

Clearly, Linux code supports (a) and (b). Like John's email, I am not
aware of too much scientific data supporting that.

But I do have a real problem on web server. The HTTP response has
normally a handful of packets. Moderating cwnd at the end or after the
(false) recovery often makes cwnd=3-4. This causes slow-start on the
subsequent HTTP response and throws away some benefit of  persistent
HTTP connections. That slow-start is not necessary and adds some extra
round-trips to serve the tiny object.


On Wed, Mar 16, 2011 at 9:18 AM, Carsten Wolff <carsten@wolffcarsten.de> wrote:
> Hi again,
>
> On Wednesday 16 March 2011, Yuchung Cheng wrote:
>> On Tue, Mar 15, 2011 at 3:07 AM, Carsten Wolff <carsten@wolffcarsten.de>
> wrote:
>> > Hi,
>> >
>> > On Monday 14 March 2011, Yuchung Cheng wrote:
>> > > On Mon, Mar 14, 2011 at 3:06 AM, Carsten Wolff
>> > > <carsten@wolffcarsten.de>
>> >
>> > wrote:
>> > > In the presence of reordering, cwnd is already moderated in Disorder
>> > > state before
>> > >  entering the (false) recovery.
>> >
>> > Sure, cwnd moderation to in_flight + 1 segment is applied in disorder
>> > state,
>>
>> it's in_flight + 3 usually. the moderation first happens
>> tcp_try_to_open() instead of tcp_cwnd_down()
>
> In disorder state, tcp_try_to_open() calls tcp_cwnd_down() which clamps cwnd
> to in_flight + 1 for dupacks (where tcp_packets_in_flight() is not to be
> confused with the IN_FLIGHT variable in IETF documents, which is called
> packets_out in Linux ...). Otherwise, Linux would be violating RFC3042, which
> allows to send one SMSS of data on each dupack before recovery (actually, just
> the first two, but since the DupThresh can be larger than 3 in linux, it
> extends Limited Transmit to more than just the first two dupacks). This is
> mostly equivalent to the aggressive variant of extended limited transmit in
> RFC4653.
>
>> > because this is implementing a form of extended limited transmit.
>> > Nevertheless, after a reordering event that caused a spurious fast
>> > retransmit, there can be an undo of congestion state changes (either
>> > after recovery or interrupting recovery, depending on the options
>> > enabled in the connection). I just wanted to point out, that the
>> > moderation step happening upon an undo may allow a larger burst, if a
>> > previous reordering event was detected and caused tp->reordering to be
>> > increased.
>>
>> Your point is that cwnd should be moderated on reordering (in undo or
>> other events). Point taken.
>>  My point is that cwnd does not need to be moderated on false
>> recoveries. Do you agree?
>> To implement your design, tcp_update_reordering should do
>> tcp_cwnd_moderation().
>> To implement my point, the moderations should be avoided in undo
>> operations.
>>
>> The two aren't in conflict. But there are cases that have both undo
>> and reordering.
>> Are we on the same page?
>
> Unfortunately, no. ;-) My point is, that cwnd should be moderated when the
> congestion state changes are undone after a spurious recovery has been
> detected. Reordering is only one possible reason for a false recovery. And I
> stick to that point because of the thoughts I pointed out in my mail to john,
> i.e. undo typically leading to exceptionally large segment bursts.
>
> As for cwnd moderation upon the detection of a reordering event (that's a
> different thing at a differnt point in time than detection of a false
> recovery!): This wouldn't make sense to me. The detection of the reordering
> event together with a metric that measures the extent of the reordering can be
> used to try and prevent false recoverys in future reordering events, by
> delaying the congestion reaction (i.e. fast retransmit) then.
>
> Reordering can be a cause of spurious recovery. But undo mechanisms and
> mechanisms to prevent false recovery(s) are orthogonal.
>
> Your patch touches all undos, while reordering is just an example for a cause
> of false recovery.
>
>> > > > More importantly, the prior ssthresh is restored and not affected by
>> > > > moderation. This means, if moderation reduces cwnd to a small value,
>> > > > then cwnd < ssthresh and TCP will quickly slow-start back to the
>> > > > previous state, without sending a big burst of segments.
>> >
>> > This is actually the more important point, because it means that the
>> > moderation does not negate the effects of the undo operation, as
>> > suggested by your patch-description.
>>
>> It's a double-edge sword. Why slow-start if there is no real loss?
>
> Its a timing thing. I mean, it is an undo operation: the harm has been done,
> some opportunity to send new data has been lost. Trying to send all that data
> at once now without an ACK-clock will cause more harm when buffers are under
> pressure. The undo operation should not try to make up for lost opportunity,
> only try to reduce further loss of opportunity to send new data. For this, the
> segment bursts have to be moderated.
>
>> It
>> hurts short
>> request-response type traffic performance badly b/c each undo makes cwnd =
>> 3.
>>
>> > False fast retransmits are mostly caused by reordering, spurious RTOs can
>> > also be caused by delay variations that do not exhibit reordering. Your
>> > patch touches all cases of spurious events. Anyway, I just mentioned
>> > reordering, because it is the event in which Linux already allows larger
>> > bursts of size tp->reordering in the moderation function (i.e.
>> > tp->reordering might be increased). It's also not important to me if the
>> > undo is happening duringor after recovery, the important question is, if
>> > burst protection in general is an important goal, or not (and I think
>> > it's there for a reason).
>>
>> I am hoping my previous explanation make sense to you (these two points are
>> not in conflict).
>
> I hope the same for my explanations. :-)
>
> Cheers
> Carsten
> --
>           /\-´-/\
>          (  @ @  )
> ________o0O___^___O0o________
>

[PATCH v2] tcp: avoid cwnd moderation in undo

[Yuchung Cheng]

In the current undo logic, cwnd is moderated after it was restored
to the value prior entering fast-recovery. It was moderated first
in tcp_try_undo_recovery then again in tcp_complete_cwr.

Since the undo indicates recovery was false, these moderations
are not necessary. If the undo is triggered when most of the
outstanding data have been acknowledged, the (restored) cwnd is
falsely pulled down to a small value.

This patch removes these cwnd moderations if cwnd is undone
  a) during fast-recovery
	b) by receiving DSACKs past fast-recovery

Signed-off-by: Yuchung Cheng <ycheng@google.com>
---
Changelog since v1:
  Revise the patch description to include undos after recovery

 net/ipv4/tcp_input.c |   12 +++++++-----
 1 files changed, 7 insertions(+), 5 deletions(-)

diff --git a/net/ipv4/tcp_input.c b/net/ipv4/tcp_input.c
index 08ea735..1bd0f38 100644
--- a/net/ipv4/tcp_input.c
+++ b/net/ipv4/tcp_input.c
@@ -2659,7 +2659,7 @@ static void DBGUNDO(struct sock *sk, const char *msg)
 #define DBGUNDO(x...) do { } while (0)
 #endif
 
-static void tcp_undo_cwr(struct sock *sk, const int undo)
+static void tcp_undo_cwr(struct sock *sk, const int undo_ssthresh)
 {
 	struct tcp_sock *tp = tcp_sk(sk);
 
@@ -2671,14 +2671,13 @@ static void tcp_undo_cwr(struct sock *sk, const int undo)
 		else
 			tp->snd_cwnd = max(tp->snd_cwnd, tp->snd_ssthresh << 1);
 
-		if (undo && tp->prior_ssthresh > tp->snd_ssthresh) {
+		if (undo_ssthresh && tp->prior_ssthresh > tp->snd_ssthresh) {
 			tp->snd_ssthresh = tp->prior_ssthresh;
 			TCP_ECN_withdraw_cwr(tp);
 		}
 	} else {
 		tp->snd_cwnd = max(tp->snd_cwnd, tp->snd_ssthresh);
 	}
-	tcp_moderate_cwnd(tp);
 	tp->snd_cwnd_stamp = tcp_time_stamp;
 }
 
@@ -2822,8 +2821,11 @@ static int tcp_try_undo_loss(struct sock *sk)
 static inline void tcp_complete_cwr(struct sock *sk)
 {
 	struct tcp_sock *tp = tcp_sk(sk);
-	tp->snd_cwnd = min(tp->snd_cwnd, tp->snd_ssthresh);
-	tp->snd_cwnd_stamp = tcp_time_stamp;
+	/* Do not moderate cwnd if it's already undone in cwr or recovery */
+	if (tp->undo_marker && tp->snd_cwnd > tp->snd_ssthresh) {
+		tp->snd_cwnd = tp->snd_ssthresh;
+		tp->snd_cwnd_stamp = tcp_time_stamp;
+	}
 	tcp_ca_event(sk, CA_EVENT_COMPLETE_CWR);
 }
 
-- 
1.7.3.1

Re: [PATCH v2] tcp: avoid cwnd moderation in undo

[David Miller]

From: Yuchung Cheng <ycheng@google.com>
Date: Mon, 14 Mar 2011 13:57:03 -0700

> In the current undo logic, cwnd is moderated after it was restored
> to the value prior entering fast-recovery. It was moderated first
> in tcp_try_undo_recovery then again in tcp_complete_cwr.
> 
> Since the undo indicates recovery was false, these moderations
> are not necessary. If the undo is triggered when most of the
> outstanding data have been acknowledged, the (restored) cwnd is
> falsely pulled down to a small value.
> 
> This patch removes these cwnd moderations if cwnd is undone
>   a) during fast-recovery
> 	b) by receiving DSACKs past fast-recovery
> 
> Signed-off-by: Yuchung Cheng <ycheng@google.com>

Applied, thank you.

I followed it up with this tidy up:

--------------------
tcp: Make undo_ssthresh arg to tcp_undo_cwr() a bool.

Signed-off-by: David S. Miller <davem@davemloft.net>
---
 net/ipv4/tcp_input.c |   12 ++++++------
 1 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/net/ipv4/tcp_input.c b/net/ipv4/tcp_input.c
index e72af7e..bef9f04 100644
--- a/net/ipv4/tcp_input.c
+++ b/net/ipv4/tcp_input.c
@@ -2659,7 +2659,7 @@ static void DBGUNDO(struct sock *sk, const char *msg)
 #define DBGUNDO(x...) do { } while (0)
 #endif
 
-static void tcp_undo_cwr(struct sock *sk, const int undo_ssthresh)
+static void tcp_undo_cwr(struct sock *sk, const bool undo_ssthresh)
 {
 	struct tcp_sock *tp = tcp_sk(sk);
 
@@ -2698,7 +2698,7 @@ static int tcp_try_undo_recovery(struct sock *sk)
 		 * or our original transmission succeeded.
 		 */
 		DBGUNDO(sk, inet_csk(sk)->icsk_ca_state == TCP_CA_Loss ? "loss" : "retrans");
-		tcp_undo_cwr(sk, 1);
+		tcp_undo_cwr(sk, true);
 		if (inet_csk(sk)->icsk_ca_state == TCP_CA_Loss)
 			mib_idx = LINUX_MIB_TCPLOSSUNDO;
 		else
@@ -2725,7 +2725,7 @@ static void tcp_try_undo_dsack(struct sock *sk)
 
 	if (tp->undo_marker && !tp->undo_retrans) {
 		DBGUNDO(sk, "D-SACK");
-		tcp_undo_cwr(sk, 1);
+		tcp_undo_cwr(sk, true);
 		tp->undo_marker = 0;
 		NET_INC_STATS_BH(sock_net(sk), LINUX_MIB_TCPDSACKUNDO);
 	}
@@ -2778,7 +2778,7 @@ static int tcp_try_undo_partial(struct sock *sk, int acked)
 		tcp_update_reordering(sk, tcp_fackets_out(tp) + acked, 1);
 
 		DBGUNDO(sk, "Hoe");
-		tcp_undo_cwr(sk, 0);
+		tcp_undo_cwr(sk, false);
 		NET_INC_STATS_BH(sock_net(sk), LINUX_MIB_TCPPARTIALUNDO);
 
 		/* So... Do not make Hoe's retransmit yet.
@@ -2807,7 +2807,7 @@ static int tcp_try_undo_loss(struct sock *sk)
 
 		DBGUNDO(sk, "partial loss");
 		tp->lost_out = 0;
-		tcp_undo_cwr(sk, 1);
+		tcp_undo_cwr(sk, true);
 		NET_INC_STATS_BH(sock_net(sk), LINUX_MIB_TCPLOSSUNDO);
 		inet_csk(sk)->icsk_retransmits = 0;
 		tp->undo_marker = 0;
@@ -3496,7 +3496,7 @@ static void tcp_undo_spur_to_response(struct sock *sk, int flag)
 	if (flag & FLAG_ECE)
 		tcp_ratehalving_spur_to_response(sk);
 	else
-		tcp_undo_cwr(sk, 1);
+		tcp_undo_cwr(sk, true);
 }
 
 /* F-RTO spurious RTO detection algorithm (RFC4138)
-- 
1.7.4.1