Re: [RFC PATCH 1/2] [TCP]: MTUprobe: receiver window & data available checks fixed

[John Heffner <jheffner@psc.edu>]

Ilpo Järvinen wrote:
> It seems that the checked range for receiver window check should
> begin from the first rather than from the last skb that is going
> to be included to the probe. And that can be achieved without
> reference to skbs at all, snd_nxt and write_seq provides the
> correct seqno already. Plus, it SHOULD account packets that are
> necessary to trigger fast retransmit [RFC4821].
> 
> Location of snd_wnd < probe_size/size_needed check is bogus
> because it will cause the other if() match as well (due to
> snd_nxt >= snd_una invariant).
> 
> Removed dead obvious comment.
> 
> Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@helsinki.fi>

Acked-by: John Heffner <jheffner@psc.edu>


> ---
>  net/ipv4/tcp_output.c |   17 ++++++++---------
>  1 files changed, 8 insertions(+), 9 deletions(-)
> 
> diff --git a/net/ipv4/tcp_output.c b/net/ipv4/tcp_output.c
> index 30d6737..ff22ce8 100644
> --- a/net/ipv4/tcp_output.c
> +++ b/net/ipv4/tcp_output.c
> @@ -1289,6 +1289,7 @@ static int tcp_mtu_probe(struct sock *sk)
>  	struct sk_buff *skb, *nskb, *next;
>  	int len;
>  	int probe_size;
> +	int size_needed;
>  	unsigned int pif;
>  	int copy;
>  	int mss_now;
> @@ -1307,6 +1308,7 @@ static int tcp_mtu_probe(struct sock *sk)
>  	/* Very simple search strategy: just double the MSS. */
>  	mss_now = tcp_current_mss(sk, 0);
>  	probe_size = 2*tp->mss_cache;
> +	size_needed = probe_size + (tp->reordering + 1) * mss_now;
>  	if (probe_size > tcp_mtu_to_mss(sk, icsk->icsk_mtup.search_high)) {
>  		/* TODO: set timer for probe_converge_event */
>  		return -1;
> @@ -1316,18 +1318,15 @@ static int tcp_mtu_probe(struct sock *sk)
>  	len = 0;
>  	if ((skb = tcp_send_head(sk)) == NULL)
>  		return -1;
> -	while ((len += skb->len) < probe_size && !tcp_skb_is_last(sk, skb))
> +	while ((len += skb->len) < size_needed && !tcp_skb_is_last(sk, skb))
>  		skb = tcp_write_queue_next(sk, skb);
> -	if (len < probe_size)
> +	if (len < size_needed)
>  		return -1;
>  
> -	/* Receive window check. */
> -	if (after(TCP_SKB_CB(skb)->seq + probe_size, tp->snd_una + tp->snd_wnd)) {
> -		if (tp->snd_wnd < probe_size)
> -			return -1;
> -		else
> -			return 0;
> -	}
> +	if (tp->snd_wnd < size_needed)
> +		return -1;
> +	if (after(tp->snd_nxt + size_needed, tp->snd_una + tp->snd_wnd))
> +		return 0;
>  
>  	/* Do we need to wait to drain cwnd? */
>  	pif = tcp_packets_in_flight(tp);