[MPTCP] Re: [RFC PATCH 11/12] mptcp: allow picking different xmit subflows

[MPTCP] Re: [RFC PATCH 11/12] mptcp: allow picking different xmit subflows

[Florian Westphal]

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

Paolo Abeni <pabeni(a)redhat.com> wrote:
> Update the scheduler to less trivial heuristic: cache
> the last used subflow, and try to send on it a reasonably
> long burst of data. When the burst or the subflow send
> space is exausted, move to the next one with available
> send space.
> 
> Signed-off-by: Paolo Abeni <pabeni(a)redhat.com>
> ---
>  net/mptcp/protocol.c | 112 +++++++++++++++++++++++++++++++++++++------
>  net/mptcp/protocol.h |   6 ++-
>  2 files changed, 101 insertions(+), 17 deletions(-)
> 
> diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
> index ef5b68c4ff49..89c0400593d1 100644
> --- a/net/mptcp/protocol.c
> +++ b/net/mptcp/protocol.c
> @@ -1012,39 +1012,112 @@ static void mptcp_nospace(struct sock *sk, struct sock *ssk)
>  		set_bit(SOCK_NOSPACE, &sock->flags);
>  }
>  
> +static bool mptcp_subflow_active(struct mptcp_subflow_context *subflow,
> +				 bool fallback)
> +{
> +	struct sock *ssk = mptcp_subflow_tcp_sock(subflow);
> +
> +	/* subflow must be open for write */
> +	if ((1 << ssk->sk_state) &
> +	    (TCPF_CLOSE | TCPF_LAST_ACK | TCPF_CLOSING | TCPF_FIN_WAIT2 |
> +	     TCPF_FIN_WAIT1))
> +		return false;

Hmmm, why is this not checking for TCP_ESTABLISHED -> true, rather than
checking 'states i do not want'?

> +	/* we can xmit on MPC and fallen back subflows in
> +	 * TCP_SYN_SENT/TCP_SYN_RECV status, but we need fully established
> +	 * MP_JOIN subflows.
> +	 */

I don't understand this comment.  The part with 'fully established
MP_JOIN' is clear to me.

Is this about SYN_SENT state is fine if ssk tested is the 'msk->first'
subflow?

If so, it might make sense to re-arrange the check to something like
'subflow->request_join && established && subflow->fully_established &&
!fallback' -> return true
else, return state == ESTABLISHED?

or even, ".. else subflow requal to msk->first and state eq ESTABLISHED
return true'?

>  static void ssk_check_wmem(struct sock *sk, struct sock *ssk)
> @@ -1142,6 +1215,7 @@ static int mptcp_sendmsg(struct sock *sk, struct msghdr *msg, size_t len)
>  		}
>  
>  		copied += ret;
> +		msk->snd_burst -= ret;

Maybe add a comment that ret > snd_burst is fine.

>  		tx_ok = msg_data_left(msg);
>  		if (!tx_ok)
> @@ -1358,6 +1432,10 @@ static bool __mptcp_move_skbs(struct mptcp_sock *msk)
>  	unsigned int moved = 0;
>  	bool done;
>  
> +	if (((struct sock *)msk)->sk_state == TCP_CLOSE)
> +		return false;

This looks funny -- why is this needed?

