Re: [MPTCP] [RFC PATCH mptcp-next v2 1/8] mptcp: add skeleton to sync msk socket options to subflows

[Paolo Abeni <pabeni@redhat.com>]

On Wed, 2021-03-24 at 21:01 +0100, Florian Westphal wrote:
> Paolo Abeni <pabeni@redhat.com> wrote:
> > On Wed, 2021-03-24 at 14:15 +0100, Florian Westphal wrote:
> > > Handle following cases:
> > > 1. setsockopt is called with multiple subflows.
> > >    Change might have to be mirrored to all of them.
> > >    This is done directly in process context/setsockopt call.
> > > 2. Outgoing subflow is created after one or several setsockopt()
> > >    calls have been made.  Old setsockopt changes should be
> > >    synced to the new socket.
> > > 3. Incoming subflow, after setsockopt call(s).
> > > 
> > > Cases 2 and 3 are handled right after the join list is spliced to the
> > > conn list.
> > > 
> > > Because splicing is sometimes done from non-preemptible context, the
> > > sync action can be deferred to the work queue.
> > > 
> > > Add sequence numbers to subflow context and mptcp socket so
> > > synchronization functions know which subflow is already updated
> > > and which are not.
> > > 
> > > seqno is re-set to 1 in mptcp_sockopt_sync_all(), at this point
> > > the list of subflows is up to date.
> > > 
> > > A setsockopt sequence count of 0 means that no setsockopt call
> > > was made so no synchronization is needed.
> > > 
> > > Signed-off-by: Florian Westphal <fw@strlen.de>
> > 
> > If I read correctly, each incoming subflow always get setsockopt_seq =
> > 0 at accept time, while outgoing subflow are always synced at creation
> > time.
> 
> Yes, outgoing are synced at creation time.
> 
> > It looks like we have 2 semantically different 'msk->setsockopt_seq'
> > values:
> > * 0 -> no sync required
> > * any value greater than 0 -> sync required for incoming subflows.
> > 
> > If so, setsockopt could always set msk->setsockopt_seq to 1 and no need
> > to reset after sync_all.
> 
> In the outgoing case, the subflow isn't linked to the conn_list yet.

My bad! I did not recall we use join_list even for outgoing subflows...
I'm wondering if we could move them directly in conn_list
inside __mptcp_subflow_connect()? I think it should not be problematic.

> It syncs current msk settings, but user could setsockopt() before
> the connect finishes, in that case another sync is needed after
> connection completes.  No idea how to handle this with single toggle
> bit.

Yep, with outgoing subflow in join_list the thing I suggested above is
not usable.

> > socket()
> > sesockopt()
> > listen()
> > // msks is accepted, msk->setsockopt_seq != 0
> > // mpj subflow is accepted, ssk->setsockopt_seq == 0, the mpj ssk
> > inherited from the listener socket the same sockoptions of the msk/mpc
> > subflow
> 
> I'm not sure all options have this 'inherited from listener' behaviour.

Why not? the subflow socket is cloned from the listener TCP subflow. 

If tcp_create_openreq_child() does not propagate some socket option,
then the expected behaviour is probably not propagating it.

WDYT?

Thanks!

Paolo