All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH net-next] tcp: remove cwnd moderation after recovery
@ 2016-03-30  0:15 Yuchung Cheng
  2016-03-30  0:35 ` Stephen Hemminger
  0 siblings, 1 reply; 3+ messages in thread
From: Yuchung Cheng @ 2016-03-30  0:15 UTC (permalink / raw)
  To: davem
  Cc: netdev, Yuchung Cheng, Matt Mathis, Neal Cardwell, Soheil Hassas Yeganeh

For non-SACK connections, cwnd is lowered to inflight plus 3 packets
when the recovery ends. This is an optional feature in the NewReno
RFC 2582 to reduce the potential burst when cwnd is "re-opened"
after recovery and inflight is low.

This feature is questionably effective because of PRR: when
the recovery ends (i.e., snd_una == high_seq) NewReno holds the
CA_Recovery state for another round trip to prevent false fast
retransmits. But if the inflight is low, PRR will overwrite the
moderated cwnd in tcp_cwnd_reduction() later.

On the other hand, if the recovery ends because the sender
detects the losses were spurious (e.g., reordering). This feature
unconditionally lowers a reverted cwnd even though nothing
was lost.

By principle loss recovery module should not update cwnd. Further
pacing is much more effective to reduce burst. Hence this patch
removes the cwnd moderation feature.

Signed-off-by: Matt Mathis <mattmathis@google.com>
Signed-off-by: Neal Cardwell <ncardwell@google.com>
Signed-off-by: Soheil Hassas Yeganeh <soheil@google.com>
---
 include/net/tcp.h    | 11 -----------
 net/ipv4/tcp_input.c | 11 -----------
 2 files changed, 22 deletions(-)

diff --git a/include/net/tcp.h b/include/net/tcp.h
index b91370f..f8bb4a4 100644
--- a/include/net/tcp.h
+++ b/include/net/tcp.h
@@ -1039,17 +1039,6 @@ static inline __u32 tcp_max_tso_deferred_mss(const struct tcp_sock *tp)
 	return 3;
 }
 
-/* Slow start with delack produces 3 packets of burst, so that
- * it is safe "de facto".  This will be the default - same as
- * the default reordering threshold - but if reordering increases,
- * we must be able to allow cwnd to burst at least this much in order
- * to not pull it back when holes are filled.
- */
-static __inline__ __u32 tcp_max_burst(const struct tcp_sock *tp)
-{
-	return tp->reordering;
-}
-
 /* Returns end sequence number of the receiver's advertised window */
 static inline u32 tcp_wnd_end(const struct tcp_sock *tp)
 {
diff --git a/net/ipv4/tcp_input.c b/net/ipv4/tcp_input.c
index e6e65f7..f87b84a 100644
--- a/net/ipv4/tcp_input.c
+++ b/net/ipv4/tcp_input.c
@@ -2252,16 +2252,6 @@ static void tcp_update_scoreboard(struct sock *sk, int fast_rexmit)
 	}
 }
 
-/* CWND moderation, preventing bursts due to too big ACKs
- * in dubious situations.
- */
-static inline void tcp_moderate_cwnd(struct tcp_sock *tp)
-{
-	tp->snd_cwnd = min(tp->snd_cwnd,
-			   tcp_packets_in_flight(tp) + tcp_max_burst(tp));
-	tp->snd_cwnd_stamp = tcp_time_stamp;
-}
-
 static bool tcp_tsopt_ecr_before(const struct tcp_sock *tp, u32 when)
 {
 	return tp->rx_opt.saw_tstamp && tp->rx_opt.rcv_tsecr &&
@@ -2410,7 +2400,6 @@ static bool tcp_try_undo_recovery(struct sock *sk)
 		/* Hold old state until something *above* high_seq
 		 * is ACKed. For Reno it is MUST to prevent false
 		 * fast retransmits (RFC2582). SACK TCP is safe. */
-		tcp_moderate_cwnd(tp);
 		if (!tcp_any_retrans_done(sk))
 			tp->retrans_stamp = 0;
 		return true;
-- 
2.8.0.rc3.226.g39d4020

^ permalink raw reply related	[flat|nested] 3+ messages in thread

* Re: [PATCH net-next] tcp: remove cwnd moderation after recovery
  2016-03-30  0:15 [PATCH net-next] tcp: remove cwnd moderation after recovery Yuchung Cheng
@ 2016-03-30  0:35 ` Stephen Hemminger
  2016-03-30 20:20   ` Yuchung Cheng
  0 siblings, 1 reply; 3+ messages in thread
From: Stephen Hemminger @ 2016-03-30  0:35 UTC (permalink / raw)
  To: Yuchung Cheng
  Cc: davem, netdev, Matt Mathis, Neal Cardwell, Soheil Hassas Yeganeh

On Tue, 29 Mar 2016 17:15:52 -0700
Yuchung Cheng <ycheng@google.com> wrote:

> For non-SACK connections, cwnd is lowered to inflight plus 3 packets
> when the recovery ends. This is an optional feature in the NewReno
> RFC 2582 to reduce the potential burst when cwnd is "re-opened"
> after recovery and inflight is low.
> 
> This feature is questionably effective because of PRR: when
> the recovery ends (i.e., snd_una == high_seq) NewReno holds the
> CA_Recovery state for another round trip to prevent false fast
> retransmits. But if the inflight is low, PRR will overwrite the
> moderated cwnd in tcp_cwnd_reduction() later.
> 
> On the other hand, if the recovery ends because the sender
> detects the losses were spurious (e.g., reordering). This feature
> unconditionally lowers a reverted cwnd even though nothing
> was lost.
> 
> By principle loss recovery module should not update cwnd. Further
> pacing is much more effective to reduce burst. Hence this patch
> removes the cwnd moderation feature.
> 
> Signed-off-by: Matt Mathis <mattmathis@google.com>
> Signed-off-by: Neal Cardwell <ncardwell@google.com>
> Signed-off-by: Soheil Hassas Yeganeh <soheil@google.com>

I have a concern that this might break Linux builtin protection
against hostile receiver sending bogus ACK's.  Remember Linux is
different than NewReno. You are changing something that has existed for
a long long time.

^ permalink raw reply	[flat|nested] 3+ messages in thread

* Re: [PATCH net-next] tcp: remove cwnd moderation after recovery
  2016-03-30  0:35 ` Stephen Hemminger
