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

[Florian Westphal <fw at strlen.de>]

[-- 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').