[MPTCP] Re: [RFC PATCH 07/12] mptcp: cleanup mptcp_subflow_discard_data()

[Florian Westphal <fw at strlen.de>]

[-- Attachment #1: Type: text/plain, Size: 1254 bytes --]

Paolo Abeni <pabeni(a)redhat.com> wrote:

Hmmm, a changelog would have been nice 8-)

> -	/* discard mapped data */
> -	pr_debug("discarding %zu bytes, current map len=%d", delta,
> -		 map_remaining);
> -	if (delta) {
> -		read_descriptor_t desc = {
> -			.count = delta,
> -		};
> -		int ret;
> -
> -		ret = tcp_read_sock(ssk, &desc, subflow_read_actor);

tcp_read_sock tosses 'delta' bytes, and may zap multiple skbs, but after
this change only at most one skb gets tossed.

>  	return 0;
>  }

Nit: After this change, mptcp_subflow_discard_data() always returns 0.

> @@ -853,7 +824,7 @@ static bool subflow_check_data_avail(struct sock *ssk)
>  		/* only accept in-sequence mapping. Old values are spurious
>  		 * retransmission
>  		 */
> -		if (mptcp_subflow_discard_data(ssk, old_ack - ack_seq))
> +		if (mptcp_subflow_discard_data(ssk, skb, old_ack - ack_seq))
>  			goto fatal;

... so this conditional can be removed.

>  		return false;

Wouldn't it make sense to get rid of the return false too so that this
checks if there is another skb to process?  This would also obsolete my
'may zap multiple skbs' comment above since the caller would invoke the
discard helper again if next skb is also outdated.