* [PATCH next] dctcp: update cwnd on congestion event
@ 2016-11-14 15:42 Florian Westphal
2016-11-16 3:02 ` David Miller
2016-12-02 21:01 ` Neal Cardwell
0 siblings, 2 replies; 4+ messages in thread
From: Florian Westphal @ 2016-11-14 15:42 UTC (permalink / raw)
To: netdev
Cc: Florian Westphal, Lawrence Brakmo, Andrew Shewmaker, Glenn Judd,
Daniel Borkmann
draft-ietf-tcpm-dctcp-02 says:
... when the sender receives an indication of congestion
(ECE), the sender SHOULD update cwnd as follows:
cwnd = cwnd * (1 - DCTCP.Alpha / 2)
So, lets do this and reduce cwnd more smoothly (and faster), as per
current congestion estimate.
Cc: Lawrence Brakmo <brakmo@fb.com>
Cc: Andrew Shewmaker <agshew@gmail.com>
Cc: Glenn Judd <glenn.judd@morganstanley.com>
Cc: Daniel Borkmann <daniel@iogearbox.net>
Signed-off-by: Florian Westphal <fw@strlen.de>
---
diff --git a/net/ipv4/tcp_dctcp.c b/net/ipv4/tcp_dctcp.c
index ab37c6775630..51139175bf61 100644
--- a/net/ipv4/tcp_dctcp.c
+++ b/net/ipv4/tcp_dctcp.c
@@ -188,8 +188,8 @@ static void dctcp_ce_state_1_to_0(struct sock *sk)
static void dctcp_update_alpha(struct sock *sk, u32 flags)
{
- const struct tcp_sock *tp = tcp_sk(sk);
struct dctcp *ca = inet_csk_ca(sk);
+ struct tcp_sock *tp = tcp_sk(sk);
u32 acked_bytes = tp->snd_una - ca->prior_snd_una;
/* If ack did not advance snd_una, count dupack as MSS size.
@@ -229,6 +229,13 @@ static void dctcp_update_alpha(struct sock *sk, u32 flags)
WRITE_ONCE(ca->dctcp_alpha, alpha);
dctcp_reset(tp, ca);
}
+
+ if (flags & CA_ACK_ECE) {
+ unsigned int cwnd = dctcp_ssthresh(sk);
+
+ if (cwnd != tp->snd_cwnd)
+ tp->snd_cwnd = cwnd;
+ }
}
static void dctcp_state(struct sock *sk, u8 new_state)
--
2.7.3
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH next] dctcp: update cwnd on congestion event
2016-11-14 15:42 [PATCH next] dctcp: update cwnd on congestion event Florian Westphal
@ 2016-11-16 3:02 ` David Miller
2016-12-02 21:01 ` Neal Cardwell
1 sibling, 0 replies; 4+ messages in thread
From: David Miller @ 2016-11-16 3:02 UTC (permalink / raw)
To: fw; +Cc: netdev, brakmo, agshew, glenn.judd, daniel
From: Florian Westphal <fw@strlen.de>
Date: Mon, 14 Nov 2016 16:42:01 +0100
> draft-ietf-tcpm-dctcp-02 says:
>
> ... when the sender receives an indication of congestion
> (ECE), the sender SHOULD update cwnd as follows:
>
> cwnd = cwnd * (1 - DCTCP.Alpha / 2)
>
> So, lets do this and reduce cwnd more smoothly (and faster), as per
> current congestion estimate.
>
> Cc: Lawrence Brakmo <brakmo@fb.com>
> Cc: Andrew Shewmaker <agshew@gmail.com>
> Cc: Glenn Judd <glenn.judd@morganstanley.com>
> Cc: Daniel Borkmann <daniel@iogearbox.net>
> Signed-off-by: Florian Westphal <fw@strlen.de>
Applied.
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH next] dctcp: update cwnd on congestion event
2016-11-14 15:42 [PATCH next] dctcp: update cwnd on congestion event Florian Westphal
2016-11-16 3:02 ` David Miller
@ 2016-12-02 21:01 ` Neal Cardwell
2016-12-02 21:49 ` Florian Westphal
1 sibling, 1 reply; 4+ messages in thread
From: Neal Cardwell @ 2016-12-02 21:01 UTC (permalink / raw)
To: Florian Westphal
Cc: Netdev, Lawrence Brakmo, Andrew Shewmaker, Glenn Judd,
Daniel Borkmann, Yuchung Cheng, Eric Dumazet,
Soheil Hassas Yeganeh
On Mon, Nov 14, 2016 at 10:42 AM, Florian Westphal <fw@strlen.de> wrote:
>
> draft-ietf-tcpm-dctcp-02 says:
>
> ... when the sender receives an indication of congestion
> (ECE), the sender SHOULD update cwnd as follows:
>
> cwnd = cwnd * (1 - DCTCP.Alpha / 2)
>
> So, lets do this and reduce cwnd more smoothly (and faster), as per
> current congestion estimate.
AFAICT this is doing a multiplicative decrease of cwnd on every ACK
that has an ECE bit.
If I am reading the code correctly, then I would have two concerns:
1) Has that been tested? That seems like an extremely dramatic
decrease in cwnd. For example, if the cwnd is 80, and there are 40
ACKs, and half the ACKs are ECE marked, then my back-of-the-envelope
calculations seem to suggest that after just 11 ACKs the cwnd would be
down to a minimal value of 2:
ack 1 cwnd=60
ack 2 cwnd=45
ack 3 cwnd=33
ack 4 cwnd=24
ack 5 cwnd=18
ack 6 cwnd=13
ack 7 cwnd=9
ack 8 cwnd=6
ack 9 cwnd=4
ack 10 cwnd=3
ack 11 cwnd=2
2) That seems to contradict another passage in the draft (v 02 or 03). Consider
https://tools.ietf.org/html/draft-ietf-tcpm-dctcp-03
where it says
Just as specified in [RFC3168], DCTCP does not react to congestion
indications more than once for every window of data.
So the draft seems to advocate not reacting to congestion indications
more than once per window. Yet this patch reacts on every ECE-marked
ACK within a window.
Am I reading something incorrectly?
cheers,
neal
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH next] dctcp: update cwnd on congestion event
2016-12-02 21:01 ` Neal Cardwell
@ 2016-12-02 21:49 ` Florian Westphal
0 siblings, 0 replies; 4+ messages in thread
From: Florian Westphal @ 2016-12-02 21:49 UTC (permalink / raw)
To: Neal Cardwell
Cc: Florian Westphal, Netdev, Lawrence Brakmo, Andrew Shewmaker,
Glenn Judd, Daniel Borkmann, Yuchung Cheng, Eric Dumazet,
Soheil Hassas Yeganeh
Neal Cardwell <ncardwell@google.com> wrote:
> On Mon, Nov 14, 2016 at 10:42 AM, Florian Westphal <fw@strlen.de> wrote:
> >
> > draft-ietf-tcpm-dctcp-02 says:
> >
> > ... when the sender receives an indication of congestion
> > (ECE), the sender SHOULD update cwnd as follows:
> >
> > cwnd = cwnd * (1 - DCTCP.Alpha / 2)
> >
> > So, lets do this and reduce cwnd more smoothly (and faster), as per
> > current congestion estimate.
>
> AFAICT this is doing a multiplicative decrease of cwnd on every ACK
> that has an ECE bit.
>
> If I am reading the code correctly, then I would have two concerns:
>
> 1) Has that been tested? That seems like an extremely dramatic
> decrease in cwnd. For example, if the cwnd is 80, and there are 40
> ACKs, and half the ACKs are ECE marked, then my back-of-the-envelope
> calculations seem to suggest that after just 11 ACKs the cwnd would be
> down to a minimal value of 2:
>
> ack 1 cwnd=60
> ack 2 cwnd=45
> ack 3 cwnd=33
[..]
You are assuming alpha = 0.5?
Then, yes, looks correct. Since some of these acks will most likely
also end an observation window acks might also cause change to alpha.
> 2) That seems to contradict another passage in the draft (v 02 or 03). Consider
> https://tools.ietf.org/html/draft-ietf-tcpm-dctcp-03
> where it says
>
> Just as specified in [RFC3168], DCTCP does not react to congestion
> indications more than once for every window of data.
>
> So the draft seems to advocate not reacting to congestion indications
> more than once per window. Yet this patch reacts on every ECE-marked
> ACK within a window.
>
> Am I reading something incorrectly?
No, I will raise this on tcpm next monday (if you want you
can of course do this yourself).
Would be easy to make it so this cwnd update only happens once in each
observation cycle, but it would be even better if this would get input
from draft authors.
Thanks Neal!
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2016-12-02 21:52 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-11-14 15:42 [PATCH next] dctcp: update cwnd on congestion event Florian Westphal
2016-11-16 3:02 ` David Miller
2016-12-02 21:01 ` Neal Cardwell
2016-12-02 21:49 ` Florian Westphal
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.