Re: [PATCH v2 mptcp-next 2/4] mptcp: stop relaying on tcp_tx_skb_cache.

[Paolo Abeni <pabeni@redhat.com>]

On Sat, 2021-09-04 at 10:00 +0200, Matthieu Baerts wrote:
> Hi Paolo,
> 
> On 02/09/2021 17:52, Paolo Abeni wrote:
> > We want to revert the skb TX cache, but MPTCP is currently
> > using it unconditionally.
> > 
> > Rework the MPTCP tx code, so that tcp_tx_skb_cache is not
> > needed anymore: do the whole coalescing check, skb allocation
> > skb initialization/update inside mptcp_sendmsg_frag(), quite
> > alike the current TCP code.
> 
> (...)
> 
> > @@ -1287,23 +1280,29 @@ static int mptcp_sendmsg_frag(struct sock *sk, struct sock *ssk,
> >  			      struct mptcp_sendmsg_info *info)
> >  {
> >  	u64 data_seq = dfrag->data_seq + info->sent;
> > +	int offset = dfrag->offset + info->sent;
> >  	struct mptcp_sock *msk = mptcp_sk(sk);
> >  	bool zero_window_probe = false;
> >  	struct mptcp_ext *mpext = NULL;
> > -	struct sk_buff *skb, *tail;
> > -	bool must_collapse = false;
> > -	int size_bias = 0;
> > -	int avail_size;
> > -	size_t ret = 0;
> > +	bool can_coalesce = false;
> > +	bool reuse_skb = true;
> > +	struct sk_buff *skb;
> > +	size_t copy;
> > +	int i;
> >  
> >  	pr_debug("msk=%p ssk=%p sending dfrag at seq=%llu len=%u already sent=%u",
> >  		 msk, ssk, dfrag->data_seq, dfrag->data_len, info->sent);
> >  
> > +	if (WARN_ON_ONCE(info->sent > info->limit ||
> > +			 info->limit > dfrag->data_len))
> > +		return 0;
> > +
> >  	/* compute send limit */
> >  	info->mss_now = tcp_send_mss(ssk, &info->size_goal, info->flags);
> > -	avail_size = info->size_goal;
> > +	copy = info->size_goal;
> > +
> >  	skb = tcp_write_queue_tail(ssk);
> > -	if (skb) {
> > +	if (skb && (copy > skb->len)) {
> >  		/* Limit the write to the size available in the
> >  		 * current skb, if any, so that we create at most a new skb.
> >  		 * Explicitly tells TCP internals to avoid collapsing on later
> > @@ -1316,53 +1315,76 @@ static int mptcp_sendmsg_frag(struct sock *sk, struct sock *ssk,
> >  			goto alloc_skb;
> >  		}
> >  
> > -		must_collapse = (info->size_goal - skb->len > 0) &&
> > -				(skb_shinfo(skb)->nr_frags < sysctl_max_skb_frags);
> > -		if (must_collapse) {
> > -			size_bias = skb->len;
> > -			avail_size = info->size_goal - skb->len;
> > +		i = skb_shinfo(skb)->nr_frags;
> > +		can_coalesce = skb_can_coalesce(skb, i, dfrag->page, offset);
> > +		if (!can_coalesce && i >= sysctl_max_skb_frags) {
> > +			tcp_mark_push(tcp_sk(ssk), skb);
> > +			goto alloc_skb;
> >  		}
> > -	}
> >  
> > +		copy -= skb->len;
> > +	} else {
> >  alloc_skb:
> > -	if (!must_collapse && !ssk->sk_tx_skb_cache &&
> > -	    !mptcp_alloc_tx_skb(sk, ssk, info->data_lock_held))
> > -		return 0;
> > +		skb = mptcp_alloc_tx_skb(sk, ssk, info->data_lock_held);
> > +		if (!skb)
> > +			return -ENOMEM;
> > +
> > +		i = skb_shinfo(skb)->nr_frags;
> > +		reuse_skb = false;
> > +		mpext = skb_ext_find(skb, SKB_EXT_MPTCP);
> > +	}
> >  
> >  	/* Zero window and all data acked? Probe. */
> > -	avail_size = mptcp_check_allowed_size(msk, data_seq, avail_size);
> > -	if (avail_size == 0) {
> > +	copy = mptcp_check_allowed_size(msk, data_seq, copy);
> > +	if (copy == 0) {
> >  		u64 snd_una = READ_ONCE(msk->snd_una);
> >  
> > -		if (skb || snd_una != msk->snd_nxt)
> > +		if (skb || snd_una != msk->snd_nxt) {
> 
> If I'm not mistaken, 'skb' will always be != NULL here.
> 
> Should we only have these 2 next lines if 'copy == 0'?

We still need to check for 'snd_una != msk->snd_nxt'. I'll send a
squash-to patch.

Thanks!

Paolo