Re: [MPTCP] [PATCH 3/3] mptcp: allow collapsing consecutive sendpages on the same substream

[Paolo Abeni <pabeni at redhat.com>]

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

Hi,

Thank you for the feedback

On Thu, 2019-04-18 at 16:56 -0700, Mat Martineau wrote:
> @@ -78,25 +91,48 @@ static int mptcp_sendmsg_frag(struct sock *sk, struct sock *ssk,
> > 	if (!psize)
> > 		return -EINVAL;
> > 
> > -	/* Mark the end of the previous write so the beginning of the
> > -	 * next write (with its own mptcp skb extension data) is not
> > -	 * collapsed.
> > -	 */
> > +	mss_now = tcp_send_mss(ssk, &size_goal, msg->msg_flags);
> > +	*pmss_now = mss_now;
> > +	*ps_goal = size_goal;
> > +	avail_size = size_goal;
> > 	skb = tcp_write_queue_tail(ssk);
> > -	if (skb)
> > -		TCP_SKB_CB(skb)->eor = 1;
> > +	if (skb) {
> > +		mpext = skb_ext_find(skb, SKB_EXT_MPTCP);
> > +		BUG_ON(!mpext);
> > +
> > +		/* Limit the write to the size available in the
> > +		 * current skb, if any, so that we create at most a new skb.
> > +		 * If we run out of space in the current skb (e.g. the window
> > +		 * size shrunk from last sent) a new skb will be allocated even
> > +		 * is collapsing was allowed: collapsing is effectively
> > +		 * disabled.
> > +		 */
> > +		can_collapse = mptcp_skb_can_collapse_to(msk, skb, mpext);
> > +		if (!can_collapse)
> > +			TCP_SKB_CB(skb)->eor = 1;
> > +		else if (size_goal - skb->len > 0)
> > +			avail_size = size_goal - skb->len;
> > +		else
> > +			can_collapse = false;
> 
> In this final clause, should it set eor as well? If we're not expecting a 
> collapse, it might be better to make sure it does not happen.

yes, we could set eor here, but if there is no available size in the
current skb, collapsing can't happen, otherwise is a bug. I think
setting it will not add additional safety.

> > -	if (skb == tcp_write_queue_tail(ssk))
> > -		pr_err("no new skb %p/%p", sk, ssk);
> > +	collapsed = skb == tcp_write_queue_tail(ssk);
> > +	BUG_ON(collapsed && !can_collapse);
> > 
> > 	skb = tcp_write_queue_tail(ssk);
> 
> Minor: seems like this skb assignment is coupled with the mpext line below 
> rather than the conditional to update mpext and exit, and could be moved 
> after the collapse check.

Agreed, I will do that in the next iteration.

Final note: it looks like this patch is not the root cause of the
stream corruption I was observing, it just uncovers a bug present in
current code. I'll try to send a fix soon.

Thanks,

Paolo