[MPTCP] Re: [PATCH net-next 8/8] mptcp: defer work schedule until mptcp lock is released

[MPTCP] Re: [PATCH net-next 8/8] mptcp: defer work schedule until mptcp lock is released

[Florian Westphal]

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

Paolo Abeni <pabeni(a)redhat.com> wrote:
> On Tue, 2020-02-25 at 13:05 +0100, Florian Westphal wrote:
> > Don't schedule the work queue right away, instead defer this
> > to the lock release callback.
> > 
> > This has the advantage that it will give recv path a chance to
> > complete -- this might have moved all pending packets from the
> > subflow to the mptcp receive queue, which allows to avoid another
> > schedule_work().
> > 
> > Signed-off-by: Florian Westphal <fw(a)strlen.de>
> > ---
> >  net/mptcp/protocol.c | 20 ++++++++++++++++++--
> >  1 file changed, 18 insertions(+), 2 deletions(-)
> > 
> > diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
> > index de6bd9d28116..f7c0d205b047 100644
> > --- a/net/mptcp/protocol.c
> > +++ b/net/mptcp/protocol.c
> > @@ -240,8 +240,15 @@ void mptcp_data_ready(struct sock *sk, struct sock *ssk)
> >  		return;
> >  
> >  	/* mptcp socket is busy, schedule worker */
> > -	if (schedule_work(&msk->work))
> > -		sock_hold((struct sock *)msk);
> > +	if (!test_and_set_bit(TCP_DELACK_TIMER_DEFERRED,
> > +			      &sk->sk_tsq_flags)) {
> > +		sock_hold(sk);
> 
> Here is the msk/sk socket lock always held by some other process?

Yes, why?

> I think the trylock in patch 6 from different subflows can collide with
> each other, so a try lock can fail even with !sk->sk_lock.owned

Not following, sorry.

> Or the process owning the msk sock lock when the previous check has
> been done can release it in the meanwhile.

Yes, why?

> In one of the above happens, can the TCP_DELACK_TIMER_DEFERRED delayed
> for a long time? Do we need to address that scenario?

Hmm:
        /* mptcp socket is busy, schedule worker */
        if (!test_and_set_bit(TCP_DELACK_TIMER_DEFERRED,
                              &sk->sk_tsq_flags)) {
                sock_hold(sk);

                /* need to try again, its possible release_cb() has already
                 * been called after the test_and_set_bit() above.
                 */
                move_skbs_to_msk(msk, ssk);
        }

I thought the last move_skbs_to_msk() would address that?
(If it fails, msk is still owned so release cb will be called 'soon').

[MPTCP] Re: [PATCH net-next 8/8] mptcp: defer work schedule until mptcp lock is released

[Paolo Abeni]

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

On Tue, 2020-02-25 at 17:18 +0100, Florian Westphal wrote:
> Paolo Abeni <pabeni(a)redhat.com> wrote:
> > On Tue, 2020-02-25 at 13:54 +0100, Florian Westphal wrote:
> > > Paolo Abeni <pabeni(a)redhat.com> wrote:
> > > > I think the trylock in patch 6 from different subflows can collide with
> > > > each other, so a try lock can fail even with !sk->sk_lock.owned
> > > 
> > > Not following, sorry.
> > 
> > I mean something alike
> > 
> > CPU0 (subflow 0)		CPU1 (subflow 1)
> > trylock				trylock
> > // enqueue the			// lock busy 
> > // data	
> > 				test_and_set_bit(TCP_DELACK_TIMER_DEFERRED)
> > 
> > but nobody will call mptcp_release_cb() soon.
> 
> This would only happen if both subflows have in-sequence data
> at the time they enter this function.
> 
> Even if it happens, then:
> 1. We enqueued new data (mptcp->ack_seq update)
> 2. We unblock/notify userspace there is new data to read
> 
> If userspaces goes sleep(3600);
> 
> then its possible that we leave 'unacked' data behind
> in case the 'losing' subflow had more data than that.

Yep, I see. This looks like a non existing issue. 

I think the posted code is fine.

Thanks for the explaination.

Paolo

[MPTCP] Re: [PATCH net-next 8/8] mptcp: defer work schedule until mptcp lock is released

[Florian Westphal]

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

Paolo Abeni <pabeni(a)redhat.com> wrote:
> On Tue, 2020-02-25 at 13:54 +0100, Florian Westphal wrote:
> > Paolo Abeni <pabeni(a)redhat.com> wrote:
> > > I think the trylock in patch 6 from different subflows can collide with
> > > each other, so a try lock can fail even with !sk->sk_lock.owned
> > 
> > Not following, sorry.
> 
> I mean something alike
> 
> CPU0 (subflow 0)		CPU1 (subflow 1)
> trylock				trylock
> // enqueue the			// lock busy 
> // data	
> 				test_and_set_bit(TCP_DELACK_TIMER_DEFERRED)
> 
> but nobody will call mptcp_release_cb() soon.

This would only happen if both subflows have in-sequence data
at the time they enter this function.

Even if it happens, then:
1. We enqueued new data (mptcp->ack_seq update)
2. We unblock/notify userspace there is new data to read

If userspaces goes sleep(3600);

then its possible that we leave 'unacked' data behind
in case the 'losing' subflow had more data than that.

If thats a concern, the fix is simple:

get rid of TCP_DELACK_TIMER_DEFERRED/release_cb and
schedule work directly.

I will do that in next series.

[MPTCP] Re: [PATCH net-next 8/8] mptcp: defer work schedule until mptcp lock is released

[Paolo Abeni]

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

On Tue, 2020-02-25 at 13:54 +0100, Florian Westphal wrote:
> Paolo Abeni <pabeni(a)redhat.com> wrote:
> > On Tue, 2020-02-25 at 13:05 +0100, Florian Westphal wrote:
> > > Don't schedule the work queue right away, instead defer this
> > > to the lock release callback.
> > > 
> > > This has the advantage that it will give recv path a chance to
> > > complete -- this might have moved all pending packets from the
> > > subflow to the mptcp receive queue, which allows to avoid another
> > > schedule_work().
> > > 
> > > Signed-off-by: Florian Westphal <fw(a)strlen.de>
> > > ---
> > >  net/mptcp/protocol.c | 20 ++++++++++++++++++--
> > >  1 file changed, 18 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
> > > index de6bd9d28116..f7c0d205b047 100644
> > > --- a/net/mptcp/protocol.c
> > > +++ b/net/mptcp/protocol.c
> > > @@ -240,8 +240,15 @@ void mptcp_data_ready(struct sock *sk, struct sock *ssk)
> > >  		return;
> > >  
> > >  	/* mptcp socket is busy, schedule worker */
> > > -	if (schedule_work(&msk->work))
> > > -		sock_hold((struct sock *)msk);
> > > +	if (!test_and_set_bit(TCP_DELACK_TIMER_DEFERRED,
> > > +			      &sk->sk_tsq_flags)) {
> > > +		sock_hold(sk);
> > 
> > Here is the msk/sk socket lock always held by some other process?
> 
> Yes, why?
> 
> > I think the trylock in patch 6 from different subflows can collide with
> > each other, so a try lock can fail even with !sk->sk_lock.owned
> 
> Not following, sorry.

I mean something alike

CPU0 (subflow 0)		CPU1 (subflow 1)
trylock				trylock
// enqueue the			// lock busy 
// data	
				test_and_set_bit(TCP_DELACK_TIMER_DEFERRED)

but nobody will call mptcp_release_cb() soon.

> > Or the process owning the msk sock lock when the previous check has
> > been done can release it in the meanwhile.
> 
> Yes, why?
> 
> > In one of the above happens, can the TCP_DELACK_TIMER_DEFERRED delayed
> > for a long time? Do we need to address that scenario?
> 
> Hmm:
>         /* mptcp socket is busy, schedule worker */
>         if (!test_and_set_bit(TCP_DELACK_TIMER_DEFERRED,
>                               &sk->sk_tsq_flags)) {
>                 sock_hold(sk);
> 
>                 /* need to try again, its possible release_cb() has already
>                  * been called after the test_and_set_bit() above.
>                  */
>                 move_skbs_to_msk(msk, ssk);
>         }
> 
> I thought the last move_skbs_to_msk() would address that?
> (If it fails, msk is still owned so release cb will be called 'soon').

Thank for pointing this out! I underlooked this part. It looks like the
2nd scenario I feared is not possible!

I'm unsure about the first one ?!? can the move_skbs_to_msk() above 
still race with another subflows ?!?

Thanks,

Paolo

[MPTCP] Re: [PATCH net-next 8/8] mptcp: defer work schedule until mptcp lock is released

[Paolo Abeni]

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

On Tue, 2020-02-25 at 13:05 +0100, Florian Westphal wrote:
> Don't schedule the work queue right away, instead defer this
> to the lock release callback.
> 
> This has the advantage that it will give recv path a chance to
> complete -- this might have moved all pending packets from the
> subflow to the mptcp receive queue, which allows to avoid another
> schedule_work().
> 
> Signed-off-by: Florian Westphal <fw(a)strlen.de>
> ---
>  net/mptcp/protocol.c | 20 ++++++++++++++++++--
>  1 file changed, 18 insertions(+), 2 deletions(-)
> 
> diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
> index de6bd9d28116..f7c0d205b047 100644
> --- a/net/mptcp/protocol.c
> +++ b/net/mptcp/protocol.c
> @@ -240,8 +240,15 @@ void mptcp_data_ready(struct sock *sk, struct sock *ssk)
>  		return;
>  
>  	/* mptcp socket is busy, schedule worker */
> -	if (schedule_work(&msk->work))
> -		sock_hold((struct sock *)msk);
> +	if (!test_and_set_bit(TCP_DELACK_TIMER_DEFERRED,
> +			      &sk->sk_tsq_flags)) {
> +		sock_hold(sk);

Here is the msk/sk socket lock always held by some other process?

I think the trylock in patch 6 from different subflows can collide with
each other, so a try lock can fail even with !sk->sk_lock.owned

Or the process owning the msk sock lock when the previous check has
been done can release it in the meanwhile.

In one of the above happens, can the TCP_DELACK_TIMER_DEFERRED delayed
for a long time? Do we need to address that scenario?

Thanks,

Paolo