netdev - Re: [PATCH 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: Windows password security audit tool. GUI, reports in PDF.

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

Date:	Mon, 8 Jun 2015 16:58:32 +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 net-next 2/2] tcp: add CDG congestion control

On Sat, Jun 6, 2015 at 7:01 PM, Yuchung Cheng <ycheng@...gle.com> wrote:
> On Fri, Jun 5, 2015 at 1:23 PM, Kenneth Klette Jonassen
> <kennetkl@....uio.no> wrote:
>>> nice patch. I would like to review it more thoroughly but I have some
>>> quick comments.
>>>
>>> given that cdg didn't include hystart. it'd be nice to have a module knob to
>>> enable and disable hystart for experiments.
>>
>> We could include a knob to disable PRR for delay backoffs as well.
>> Neither are/were available in FreeBSD.
> Seems to belong to a different bigger change to make PRR option for CC
> in general? or is it easy to do in CDG locally?

Yes, I agree. A global option is probably the better alternative. We
could use it to experiment with PRR's effects in other CCs etc.

>
> Understood. My previous comment was not clear: while the shadow_wnd
> can be over the clamp, it's fine because we always clamp the cwnd in
> tcp_reno_cong_avoid()?
>
> In the future if we want to dump shadow_wnd via TCP_CC_INFO, it'll
> be good to know the real value, not the clamped one set by apps.

Ah ok. Yes, we do not need to clamp shadow_wnd in cong_avoid(). We can
change it to only deal with integer overflow instead.

>>>> +       /* If non-ECN: */
>>>> +       if (tp->prior_ssthresh) {
>>> no need to check prior_ssthresh here b/c it's checked in
>>> tcp_undo_cwnd_reduction() already.
>>
>> If we get ECE from the receiver (ECN signal), then cwnd undo is disabled.
>>
>> The above check ensures that the loss tolerance heuristic can only be
>> used if cwnd undo is possible.
>>
>> As an alternative, perhaps ssthresh could be extended with a  second
>> parameter to indicate why it was invoked:
>> ssthresh(sk, TCP_SIGNAL_LOSS)
>> ssthresh(sk, TCP_SIGNAL_ECN)
>> ssthresh(sk, TCP_SIGNAL_PRIVATE)
>>
>> Then tcp_cdg_ssthresh could do:
>> iff signal == TCP_SIGNAL_LOSS && use_tolerance, use loss tolerance heuristic.
>> iff signal == TCP_SIGNAL_PRIVATE, calculate delay backoff cwnd
> maybe passing the ack "flag" is good enough?
>

On second thought, this touches a lot of code for a small benefit.

Is it okay if we just do:
bool is_ecn = (tp->prior_ssthresh == 0);
?
--
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