From mboxrd@z Thu Jan 1 00:00:00 1970 From: Neal Cardwell Subject: Re: A buggy behavior for Linux TCP Reno and HTCP Date: Sun, 23 Jul 2017 22:37:35 -0400 Message-ID: References: Mime-Version: 1.0 Content-Type: text/plain; charset="UTF-8" Cc: Lisong Xu , Wei Sun , netdev To: Yuchung Cheng Return-path: Received: from mail-oi0-f47.google.com ([209.85.218.47]:33260 "EHLO mail-oi0-f47.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751997AbdGXCiH (ORCPT ); Sun, 23 Jul 2017 22:38:07 -0400 Received: by mail-oi0-f47.google.com with SMTP id a9so13433286oih.0 for ; Sun, 23 Jul 2017 19:38:06 -0700 (PDT) In-Reply-To: Sender: netdev-owner@vger.kernel.org List-ID: On Sun, Jul 23, 2017 at 10:36 PM, Neal Cardwell wrote: > On Fri, Jul 21, 2017 at 5:16 PM, Yuchung Cheng wrote: >> On Fri, Jul 21, 2017 at 1:46 PM, Neal Cardwell wrote: >>> On Fri, Jul 21, 2017 at 4:27 PM, Lisong Xu wrote: >>>> >>>> Hi Yuchung, >>>> >>>> This test scenario is only one example to trigger this bug. In general, as >>>> long as cwnd <4, the undo function has this bug. >>> >>> >>> Yes, personally I agree that this seems like an issue that is general enough >>> to be worth fixing. In the sense that, if cwnd <4, then we may well be very >>> congested. So we don't want to get hit by this bug wherein an undo of a loss >>> recovery can cause cwnd to suddenly jump (from 1, 2, or 3) up to 4. >>> >>> Seems like any of the several CCs that use tcp_reno_undo_cwnd() have this >>> bug. >>> >>> I guess in my mind the only question is whether we want to add a >>> tcp_foo_undo_cwnd() and ca->loss_cwnd to every CC module to handle this >>> issue (i.e. make every CC module handle it the way CUBIC does), or (my >> I would prefer the former b/c loss_cwnd may not be universal TCP >> state, just like ssthresh carries no meaning in some CC (bbr). It also >> seems also more consistent with the recent change on undo >> >> commit e97991832a4ea4a5f47d65f068a4c966a2eb5730 >> Author: Florian Westphal >> Date: Mon Nov 21 14:18:38 2016 +0100 >> >> tcp: make undo_cwnd mandatory for congestion modules >> > > You are certainly right that it is more pure to keep a CC detail like > that inside the CC module. > > But it's a bit sad to me that we have 9 separate identical > implementations of a cwnd undo function, and that approach would add 6 > more. > > We do have tp->snd_ssthresh and tp->prior_ssthresh, even though not > all CC modules use ssthresh. > > What if we call the field tp->prior_cwnd? Then at least we'd have some > nice symmetry: > > - tp->snd_cwnd, which is saved in tp->prior_cwnd (and restored upon undo) > - tp->snd_ssthresh, which is saved in tp-> prior_ssthresh (and > restored upon undo) > > That sounds appealing to me. WDYT? And, I should add, if we go with the tp->prior_cwnd approach, then we can have a single "default"/CUBIC-style undo function, instead of 15 separate but identical implementations... neal