Re: [PATCH] tcp: fixing TLP's FIN recovery

[Per Hurtig <per.hurtig@kau.se>]

On sön  8 jun 2014 04:58:25, Eric Dumazet wrote:
> On Sat, 2014-06-07 at 16:34 +0200, Per Hurtig wrote:
>> Fix to a problem observed when losing a FIN segment that does not
>> contain data.  In such situations, TLP is unable to recover from
>> *any* tail loss and instead adds at least PTO ms to the
>> retransmission process, i.e., RTO = RTO + PTO.
>>
>> Signed-off-by: Per Hurtig <per.hurtig@kau.se>
>> ---
>>   net/ipv4/tcp_output.c | 6 ++++--
>>   1 file changed, 4 insertions(+), 2 deletions(-)
>>
>> diff --git a/net/ipv4/tcp_output.c b/net/ipv4/tcp_output.c
>> index d463c35..6573765 100644
>> --- a/net/ipv4/tcp_output.c
>> +++ b/net/ipv4/tcp_output.c
>> @@ -2130,8 +2130,10 @@ void tcp_send_loss_probe(struct sock *sk)
>>   	if (WARN_ON(!skb || !tcp_skb_pcount(skb)))
>>   		goto rearm_timer;
>>
>> -	/* Probe with zero data doesn't trigger fast recovery. */
>> -	if (skb->len > 0)
>> +	/* Probe with zero data doesn't trigger fast recovery, if FIN
>> +	 * flag is not set.
>> +	 */
>> +	if ((skb->len > 0) || (TCP_SKB_CB(skb)->tcp_flags & TCPHDR_FIN))
>>   		err = __tcp_retransmit_skb(sk, skb);
>>
>>   	/* Record snd_nxt for loss detection. */
>
>
> You know, I believe the test was exactly to avoid sending data less FIN
> packets.
>
> If you write :
>
>      if (A  || !A)
>
> Better remove the condition, completely ;)
>
Obviously, but I don't think that FINs are the only segments
who are targeted by this condition (or targeted at all given
the implications of this statement). Furthermore, the comment above
the if statement would probably have mentioned FINs explicity
and not zero sized segments in general if this were the case.


>
> Nandita, why FIN packet wont trigger fast retransnmits ?
>

They do, that's the whole thing with this patch.

> It sounds like if the timer is the issue you want to fix, you might
> simply rearm a timer with RTO-PTO instead of RTO ?
>
>
No I want to enable TLP for tail loss where an empty FIN is involved,
this does not work now.

>