On Sun, Jul 23, 2017 at 7:37 PM, Neal Cardwell <ncardw...@google.com> wrote: > 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... you mean all CC modules share one ca_ops->undo_cwnd function? sounds a nice consolidation work.
> > neal
