On Mon, 24 Jul 2017 16:41:12 -0700
Yuchung Cheng <ych...@google.com> wrote:
> On Mon, Jul 24, 2017 at 11:29 AM, Neal Cardwell <ncardw...@google.com> wrote:
> > On Mon, Jul 24, 2017 at 2:17 PM, Yuchung Cheng <ych...@google.com> wrote:
> >> 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:
> > ...
> >>>> 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.
> >
> > Yes, exactly.
> >
> > Right now we have 9 modules that have identical tcp_foo_cwnd_undo functions:
> >
> > tcp_bic.c:188: return max(tp->snd_cwnd, ca->loss_cwnd);
> > tcp_cubic.c:378: return max(tcp_sk(sk)->snd_cwnd, ca->loss_cwnd);
> > tcp_dctcp.c:318: return max(tcp_sk(sk)->snd_cwnd, ca->loss_cwnd);
> > tcp_highspeed.c:165: return max(tcp_sk(sk)->snd_cwnd, ca->loss_cwnd);
> > tcp_illinois.c:309: return max(tcp_sk(sk)->snd_cwnd, ca->loss_cwnd);
> > tcp_nv.c:190: return max(tcp_sk(sk)->snd_cwnd, ca->loss_cwnd);
> > tcp_scalable.c:50: return max(tcp_sk(sk)->snd_cwnd, ca->loss_cwnd);
> > tcp_veno.c:210: return max(tcp_sk(sk)->snd_cwnd, veno->loss_cwnd);
> > tcp_yeah.c:232: return max(tcp_sk(sk)->snd_cwnd, yeah->loss_cwnd);
> >
> > And if we fix this bug in tcp_reno_undo_cwnd() by referring to
> > ca->loss_cwnd then we will add another 6 like this.
> >
> > So my proposal would be
> >
> > - 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)
> >
> > Actually, now that I re-read the code, we already do have a
> > prior_cwnd, which is used for the PRR code, and already set upon
> > entering CA_Recovery. So if we set prior_cwnd for CA_Loss, perhaps we
> > can do something like:
> >
> > diff --git a/net/ipv4/tcp_cong.c b/net/ipv4/tcp_cong.c
> > index fde983f6376b..c2b174469645 100644
> > --- a/net/ipv4/tcp_cong.c
> > +++ b/net/ipv4/tcp_cong.c
> > @@ -456,7 +456,7 @@ u32 tcp_reno_undo_cwnd(struct sock *sk)
> > {
> > const struct tcp_sock *tp = tcp_sk(sk);
> >
> > - return max(tp->snd_cwnd, tp->snd_ssthresh << 1);
> > + return max(tp->snd_cwnd, tp->prior_cwnd);
> > }
> > EXPORT_SYMBOL_GPL(tcp_reno_undo_cwnd);
> >
> > diff --git a/net/ipv4/tcp_input.c b/net/ipv4/tcp_input.c
> > index 2920e0cb09f8..ae790a84302d 100644
> > --- a/net/ipv4/tcp_input.c
> > +++ b/net/ipv4/tcp_input.c
> > @@ -1951,6 +1951,7 @@ void tcp_enter_loss(struct sock *sk)
> > !after(tp->high_seq, tp->snd_una) ||
> > (icsk->icsk_ca_state == TCP_CA_Loss &&
> > !icsk->icsk_retransmits)) {
> > tp->prior_ssthresh = tcp_current_ssthresh(sk);
> > + tp->prior_cwnd = tp->snd_cwnd;
> > tp->snd_ssthresh = icsk->icsk_ca_ops->ssthresh(sk);
> > tcp_ca_event(sk, CA_EVENT_LOSS);
> > tcp_init_undo(tp);
> >
> > And then change all the CC modules but BBR to use the
> > tcp_reno_undo_cwnd() instead of their own custom undo code.
> >
> > WDYT?
> Looks reasonable. But we might want to eventually refactor TCP undo
> code: the stats changes (prior_ssthresh, prior_cwnd, undo_marker,
> undo_retrans) are scattered in different helpers, making the code hard
> to audit.
I like having common code as much as possible,
having per CC undo means more variations and sources of errors.
