netdev - Re: [PATCH v2 net-next 2/2] tcp: add CDG congestion control

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Wed, 10 Jun 2015 00:41:26 +0200
From:	Kenneth Klette Jonassen <kennetkl@....uio.no>
To:	Yuchung Cheng <ycheng@...gle.com>
Cc:	Kenneth Klette Jonassen <kennetkl@....uio.no>,
	netdev <netdev@...r.kernel.org>,
	Eric Dumazet <edumazet@...gle.com>,
	Stephen Hemminger <stephen@...workplumber.org>,
	Neal Cardwell <ncardwell@...gle.com>,
	David Hayes <davihay@....uio.no>,
	Andreas Petlund <apetlund@...ula.no>,
	Dave Taht <dave.taht@...ferbloat.net>,
	Nicolas Kuhn <nicolas.kuhn@...ecom-bretagne.eu>
Subject: Re: [PATCH v2 net-next 2/2] tcp: add CDG congestion control

On Tue, Jun 9, 2015 at 7:44 AM, Yuchung Cheng <ycheng@...gle.com> wrote:
> On Mon, Jun 8, 2015 at 10:43 AM, Kenneth Klette Jonassen
> <kennetkl@....uio.no> wrote:
…

>> +enum cdg_state {
>> +       CDG_UNKNOWN = 0,
>> +       CDG_FULL    = 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 = tp->snd_cwnd;
>> +       tcp_reno_cong_avoid(sk, ack, acked);
>> +
>> +       if (ca->shadow_wnd) {
>> +               u32 incr = tp->snd_cwnd - prior_snd_cwnd;
>> +
>> +               ca->shadow_wnd = max(ca->shadow_wnd, ca->shadow_wnd + 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 of
shadow_wnd.

We limit shadow_wnd to min(shadow_wnd / 2, snd_cwnd) in ssthresh().

>> +       if (ca->state == CDG_BACKOFF)
>> +               return max(2U, (tp->snd_cwnd * min(1024U, backoff_beta)) >> 10);
>> +
>> +       if (ca->state == CDG_NONFULL && use_tolerance) {
>> +               bool is_ecn = (tp->prior_ssthresh == 0);
>> +
>> +               if (!is_ecn) {
>> +                       tp->ecn_flags &= ~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 = 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 signal.

Inferring ECN from (prior_ssthresh == 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.

>> +       if (use_shadow)
>> +               ca->shadow_wnd = min(ca->shadow_wnd >> 1, tp->snd_cwnd);
>> +       else
>> +               ca->shadow_wnd = 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.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html