@ 2016-03-30 20:20   ` Yuchung Cheng
  0 siblings, 0 replies; 3+ messages in thread
From: Yuchung Cheng @ 2016-03-30 20:20 UTC (permalink / raw)
  To: Stephen Hemminger
  Cc: David Miller, netdev, Matt Mathis, Neal Cardwell, Soheil Hassas Yeganeh

On Tue, Mar 29, 2016 at 5:35 PM, Stephen Hemminger
<stephen@networkplumber.org> wrote:
> On Tue, 29 Mar 2016 17:15:52 -0700
> Yuchung Cheng <ycheng@google.com> wrote:
>
>> For non-SACK connections, cwnd is lowered to inflight plus 3 packets
>> when the recovery ends. This is an optional feature in the NewReno
>> RFC 2582 to reduce the potential burst when cwnd is "re-opened"
>> after recovery and inflight is low.
>>
>> This feature is questionably effective because of PRR: when
>> the recovery ends (i.e., snd_una == high_seq) NewReno holds the
>> CA_Recovery state for another round trip to prevent false fast
>> retransmits. But if the inflight is low, PRR will overwrite the
>> moderated cwnd in tcp_cwnd_reduction() later.
>>
>> On the other hand, if the recovery ends because the sender
>> detects the losses were spurious (e.g., reordering). This feature
>> unconditionally lowers a reverted cwnd even though nothing
>> was lost.
>>
>> By principle loss recovery module should not update cwnd. Further
>> pacing is much more effective to reduce burst. Hence this patch
>> removes the cwnd moderation feature.
>>
>> Signed-off-by: Matt Mathis <mattmathis@google.com>
>> Signed-off-by: Neal Cardwell <ncardwell@google.com>
>> Signed-off-by: Soheil Hassas Yeganeh <soheil@google.com>
>
> I have a concern that this might break Linux builtin protection
> against hostile receiver sending bogus ACK's.  Remember Linux is
> different than NewReno. You are changing something that has existed for
> a long long time.

I suppose the bogus ACKs are ACKs acking future but in-flight data
packets. The most bogus ACK would ack at most SND.NXT otherwise it
will be filtered early in tcp_ack(). Then cwnd_moderation() will pull
cwnd down to inflight + maxburst = 0 + 3 = 3. But immediately
afterward tcp_cwnd_reduction() will over-write cwnd = inflight +
sndcnt =~ 0 + newly_acked_sacked.

w/o pacing, such a bogus ACK can at most induce a burst of a window
(minus 3 dupacks). Restart after (short) idle or receiving stretched
ACKs can induce the same degree of behavior. More importantly the cwnd
moderation is already a NOP b/c cwnd is over-written by the PRR,
regardless of this patch. If the receiver's intention is to speed up
recovery, he is risking reliability w/ future ACKs. if the sole
intention is network abuse then he unlikely would bother to trigger
recovery and congestion control reactions in the first place.

I will update the commit message regarding your concern. Thanks.

^ permalink raw reply	[flat|nested] 3+ messages in thread

end of thread, other threads:[~2016-03-30 20:21 UTC | newest]

Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-03-30  0:15 [PATCH net-next] tcp: remove cwnd moderation after recovery Yuchung Cheng
2016-03-30  0:35 ` Stephen Hemminger
2016-03-30 20:20   ` Yuchung Cheng

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.