On Sun, Jul 23, 2017 at 10:36 PM, Neal Cardwell <ncardw...@google.com> wrote: > On Fri, Jul 21, 2017 at 5:16 PM, Yuchung Cheng <ych...@google.com> wrote: >> On Fri, Jul 21, 2017 at 1:46 PM, Neal Cardwell <ncardw...@google.com> wrote: >>> On Fri, Jul 21, 2017 at 4:27 PM, Lisong Xu <x...@unl.edu> 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 <f...@strlen.de> >> 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
