From mboxrd@z Thu Jan 1 00:00:00 1970 From: Yuchung Cheng Subject: Re: [PATCH v2 net-next 2/2] tcp: add CDG congestion control Date: Tue, 9 Jun 2015 16:25:20 -0700 Message-ID: References: <1433785420-4191-1-git-send-email-kennetkl@ifi.uio.no> <1433785420-4191-2-git-send-email-kennetkl@ifi.uio.no> Mime-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: QUOTED-PRINTABLE Cc: netdev , Eric Dumazet , Stephen Hemminger , Neal Cardwell , David Hayes , Andreas Petlund , Dave Taht , Nicolas Kuhn To: Kenneth Klette Jonassen Return-path: Received: from mail-oi0-f45.google.com ([209.85.218.45]:35545 "EHLO mail-oi0-f45.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752587AbbFIX0B convert rfc822-to-8bit (ORCPT ); Tue, 9 Jun 2015 19:26:01 -0400 Received: by oihd6 with SMTP id d6so21412948oih.2 for ; Tue, 09 Jun 2015 16:26:00 -0700 (PDT) In-Reply-To: Sender: netdev-owner@vger.kernel.org List-ID: On Tue, Jun 9, 2015 at 3:41 PM, Kenneth Klette Jonassen wrote: > On Tue, Jun 9, 2015 at 7:44 AM, Yuchung Cheng wro= te: >> On Mon, Jun 8, 2015 at 10:43 AM, Kenneth Klette Jonassen >> wrote: > =E2=80=A6 > >>> +enum cdg_state { >>> + CDG_UNKNOWN =3D 0, >>> + CDG_FULL =3D 0, >> why duplicate states? > > We explicitly infer a full or non-full queue in tcp_cdg_grad(). > > The unknown state is semantically different; it is before we infer > anything about the network. > > We could change the full state to have its own value. (It does not > matter in the current code, but it could be useful for TCP_CC_INFO.) > >>> + if (hystart_detect & 1) { >> Define 1 and 2 like cubic does? >>> + prior_snd_cwnd =3D tp->snd_cwnd; >>> + tcp_reno_cong_avoid(sk, ack, acked); >>> + >>> + if (ca->shadow_wnd) { >>> + u32 incr =3D tp->snd_cwnd - prior_snd_cwnd; >>> + >>> + ca->shadow_wnd =3D max(ca->shadow_wnd, ca->shadow_w= nd + incr); >> u32 shadow_wnd may overflow?! there might be some bugs ... > > CDG can potentially operate without losses over some time, so > shadow_wnd can reach U32_MAX on normal links. > The max() should revert any increment that causes overflow/wrapping o= f > shadow_wnd. the situation you described could only happen if we didn't have the application-limited check (i.e., tcp_is_cwnd_limited() in tcp_reno_con= g_avoid). I am really doubtful, even tho having a max won't hurt anything. > > We limit shadow_wnd to min(shadow_wnd / 2, snd_cwnd) in ssthresh(). > >>> + if (ca->state =3D=3D CDG_BACKOFF) >>> + return max(2U, (tp->snd_cwnd * min(1024U, backoff_b= eta)) >> 10); >>> + >>> + if (ca->state =3D=3D CDG_NONFULL && use_tolerance) { >>> + bool is_ecn =3D (tp->prior_ssthresh =3D=3D 0); >>> + >>> + if (!is_ecn) { >>> + tp->ecn_flags &=3D ~TCP_ECN_DEMAND_CWR; >>> + return tp->snd_cwnd; >>> + } >> I might be missing something here: cdg_backoff also calls >> tcp_enter_cwr() that clears prior_ssthresh but it's not triggered by >> ECE marks? > > That is ok. We set ca->state =3D CDG_BACKOFF in tcp_cdg_backoff() to > distinguish a delay backoff. > >> >> if the intention is to use loss tolerance only when undo is enabled >> why not just check undo_marker? > > The intention is to not use loss tolerance if we receive an ECN signa= l. > > Inferring ECN from (prior_ssthresh =3D=3D 0) is far from perfect of > course, but it is always cleared when receiving ECN. > > In tcp_enter_loss(), undo_marker is set after calling ssthresh(). We > could change that, but undo_marker is also imperfect for detecting > ECN. > > If in doubt, let us remove the ECN check now. Then in a later patch > set, we could try passing flag to ssthresh like you suggested. We > should also look at tcp_dctcp if we do that. +1 to split ecn handling in a separate (future) patch > >>> + if (use_shadow) >>> + ca->shadow_wnd =3D min(ca->shadow_wnd >> 1, tp->snd= _cwnd); >>> + else >>> + ca->shadow_wnd =3D 0; >> it'd be good to not reset shadow_wnd, but only use it if use_shadow. >> so we can monitor shadow_wnd >> in TCP_CC_INFO in the future. > > Ok, then we should always set/maintain a shadow_wnd.