On Tue, Nov 27, 2018 at 10:57 AM Eric Dumazet <eduma...@google.com> wrote:
>
> Neal pointed out that non sack flows might suffer from ACK compression
> added in the following patch ("tcp: implement coalescing on backlog queue")
>
> Instead of tweaking tcp_add_backlog() we can take into
> account how many ACK were coalesced, this information
> will be available in skb_shinfo(skb)->gso_segs
>
> Signed-off-by: Eric Dumazet <eduma...@google.com>
> ---
...
> @@ -2679,8 +2683,8 @@ static void tcp_process_loss(struct sock *sk, int flag,
> bool is_dupack,
> /* A Reno DUPACK means new data in F-RTO step 2.b above are
> * delivered. Lower inflight to clock out (re)tranmissions.
> */
> - if (after(tp->snd_nxt, tp->high_seq) && is_dupack)
> - tcp_add_reno_sack(sk);
> + if (after(tp->snd_nxt, tp->high_seq))
> + tcp_add_reno_sack(sk, num_dupack);
> else if (flag & FLAG_SND_UNA_ADVANCED)
> tcp_reset_reno_sack(tp);
> }
I think this probably should be checking num_dupack, something like:
+ if (after(tp->snd_nxt, tp->high_seq) && num_dupack)
+ tcp_add_reno_sack(sk, num_dupack);
If we don't check num_dupack, that seems to mean that after FRTO sends
the two new data packets (making snd_nxt after high_seq), the patch
would have a particular non-SACK FRTO loss recovery always go into the
"if" branch where we tcp_add_reno_sack() function, and we would never
have a chance to get to the "else" branch where we check if
FLAG_SND_UNA_ADVANCED and zero out the reno SACKs.
Otherwise the patch looks great to me. Thanks for doing this!
neal
