[MPTCP] Re: [RFC PATCH] Squash-to: "tcp: Prevent coalesce/collapse when skb has MPTCP extensions"

[Florian Westphal <fw at strlen.de>]

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

Mat Martineau <mathew.j.martineau(a)linux.intel.com> wrote:
> > moving bits from skb1 to skb2, as the mapping in skb2 will already
> > fit, and mapping in skb1 will be dropped as/if needed.
> > 
> > When coalescing to a newly allocated skb, we transfer the relevant
> > extension to. An MPTCP specific helper is added for that goal,
> > possibly we could use/create a generic one.
> > 
> > Completely untested, just to collect early feedback.
> > 
> > Signed-off-by: Paolo Abeni <pabeni(a)redhat.com>
> > ---
> > include/net/mptcp.h  | 38 ++++++++++++++++++++++++++++++++++++++
> > include/net/tcp.h    |  2 +-
> > net/ipv4/tcp_input.c |  9 +++++----
> > 3 files changed, 44 insertions(+), 5 deletions(-)
> > 
> > diff --git a/include/net/mptcp.h b/include/net/mptcp.h
> > index 43ddfdf9e4a3..8a702e7566e6 100644
> > --- a/include/net/mptcp.h
> > +++ b/include/net/mptcp.h
> > @@ -27,17 +27,55 @@ struct mptcp_ext {
> > 
> > #ifdef CONFIG_MPTCP
> > 
> > +static inline void mptcp_skb_ext_move(struct sk_buff *to,
> > +				      const struct sk_buff *from)
> > +{
> > +	skb_ext_put(to);
> 
> We don't want to accidentally free the ext. Since the refcount is part of
> the ext (not the skb), better to leave it unchanged. It may or may not be
> referenced by other skbs.

Maybe change this so that the extension from the first skb is carried
over.

This should be fine because mptcp_skb_can_collapse() told us
that the extensions are the same.

> > +	to->active_extensions = from->active_extensions;
> > +	to->extensions = from->extensions;
> > +	from->extensions = NULL;
> > +	from->active_extensions = 0;
> > +}
> 
> For the "move" case, do we care if there are active non-MPTCP extensions?

I don't think so.  Current code doesn't copy/consider them and with
exception of mptcp they have no meaning/use anymore at this point.