>  static struct sock *mptcp_subflow_get_retrans(const struct mptcp_sock *msk)
>  {
> +	bool fallback = __mptcp_check_fallback(msk);
>  	struct mptcp_subflow_context *subflow;
>  	struct sock *backup = NULL;
>  
> @@ -1521,6 +1600,9 @@ static struct sock *mptcp_subflow_get_retrans(const struct mptcp_sock *msk)
>  	mptcp_for_each_subflow(msk, subflow) {
>  		struct sock *ssk = mptcp_subflow_tcp_sock(subflow);
>  
> +		if (!mptcp_subflow_active(subflow, fallback))
> +			continue;

Hmm, should this be a separate bug fix (prevent non-fully-established-join)?

[MPTCP] Re: [RFC PATCH 11/12] mptcp: allow picking different xmit subflows

[Paolo Abeni]

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

On Sun, 2020-08-02 at 01:50 +0200, Florian Westphal wrote:
> Paolo Abeni <pabeni(a)redhat.com> wrote:
> > Update the scheduler to less trivial heuristic: cache
> > the last used subflow, and try to send on it a reasonably
> > long burst of data. When the burst or the subflow send
> > space is exausted, move to the next one with available
> > send space.
> > 
> > Signed-off-by: Paolo Abeni <pabeni(a)redhat.com>
> > ---
> >  net/mptcp/protocol.c | 112 +++++++++++++++++++++++++++++++++++++------
> >  net/mptcp/protocol.h |   6 ++-
> >  2 files changed, 101 insertions(+), 17 deletions(-)
> > 
> > diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
> > index ef5b68c4ff49..89c0400593d1 100644
> > --- a/net/mptcp/protocol.c
> > +++ b/net/mptcp/protocol.c
> > @@ -1012,39 +1012,112 @@ static void mptcp_nospace(struct sock *sk, struct sock *ssk)
> >  		set_bit(SOCK_NOSPACE, &sock->flags);
> >  }
> >  
> > +static bool mptcp_subflow_active(struct mptcp_subflow_context *subflow,
> > +				 bool fallback)
> > +{
> > +	struct sock *ssk = mptcp_subflow_tcp_sock(subflow);
> > +
> > +	/* subflow must be open for write */
> > +	if ((1 << ssk->sk_state) &
> > +	    (TCPF_CLOSE | TCPF_LAST_ACK | TCPF_CLOSING | TCPF_FIN_WAIT2 |
> > +	     TCPF_FIN_WAIT1))
> > +		return false;
> 
> Hmmm, why is this not checking for TCP_ESTABLISHED -> true, rather than
> checking 'states i do not want'?

I think we can pick a subflow for xmit even after receiving a TCP FIN
on such subflows, so the subflow status could be different from
TCP_ESTABLISHED. 

To my twiested mind was more natural check for not allowed states, I
can flip to allowed one if needed.

> > +	/* we can xmit on MPC and fallen back subflows in
> > +	 * TCP_SYN_SENT/TCP_SYN_RECV status, but we need fully established
> > +	 * MP_JOIN subflows.
> > +	 */
> 
> I don't understand this comment.  The part with 'fully established
> MP_JOIN' is clear to me.
> 
> Is this about SYN_SENT state is fine if ssk tested is the 'msk->first'
> subflow?

yes, exactly.

> If so, it might make sense to re-arrange the check to something like
> 'subflow->request_join && established && subflow->fully_established &&
> !fallback' -> return true
> else, return state == ESTABLISHED?
> 
> or even, ".. else subflow requal to msk->first and state eq ESTABLISHED
> return true'?

I think we need to check for more TCP states than ESTABLISHED for both
first and MP_JOIN subflow. For '->first' we should additionally
allow SYN_SENT and TCP_SYN_RECV. Not sure the above will simplify ?!?
> 
> >  static void ssk_check_wmem(struct sock *sk, struct sock *ssk)
> > @@ -1142,6 +1215,7 @@ static int mptcp_sendmsg(struct sock *sk, struct msghdr *msg, size_t len)
> >  		}
> >  
> >  		copied += ret;
> > +		msk->snd_burst -= ret;
> 
> Maybe add a comment that ret > snd_burst is fine.
> 
> >  		tx_ok = msg_data_left(msg);
> >  		if (!tx_ok)
> > @@ -1358,6 +1432,10 @@ static bool __mptcp_move_skbs(struct mptcp_sock *msk)
> >  	unsigned int moved = 0;
> >  	bool done;
> >  
> > +	if (((struct sock *)msk)->sk_state == TCP_CLOSE)
> > +		return false;
> 
> This looks funny -- why is this needed?

mptcp workqueue is called on a subflow with empty rx queue - flushed by 
__mptcp_close_ssk - and not null data_avail. __mptcp_move_skbs() will
loop forever. I can likely clear data_avail in __mptcp_close_ssk(), but
never tried.

> >  static struct sock *mptcp_subflow_get_retrans(const struct mptcp_sock *msk)
> >  {
> > +	bool fallback = __mptcp_check_fallback(msk);
> >  	struct mptcp_subflow_context *subflow;
> >  	struct sock *backup = NULL;
> >  
> > @@ -1521,6 +1600,9 @@ static struct sock *mptcp_subflow_get_retrans(const struct mptcp_sock *msk)
> >  	mptcp_for_each_subflow(msk, subflow) {
> >  		struct sock *ssk = mptcp_subflow_tcp_sock(subflow);
> >  
> > +		if (!mptcp_subflow_active(subflow, fallback))
> > +			continue;
> 
> Hmm, should this be a separate bug fix (prevent non-fully-established-join)?

I think the bug is only apparent after we have non backup subflows
and/or remove addr support ?!?

reinjection will likely need revisiting, but I haven't looked at that
yet.

/P

[MPTCP] Re: [RFC PATCH 11/12] mptcp: allow picking different xmit subflows

[Florian Westphal]

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

Paolo Abeni <pabeni(a)redhat.com> wrote:
> On Sun, 2020-08-02 at 01:50 +0200, Florian Westphal wrote:
> > Paolo Abeni <pabeni(a)redhat.com> wrote:
> > > Update the scheduler to less trivial heuristic: cache
> > > the last used subflow, and try to send on it a reasonably
> > > long burst of data. When the burst or the subflow send
> > > space is exausted, move to the next one with available
> > > send space.
> > > 
> > > Signed-off-by: Paolo Abeni <pabeni(a)redhat.com>
> > > ---
> > >  net/mptcp/protocol.c | 112 +++++++++++++++++++++++++++++++++++++------
> > >  net/mptcp/protocol.h |   6 ++-
> > >  2 files changed, 101 insertions(+), 17 deletions(-)
> > > 
> > > diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
> > > index ef5b68c4ff49..89c0400593d1 100644
> > > --- a/net/mptcp/protocol.c
> > > +++ b/net/mptcp/protocol.c
> > > @@ -1012,39 +1012,112 @@ static void mptcp_nospace(struct sock *sk, struct sock *ssk)
> > >  		set_bit(SOCK_NOSPACE, &sock->flags);
> > >  }
> > >  
> > > +static bool mptcp_subflow_active(struct mptcp_subflow_context *subflow,
> > > +				 bool fallback)
> > > +{
> > > +	struct sock *ssk = mptcp_subflow_tcp_sock(subflow);
> > > +
> > > +	/* subflow must be open for write */
> > > +	if ((1 << ssk->sk_state) &
> > > +	    (TCPF_CLOSE | TCPF_LAST_ACK | TCPF_CLOSING | TCPF_FIN_WAIT2 |
> > > +	     TCPF_FIN_WAIT1))
> > > +		return false;
> > 
> > Hmmm, why is this not checking for TCP_ESTABLISHED -> true, rather than
> > checking 'states i do not want'?
> 
> I think we can pick a subflow for xmit even after receiving a TCP FIN
> on such subflows, so the subflow status could be different from
> TCP_ESTABLISHED. 
>
> To my twiested mind was more natural check for not allowed states, I
> can flip to allowed one if needed.

I would prefer that.  Otherwise I have to invert manually during review,
and this makes no sense to me when I do the negation:

States:
TCPF_ESTABLISHED = (1 << TCP_ESTABLISHED),
TCPF_SYN_SENT    = (1 << TCP_SYN_SENT),
TCPF_SYN_RECV    = (1 << TCP_SYN_RECV),
TCPF_FIN_WAIT1   = (1 << TCP_FIN_WAIT1),
TCPF_FIN_WAIT2   = (1 << TCP_FIN_WAIT2),
TCPF_TIME_WAIT   = (1 << TCP_TIME_WAIT),
TCPF_CLOSE       = (1 << TCP_CLOSE),
TCPF_CLOSE_WAIT  = (1 << TCP_CLOSE_WAIT),
TCPF_LAST_ACK    = (1 << TCP_LAST_ACK),
TCPF_LISTEN      = (1 << TCP_LISTEN),
TCPF_CLOSING     = (1 << TCP_CLOSING),
TCPF_NEW_SYN_RECV = (1 << TCP_NEW_SYN_RECV),

So, above checks disables:
TCPF_CLOSE       = (1 << TCP_CLOSE),
TCPF_LAST_ACK    = (1 << TCP_LAST_ACK),
TCPF_CLOSING     = (1 << TCP_CLOSING),
TCPF_FIN_WAIT1   = (1 << TCP_FIN_WAIT1),
TCPF_FIN_WAIT2   = (1 << TCP_FIN_WAIT2),

... which makes following ok:

TCPF_ESTABLISHED = (1 << TCP_ESTABLISHED),

Agree on that.

TCPF_SYN_SENT    = (1 << TCP_SYN_SENT),

we don't know yet if connection will complete, or if
mptcp will be available.

TCPF_SYN_RECV    = (1 << TCP_SYN_RECV),

same.

TCPF_TIME_WAIT   = (1 << TCP_TIME_WAIT),

This is also strange, this is a quiesence period to catch
in-flight packets.  I don't think this subflow should still
be used for xmit.

TCPF_CLOSE_WAIT  = (1 << TCP_CLOSE_WAIT),

Agree on this one as well -- its ok to xmit here.

TCPF_LISTEN      = (1 << TCP_LISTEN),

Should not happen of course, but its definitely
not usable :-)

TCPF_NEW_SYN_RECV = (1 << TCP_NEW_SYN_RECV),

and that is... mhhh... strange.  Why is this one ok?

Checking 'fallback' made no sense to me.  Why do we need to check for this?

Am I missing anything?  If yes, please add a comment explaining that
scenario.  Same for fallback, I do not understand how fallback yes/no makes
a difference here.

> > I don't understand this comment.  The part with 'fully established
> > MP_JOIN' is clear to me.
> > 
> > Is this about SYN_SENT state is fine if ssk tested is the 'msk->first'
> > subflow?
> 
> yes, exactly.

Ok.  Would still like to understand how this can occur (data to transmit,
first connection not established yet).

> > If so, it might make sense to re-arrange the check to something like
> > 'subflow->request_join && established && subflow->fully_established &&
> > !fallback' -> return true
> > else, return state == ESTABLISHED?
> > 
> > or even, ".. else subflow requal to msk->first and state eq ESTABLISHED
> > return true'?
> 
> I think we need to check for more TCP states than ESTABLISHED for both
> first and MP_JOIN subflow. For '->first' we should additionally
> allow SYN_SENT and TCP_SYN_RECV. Not sure the above will simplify ?!?

Check for syn sent/recv makes no sense to me.  How can we get
data to send at this point?

I would propose something like:

static bool mptcp_subflow_active(struct mptcp_subflow_context *subflow)
{
  struct sock *ssk = mptcp_subflow_tcp_sock(subflow);

  /* can't send if JOIN hasn't completed yet (i.e. is usable for mptcp) */
  if (subflow->request_join && !subflow->fully_established)
     return false;

  /* only send if our side has not closed yet */
  return ((1 << ssk->sk_state) & (TCPF_ESTABLISHED | TCPF_CLOSE_WAIT));
}

Way less cases/tests.  What breaks/is missing here?

> > > @@ -1358,6 +1432,10 @@ static bool __mptcp_move_skbs(struct mptcp_sock *msk)
> > >  	unsigned int moved = 0;
> > >  	bool done;
> > >  
> > > +	if (((struct sock *)msk)->sk_state == TCP_CLOSE)
> > > +		return false;
> > 
> > This looks funny -- why is this needed?
> 
> mptcp workqueue is called on a subflow with empty rx queue - flushed by 
> __mptcp_close_ssk - and not null data_avail. __mptcp_move_skbs() will
> loop forever. I can likely clear data_avail in __mptcp_close_ssk(), but
> never tried.

Ok, if that doesn't work please just add a small comment with above
explanation.  Thanks.

> > Hmm, should this be a separate bug fix (prevent non-fully-established-join)?
> 
> I think the bug is only apparent after we have non backup subflows
> and/or remove addr support ?!?

What about other mptcp stack peer?  Couldn't they create a non-backup flow?

[ I don't mind if you don't want a separate patch for this, it just
looked like a small fix to me that could be applied already ]

[MPTCP] Re: [RFC PATCH 11/12] mptcp: allow picking different xmit subflows

[Paolo Abeni]

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

On Mon, 2020-08-03 at 14:56 +0200, Florian Westphal wrote:
> Paolo Abeni <pabeni(a)redhat.com> wrote:
> > On Sun, 2020-08-02 at 01:50 +0200, Florian Westphal wrote:
> > > Paolo Abeni <pabeni(a)redhat.com> wrote:
> > > > Update the scheduler to less trivial heuristic: cache
> > > > the last used subflow, and try to send on it a reasonably
> > > > long burst of data. When the burst or the subflow send
> > > > space is exausted, move to the next one with available
> > > > send space.
> > > > 
> > > > Signed-off-by: Paolo Abeni <pabeni(a)redhat.com>
> > > > ---
> > > >  net/mptcp/protocol.c | 112 +++++++++++++++++++++++++++++++++++++------
> > > >  net/mptcp/protocol.h |   6 ++-
> > > >  2 files changed, 101 insertions(+), 17 deletions(-)
> > > > 
> > > > diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
> > > > index ef5b68c4ff49..89c0400593d1 100644
> > > > --- a/net/mptcp/protocol.c
> > > > +++ b/net/mptcp/protocol.c
> > > > @@ -1012,39 +1012,112 @@ static void mptcp_nospace(struct sock *sk, struct sock *ssk)
> > > >  		set_bit(SOCK_NOSPACE, &sock->flags);
> > > >  }
> > > >  
> > > > +static bool mptcp_subflow_active(struct mptcp_subflow_context *subflow,
> > > > +				 bool fallback)
> > > > +{
> > > > +	struct sock *ssk = mptcp_subflow_tcp_sock(subflow);
> > > > +
> > > > +	/* subflow must be open for write */
> > > > +	if ((1 << ssk->sk_state) &
> > > > +	    (TCPF_CLOSE | TCPF_LAST_ACK | TCPF_CLOSING | TCPF_FIN_WAIT2 |
> > > > +	     TCPF_FIN_WAIT1))
> > > > +		return false;
> > > 
> > > Hmmm, why is this not checking for TCP_ESTABLISHED -> true, rather than
> > > checking 'states i do not want'?
> > 
> > I think we can pick a subflow for xmit even after receiving a TCP FIN
> > on such subflows, so the subflow status could be different from
> > TCP_ESTABLISHED. 
> > 
> > To my twiested mind was more natural check for not allowed states, I
> > can flip to allowed one if needed.
> 
> I would prefer that.  Otherwise I have to invert manually during review,
> and this makes no sense to me when I do the negation:
> 
> States:
> TCPF_ESTABLISHED = (1 << TCP_ESTABLISHED),
> TCPF_SYN_SENT    = (1 << TCP_SYN_SENT),
> TCPF_SYN_RECV    = (1 << TCP_SYN_RECV),
> TCPF_FIN_WAIT1   = (1 << TCP_FIN_WAIT1),
> TCPF_FIN_WAIT2   = (1 << TCP_FIN_WAIT2),
> TCPF_TIME_WAIT   = (1 << TCP_TIME_WAIT),
> TCPF_CLOSE       = (1 << TCP_CLOSE),
> TCPF_CLOSE_WAIT  = (1 << TCP_CLOSE_WAIT),
> TCPF_LAST_ACK    = (1 << TCP_LAST_ACK),
> TCPF_LISTEN      = (1 << TCP_LISTEN),
> TCPF_CLOSING     = (1 << TCP_CLOSING),
> TCPF_NEW_SYN_RECV = (1 << TCP_NEW_SYN_RECV),
> 
> So, above checks disables:
> TCPF_CLOSE       = (1 << TCP_CLOSE),
> TCPF_LAST_ACK    = (1 << TCP_LAST_ACK),
> TCPF_CLOSING     = (1 << TCP_CLOSING),
> TCPF_FIN_WAIT1   = (1 << TCP_FIN_WAIT1),
> TCPF_FIN_WAIT2   = (1 << TCP_FIN_WAIT2),
> 
> ... which makes following ok:
> 
> TCPF_ESTABLISHED = (1 << TCP_ESTABLISHED),
> 
> Agree on that.
> 
> TCPF_SYN_SENT    = (1 << TCP_SYN_SENT),
> 
> we don't know yet if connection will complete, or if
> mptcp will be available.
> 
> TCPF_SYN_RECV    = (1 << TCP_SYN_RECV),
> 
> same.
> 
> TCPF_TIME_WAIT   = (1 << TCP_TIME_WAIT),
> 
> This is also strange, this is a quiesence period to catch
> in-flight packets.  I don't think this subflow should still
> be used for xmit.
> 
> TCPF_CLOSE_WAIT  = (1 << TCP_CLOSE_WAIT),
> 
> Agree on this one as well -- its ok to xmit here.
> 
> TCPF_LISTEN      = (1 << TCP_LISTEN),
> 
> Should not happen of course, but its definitely
> not usable :-)
> 
> TCPF_NEW_SYN_RECV = (1 << TCP_NEW_SYN_RECV),
> 
> and that is... mhhh... strange.  Why is this one ok?

Agreed to the above - we also aready have sk_stream_wait_connect() if
state is not TCPF_ESTABLISHED | TCPF_CLOSE_WAIT.

> Checking 'fallback' made no sense to me.  Why do we need to check for this?

In teory we should not have additional subflows in the fallback status,
but we can have an MPJ handskake and fallback racing togethar, so we
end-up with msk in fallback status and one or more fully established
subflows. If/when that happen we must avoid picking mpj subflows. 

Perhaps is just easier/clearer add an explict (unlikely???) case
in mptcp_subflow_get_send() and mptcp_subflow_get_retrans()

> > > Hmm, should this be a separate bug fix (prevent non-fully-established-join)?
> > 
> > I think the bug is only apparent after we have non backup subflows
> > and/or remove addr support ?!?
> 
> What about other mptcp stack peer?  Couldn't they create a non-backup flow?
> 
> [ I don't mind if you don't want a separate patch for this, it just
> looked like a small fix to me that could be applied already ]

yep, we can have non backup subflows with mptcp.org peers. I still
would opt for a single patch ;)

