netdev - Re: [PATCH net-next v2] tcp: avoid reducing cwnd when ACK+DSACK is received

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:	Sat, 10 Jan 2015 12:51:49 +0100
From:	Sébastien Barré <sebastien.barre@...ouvain.be>
To:	Yuchung Cheng <ycheng@...gle.com>,
	Eric Dumazet <eric.dumazet@...il.com>
CC:	Neal Cardwell <ncardwell@...gle.com>,
	David Miller <davem@...emloft.net>,
	Netdev <netdev@...r.kernel.org>,
	Gregory Detal <gregory.detal@...ouvain.be>,
	Nandita Dukkipati <nanditad@...gle.com>
Subject: Re: [PATCH net-next v2] tcp: avoid reducing cwnd when ACK+DSACK is
 received

All,

Le 09/01/2015 20:43, Yuchung Cheng a écrit :
>
> Sebastien: I suggest breaking down by ACK types for readability. e.g.,
>
> /* This routine deals with acks during a TLP episode.
>   * We mark the end of a TLP episode on receiving TLP dupack or when
>   * ack is after tlp_high_seq.
>   * Ref: loss detection algorithm in draft-dukkipati-tcpm-tcp-loss-probe.
>   */
> static void tcp_process_tlp_ack(struct sock *sk, u32 ack, int flag)
> {
>          struct tcp_sock *tp = tcp_sk(sk);
>
>          if (before(ack, tp->tlp_high_seq))
>                  return;
>
>          if (flag & FLAG_DSACKING_ACK) {
>                  /* This DSACK means original and TLP probe arrived; no loss */
>                  tp->tlp_high_seq = 0;
>          } else if (after(ack, tp->tlp_high_seq)) {
>                  /* ACK advances: there was a loss, so reduce cwnd. Reset
>                   * tlp_high_seq in tcp_init_cwnd_reduction()
Indeed, hadn't seen that.
>                   */
>                  tcp_init_cwnd_reduction(sk);
>                  tcp_set_ca_state(sk, TCP_CA_CWR);
>                  tcp_end_cwnd_reduction(sk);
>                  tcp_try_keep_open(sk);
>                  NET_INC_STATS_BH(sock_net(sk),
>                                   LINUX_MIB_TCPLOSSPROBERECOVERY);
>          } else if (!(flag & (FLAG_SND_UNA_ADVANCED |
>                               FLAG_NOT_DUP | FLAG_DATA_SACKED))) {
>                  /* Pure dupack: original and TLP probe arrived; no loss */
>                  tp->tlp_high_seq = 0;
>          }
> }
That looks much more readable compared to my v2.
It is currently passing our tests (These are in fact MPTCP tests appart 
from Neal's packetdrill that I will add, but actually the MPTCP stack 
happens to reveal this situation quite easily, I think because in MPTCP, 
we store the send queue in the "meta-flow", which currently cannot be 
used for tail loss probes).

As probably everyone will be happy with this (Eric as well ?), I suggest 
I prepare a v3 once all our tests are passed as well, with Yuchung's 
structure and Neal's packetdrill test in the commit text. Will also add 
proper credit as there is now stuff from several people in those few 
lines now :-).

Looks good ?

Thanks again for your fast and helpful interactions !

Sébastien.

>
>>

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