On 11/27/2018 01:19 PM, Neal Cardwell wrote:
> 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!
>
Oh right, I missed the else clause, I thought that tcp_add_reno_sack()
checking the num_dupack was enough.
Thanks.
