Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
MIME-Version: 1.0
References: <20190404082055.8981-1-olivier.tilmans@nokia-bell-labs.com> <efdc9c07-1feb-d594-7954-e4e6a379a53d@iogearbox.net>
In-Reply-To: <efdc9c07-1feb-d594-7954-e4e6a379a53d@iogearbox.net>
From:   Eric Dumazet <edumazet@google.com>
Date:   Thu, 4 Apr 2019 02:07:25 -0700
Message-ID: <CANn89iLMuk3eWa_8zqvRrB7quMJyX2Annz0tscp9hB0FPT0Vsw@mail.gmail.com>
Subject: Re: [PATCH net-next] tcp: Ensure DCTCP reacts to losses
To:     Daniel Borkmann <daniel@iogearbox.net>
Cc:     "Tilmans, Olivier (Nokia - BE/Antwerp)" 
        <olivier.tilmans@nokia-bell-labs.com>,
        "De Schepper, Koen (Nokia - BE/Antwerp)" 
        <koen.de_schepper@nokia-bell-labs.com>,
        Bob Briscoe <research@bobbriscoe.net>,
        Lawrence Brakmo <brakmo@fb.com>,
        Florian Westphal <fw@strlen.de>,
        Daniel Borkmann <borkmann@iogearbox.net>,
        Yuchung Cheng <ycheng@google.com>,
        Neal Cardwell <ncardwell@google.com>,
        Andrew Shewmaker <agshew@gmail.com>,
        Glenn Judd <glenn.judd@morganstanley.com>,
        "David S. Miller" <davem@davemloft.net>,
        Alexey Kuznetsov <kuznet@ms2.inr.ac.ru>,
        Hideaki YOSHIFUJI <yoshfuji@linux-ipv6.org>,
        "netdev@vger.kernel.org" <netdev@vger.kernel.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

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 =C2=A73.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
> >         =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D|=3D=3D=
=3D=3D=3D=3D=3D=3D=3D|=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D|=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D
> >                     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_mostl=
y =3D DCTCP_MAX_ALPHA;
> >  module_param(dctcp_alpha_on_init, uint, 0644);
> >  MODULE_PARM_DESC(dctcp_alpha_on_init, "parameter for initial alpha val=
ue");
> >
> > -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 loss=
es");
>
> 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 !