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? neal
