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

["Sébastien Barré" <sebastien.barre@uclouvain.be>]

Le 08/01/2015 16:19, Eric Dumazet a écrit :
> On Thu, 2015-01-08 at 13:20 +0100, Sébastien Barré wrote:
>> When the peer has delayed ack enabled, it may reply to a probe with an
>> ACK+D-SACK, with ack value set to tlp_high_seq. In the current code,
>> such ACK+DSACK will be missed and only at next, higher ack will the TLP
>> episode be considered done. Since the DSACK is not present anymore,
>> this will cost a cwnd reduction.
>>
> ...
>>   net/ipv4/tcp_input.c | 30 +++++++++++++-----------------
>>   1 file changed, 13 insertions(+), 17 deletions(-)
>>
>> diff --git a/net/ipv4/tcp_input.c b/net/ipv4/tcp_input.c
>> index 075ab4d..cf63a29 100644
>> --- a/net/ipv4/tcp_input.c
>> +++ b/net/ipv4/tcp_input.c
>> @@ -3363,29 +3363,25 @@ static void tcp_replace_ts_recent(struct tcp_sock *tp, u32 seq)
>>   static void tcp_process_tlp_ack(struct sock *sk, u32 ack, int flag)
>>   {
>>   	struct tcp_sock *tp = tcp_sk(sk);
>> -	bool is_tlp_dupack = (ack == tp->tlp_high_seq) &&
>> -			     !(flag & (FLAG_SND_UNA_ADVANCED |
>> -				       FLAG_NOT_DUP | FLAG_DATA_SACKED));
>>   
> No idea why you removed this bool, it really helped to understand the
> code. This makes your patch looks more complex than needed.
I did not remove it in v1 
(http://www.spinics.net/lists/netdev/msg308653.html)
The removal was a request from Neal 
(http://www.spinics.net/lists/netdev/msg308758.html)

I think he found it special to have part of the logic apart, in a bool, 
and part of it in the if condition.
One possible option is to restore is_tlp_dupack and include the DSACK 
check in it, although this will
not do much more than moving complexity in the bool definition. But 
indeed, that might make
the patch more readable.

What do you and Neal think ?

regards,

Sébastien.
>
>>   	/* Mark the end of TLP episode on receiving TLP dupack or when
>>   	 * ack is after tlp_high_seq.
>> +	 * With delayed acks, we may also get a regular ACK+DSACK, in which
>> +	 * case we don't want to reduce the cwnd either.
>>   	 */
>> -	if (is_tlp_dupack) {
>> +	if (((ack == tp->tlp_high_seq) &&
>> +	     !(flag & (FLAG_SND_UNA_ADVANCED |
>> +		       FLAG_NOT_DUP | FLAG_DATA_SACKED))) ||
>> +	    (!before(ack, tp->tlp_high_seq) && (flag & FLAG_DSACKING_ACK))) {
>>   		tp->tlp_high_seq = 0;
>> -		return;
>> -	}
>