netdev - Re: [PATCH net-next] tcp: Fix CWV being too strict on thin streams

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] [thread-next>] [day] [month] [year] [list]

Date:	Wed, 23 Sep 2015 16:46:56 +0200
From:	Bendik Rønning Opstad <bro.devel@...il.com>
To:	Neal Cardwell <ncardwell@...gle.com>
Cc:	Eric Dumazet <eric.dumazet@...il.com>,
	"David S. Miller" <davem@...emloft.net>,
	Alexey Kuznetsov <kuznet@....inr.ac.ru>,
	James Morris <jmorris@...ei.org>,
	Hideaki YOSHIFUJI <yoshfuji@...ux-ipv6.org>,
	Patrick McHardy <kaber@...sh.net>,
	Netdev <netdev@...r.kernel.org>,
	Eric Dumazet <edumazet@...gle.com>,
	Andreas Petlund <apetlund@...ula.no>,
	Carsten Griwodz <griff@...ula.no>,
	Jonas Markussen <jonassm@....uio.no>,
	Kenneth Klette Jonassen <kennetkl@....uio.no>,
	Yuchung Cheng <ycheng@...gle.com>
Subject: Re: [PATCH net-next] tcp: Fix CWV being too strict on thin streams


Neal, Eric, sorry for the late replies, but keeping up with your speedy replies
is a full time job :-)

The packetdrill scripts are certainly useful to test this, so thanks for
supplying those!

On Tuesday, September 22, 2015 04:04:37 PM you wrote:
> On Tue, Sep 22, 2015 at 2:02 PM, Neal Cardwell <ncardwell@...gle.com> wrote:
> > More generally, my sense is that we should tweak the is_cwnd_limited code to
> > shift from saying "set is_cwnd_limited to true iff the cwnd is known to be
> > limiting transmits" to saying "set is_cwnd_limited to true iff the packets in
> > flight fill the cwnd".

A fully agree. This ensures that a failed attempt at transmitting a packet
is not required for the connection to be considered CWND limited.

> Here is a proposed patch, and a packetdrill test case based on Eric's,
> trying to capture the flavor of your Scenario 1. The test shows how
> this proposed variant allows the sender (Reno in this case, for
> simplicity) to increase cwnd each RTT in congestion avoidance, since
> the cwnd is fully utilized, even though we run out of packets to send
> and available cwnd at exactly the same moment.

This is actually very close to a variation of the patch we experimented with,
where we added a test before the call to tcp_cwnd_validate() to set
is_cwnd_limited. However, to avoid the extra procedure for every time one or
more packets are transmitted (which we presumed would be preferable for
performance) we ended up with updating the value only when no packets were sent.
If this is not a problem, then updating the value before the call to
tcp_cwnd_validate() will be better and also more correct according to RFC2861 as
you mentioned.

> Bendik, does this fix your scenario? How does this strike you as a
> potential fix?

This fixes the identified problem and is a good fix as far as our tests show.

Should I resend this as PATCH v2?

> -------------------------------------
> 
> diff --git a/net/ipv4/tcp_output.c b/net/ipv4/tcp_output.c
> index 4cd0b50..57a586f 100644
> --- a/net/ipv4/tcp_output.c
> +++ b/net/ipv4/tcp_output.c
> @@ -1827,7 +1827,7 @@ static bool tcp_tso_should_defer(struct sock
> *sk, struct sk_buff *skb,
> 
>         /* Ok, it looks like it is advisable to defer. */
> 
> -       if (cong_win < send_win && cong_win < skb->len)
> +       if (cong_win < send_win && cong_win <= skb->len)
>                 *is_cwnd_limited = true;
> 
>         return true;
> @@ -2060,7 +2060,6 @@ static bool tcp_write_xmit(struct sock *sk,
> unsigned int mss_now, int nonagle,
> 
>                 cwnd_quota = tcp_cwnd_test(tp, skb);
>                 if (!cwnd_quota) {
> -                       is_cwnd_limited = true;
>                         if (push_one == 2)
>                                 /* Force out a loss probe pkt. */
>                                 cwnd_quota = 1;
> @@ -2142,6 +2141,7 @@ repair:
>                 /* Send one loss probe per tail loss episode. */
>                 if (push_one != 2)
>                         tcp_schedule_loss_probe(sk);
> +               is_cwnd_limited |= (tcp_packets_in_flight(tp) >= tp->snd_cwnd);
>                 tcp_cwnd_validate(sk, is_cwnd_limited);
>                 return false;
>         }
> 

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