Messages in this thread | | | From | Eric Dumazet <> | Date | Thu, 4 Apr 2019 02:07:25 -0700 | Subject | Re: [PATCH net-next] tcp: Ensure DCTCP reacts to losses |
| |
On Thu, Apr 4, 2019 at 1:47 AM Daniel Borkmann <daniel@iogearbox.net> wrote: > > On 04/04/2019 10:26 AM, Tilmans, Olivier (Nokia - BE/Antwerp) wrote: > > RFC8257 §3.5 explicitly states that DCTCP should "react to loss > > episode in the same way that a conventional TCP". > > This is also the behavior on MS Windows. > > > > Currently, Linux DCTCP performs no ssthresh reduction when losses > > are encountered. Optionally, the dctcp_clamp_alpha_on_loss resets > > alpha to its maximal value if a RTO happens. This behavior > > is sub-optimal for at least two reasons: i) it ignores losses > > triggering fast retransmissions; and ii) it causes unnecessary large > > cwnd reduction in the future if the loss was isolated as it resets > > the historical term of DCTCP's alpha EWMA to its maximal value (i.e., > > denoting a total congestion). The second reason has an especially > > noticeable effect when using DCTCP in high BDP environments, where > > alpha normally stays at low values. > > > > This patch replace the clamping of alpha by setting ssthresh to > > half of cwnd for both fast retransmissions and RTOs, at most once > > per RTT. To reflect the change, the dctcp_clamp_alpha_on_loss option > > has been renamed to dctcp_halve_cwnd_on_loss. > > > > The table below shows experimental results where we measured the > > drop probability of a PIE AQM (not applying ECN marks) at a > > bottleneck in the presence of a single TCP flow with either the > > alpha-clamping option enabled or the cwnd halving proposed by this > > patch. Results using reno or cubic are given for comparison. > > > > | Link | RTT | Drop > > TCP CC | speed | base+AQM | probability > > ==================|=========|==========|============ > > CUBIC | 40Mbps | 7+20ms | 0.21% > > RENO | | | 0.19% > > DCTCP-CLAMP-ALPHA | | | 25.80% > > DCTCP-HALVE-CWND | | | 0.22% > > ------------------|---------|----------|------------ > > CUBIC | 100Mbps | 7+20ms | 0.03% > > RENO | | | 0.02% > > DCTCP-CLAMP-ALPHA | | | 23.30% > > DCTCP-HALVE-CWND | | | 0.04% > > ------------------|---------|----------|------------ > > CUBIC | 800Mbps | 1+1ms | 0.04% > > RENO | | | 0.05% > > DCTCP-CLAMP-ALPHA | | | 18.70% > > DCTCP-HALVE-CWND | | | 0.06% > > > > We see that, without halving its cwnd for all source of losses, > > DCTCP drives the AQM to large drop probabilities in order to keep > > the queue length under control (i.e., it repeatedly faces RTOs). > > Instead, if DCTCP reacts to all source of losses, it can then be > > controlled by the AQM using similar drop levels than cubic or reno. > > > > Signed-off-by: Koen De Schepper <koen.de_schepper@nokia-bell-labs.com> > > Signed-off-by: Olivier Tilmans <olivier.tilmans@nokia-bell-labs.com> > > Cc: Bob Briscoe <research@bobbriscoe.net> > > Cc: Lawrence Brakmo <brakmo@fb.com> > > Cc: Florian Westphal <fw@strlen.de> > > Cc: Daniel Borkmann <borkmann@iogearbox.net> > > Cc: Yuchung Cheng <ycheng@google.com> > > Cc: Neal Cardwell <ncardwell@google.com> > > Cc: Eric Dumazet <edumazet@google.com> > > Cc: Andrew Shewmaker <agshew@gmail.com> > > Cc: Glenn Judd <glenn.judd@morganstanley.com> > > --- > > net/ipv4/tcp_dctcp.c | 39 ++++++++++++++++++++++----------------- > > 1 file changed, 22 insertions(+), 17 deletions(-) > > > > diff --git a/net/ipv4/tcp_dctcp.c b/net/ipv4/tcp_dctcp.c > > index cd4814f7e962..60417243e7d7 100644 > > --- a/net/ipv4/tcp_dctcp.c > > +++ b/net/ipv4/tcp_dctcp.c > > @@ -67,10 +67,9 @@ static unsigned int dctcp_alpha_on_init __read_mostly = DCTCP_MAX_ALPHA; > > module_param(dctcp_alpha_on_init, uint, 0644); > > MODULE_PARM_DESC(dctcp_alpha_on_init, "parameter for initial alpha value"); > > > > -static unsigned int dctcp_clamp_alpha_on_loss __read_mostly; > > -module_param(dctcp_clamp_alpha_on_loss, uint, 0644); > > -MODULE_PARM_DESC(dctcp_clamp_alpha_on_loss, > > - "parameter for clamping alpha on loss"); > > +static unsigned int dctcp_halve_cwnd_on_loss __read_mostly; > > +module_param(dctcp_halve_cwnd_on_loss, uint, 0644); > > +MODULE_PARM_DESC(dctcp_halve_cwnd_on_loss, "halve cwnd in case of losses"); > > Is there a reason we still need to keep this module parameter around? > The final RFC even says "A DCTCP sender MUST react to loss episodes in > the same way as conventional TCP". So it's a MUST requirement in which > case it should be enabled by default. The dctcp_clamp_alpha_on_loss was > a bit of a hack from very early days..
I agree with Daniel and Florian
Please respin the patch removing the modparam
Also please check your SOB chain
If we see :
Signed-off-by: Koen De Schepper <koen.de_schepper@nokia-bell-labs.com> Signed-off-by: Olivier Tilmans <olivier.tilmans@nokia-bell-labs.com>
We expect patch author is Koen De Schepper, not Olivier Tilmans
So the patch should start by 'From: Koen De Schepper <koen.de_schepper@nokia-bell-labs.com>' if sent by Olivier.
Thanks !
| |