[MPTCP] Re: [PATCH net-next 6/8] mptcp: avoid work queue scheduling if possible

[MPTCP] Re: [PATCH net-next 6/8] mptcp: avoid work queue scheduling if possible

[Florian Westphal]

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

Florian Westphal <fw(a)strlen.de> wrote:
> +void mptcp_data_ready(struct sock *sk, struct sock *ssk)
>  {
>  	struct mptcp_sock *msk = mptcp_sk(sk);
>  
>  	set_bit(MPTCP_DATA_READY, &msk->flags);
>  	sk->sk_data_ready(sk);
>  
> +	if (atomic_read(&sk->sk_rmem_alloc) < READ_ONCE(sk->sk_rcvbuf) &&
> +	    move_skbs_to_msk(msk, ssk))
> +		return;
> +

It just occured to me that it might be better to move the
sk->sk_data_ready(sk); call to the end of mptcp_data_ready(),
I would do this for next iteration.

[MPTCP] Re: [PATCH net-next 6/8] mptcp: avoid work queue scheduling if possible

[Florian Westphal]

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

Paolo Abeni <pabeni(a)redhat.com> wrote:
> > +/* In most cases we will be able to lock the mptcp socket.  If its already
> > + * owned, we need to defer to the work queue to avoid ABBA deadlock.
> > + */
> > +static bool move_skbs_to_msk(struct mptcp_sock *msk, struct sock *ssk)
> > +{
> > +	struct sock *sk = (struct sock *)msk;
> > +	unsigned int moved = 0;
> > +
> > +	if (sock_owned_by_user_nocheck(sk))
> > +		return false;
> > +
> > +	if (unlikely(!spin_trylock_bh(&sk->sk_lock.slock)))
> > +		return false;
> > +
> > +	/* must re-check after taking the lock */
> > +	if (!sock_owned_by_user_nocheck(sk))
> 
> Don't we additionally need a READ_ONCE() here and above ?
> Otherwise the compiler and/or CPU could merge the read?

Oh, right, at least first check needs READ_ONCE annotation, thanks.

> AFAICS sock_owned_by_user_nocheck() does not include the additional
> annotation.
> 
> I think we could simply drop the first check (and always do the
> trylock)

The trylock would succeed however when socket is owned, leading to
a uneeded lock operation.

[MPTCP] Re: [PATCH net-next 6/8] mptcp: avoid work queue scheduling if possible

[Paolo Abeni]

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

On Tue, 2020-02-25 at 13:05 +0100, Florian Westphal wrote:
> We can't grab the mptcp socket lock from the subflow data_ready
> callback, it would result in ABBA deadlock.
> 
> But we can try to grab the mptcp lock.
> If we can't, we can defer to work queue.
> If the lock succeeds however we can avoid the schedule_work()
> call and move all in-sequence skbs to the mptcp receive queue.
> 
> Signed-off-by: Florian Westphal <fw(a)strlen.de>
> ---
>  net/mptcp/protocol.c | 30 +++++++++++++++++++++++++++++-
>  net/mptcp/protocol.h |  2 +-
>  net/mptcp/subflow.c  |  4 ++--
>  3 files changed, 32 insertions(+), 4 deletions(-)
> 
> diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
> index c96489c5c875..c28c5cc49d65 100644
> --- a/net/mptcp/protocol.c
> +++ b/net/mptcp/protocol.c
> @@ -201,17 +201,45 @@ static bool __mptcp_move_skbs_from_subflow(struct mptcp_sock *msk,
>  	return done;
>  }
>  
> -void mptcp_data_ready(struct sock *sk)
> +/* In most cases we will be able to lock the mptcp socket.  If its already
> + * owned, we need to defer to the work queue to avoid ABBA deadlock.
> + */
> +static bool move_skbs_to_msk(struct mptcp_sock *msk, struct sock *ssk)
> +{
> +	struct sock *sk = (struct sock *)msk;
> +	unsigned int moved = 0;
> +
> +	if (sock_owned_by_user_nocheck(sk))
> +		return false;
> +
> +	if (unlikely(!spin_trylock_bh(&sk->sk_lock.slock)))
> +		return false;
> +
> +	/* must re-check after taking the lock */
> +	if (!sock_owned_by_user_nocheck(sk))

Don't we additionally need a READ_ONCE() here and above ?
Otherwise the compiler and/or CPU could merge the read?

AFAICS sock_owned_by_user_nocheck() does not include the additional
annotation.

I think we could simply drop the first check (and always do the
trylock)

Cheers,

Paolo