/P

[MPTCP] Re: [RFC PATCH 11/12] mptcp: allow picking different xmit subflows

[Florian Westphal]

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

Paolo Abeni <pabeni(a)redhat.com> wrote:
> > Checking 'fallback' made no sense to me.  Why do we need to check for this?
> 
> In teory we should not have additional subflows in the fallback status,
> but we can have an MPJ handskake and fallback racing togethar, so we
> end-up with msk in fallback status and one or more fully established
> subflows. If/when that happen we must avoid picking mpj subflows. 

That sounds bad.  I'd suggest to make sure that we only use
msk->first if msk is marked fallback.

> Perhaps is just easier/clearer add an explict (unlikely???) case
> in mptcp_subflow_get_send() and mptcp_subflow_get_retrans()

Agree, its easier to understand, at least.

> > > > Hmm, should this be a separate bug fix (prevent non-fully-established-join)?
> > > 
> > > I think the bug is only apparent after we have non backup subflows
> > > and/or remove addr support ?!?
> > 
> > What about other mptcp stack peer?  Couldn't they create a non-backup flow?
> > 
> > [ I don't mind if you don't want a separate patch for this, it just
> > looked like a small fix to me that could be applied already ]
> 
> yep, we can have non backup subflows with mptcp.org peers. I still
> would opt for a single patch ;)

Ok, fair enough, I thought you could split that from the series and
get it merged already.