Re: [PATCH net v4 1/3] net: mptcp: factor out __mptcp_close() without socket lock

[Paolo Abeni <pabeni@redhat.com>]

Hello,

overall the series LGTM, but there are a few small cosmetic changes I
I'd  like to suggest, see below and the other patches for the details.

On Fri, 2022-09-09 at 13:49 +0800, menglong8.dong@gmail.com wrote:
> From: Menglong Dong <imagedong@tencent.com>
> 
> Factor out __mptcp_close() from mptcp_close(). The caller of
> __mptcp_close() should hold the socket lock, and cancel mptcp work when
> __mptcp_close() return true.
> 
> This function will be used in the next commit.
> 
> Reviewed-by: Jiang Biao <benbjiang@tencent.com>
> Reviewed-by: Mengen Sun <mengensun@tencent.com>
> Signed-off-by: Menglong Dong <imagedong@tencent.com>
> ---
>  net/mptcp/protocol.c | 19 ++++++++++++++-----
>  net/mptcp/protocol.h |  1 +
>  2 files changed, 15 insertions(+), 5 deletions(-)
> 
> diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
> index d398f3810662..1cfb4f1ff4e4 100644
> --- a/net/mptcp/protocol.c
> +++ b/net/mptcp/protocol.c
> @@ -2796,13 +2796,12 @@ static void __mptcp_destroy_sock(struct sock *sk)
>  	sock_put(sk);
>  }
>  
> -static void mptcp_close(struct sock *sk, long timeout)
> +bool __mptcp_close(struct sock *sk, long timeout)
>  {
>  	struct mptcp_subflow_context *subflow;
>  	struct mptcp_sock *msk = mptcp_sk(sk);
>  	bool do_cancel_work = false;
>  
> -	lock_sock(sk);
>  	sk->sk_shutdown = SHUTDOWN_MASK;
>  
>  	if ((1 << sk->sk_state) & (TCPF_LISTEN | TCPF_CLOSE)) {
> @@ -2833,7 +2832,6 @@ static void mptcp_close(struct sock *sk, long timeout)
>  	}
>  	sock_orphan(sk);
>  
> -	sock_hold(sk);

If you don't remove this line you can avoid adding a sock_hold(sk) in
mptcp_close() and an additional one in patch 3. The diff will be
smaller and the patch clearer.

>  	pr_debug("msk=%p state=%d", sk, sk->sk_state);
>  	if (mptcp_sk(sk)->token)
>  		mptcp_event(MPTCP_EVENT_CLOSED, msk, NULL, GFP_KERNEL);
> @@ -2844,10 +2842,21 @@ static void mptcp_close(struct sock *sk, long timeout)
>  	} else {
>  		mptcp_reset_timeout(msk, 0);
>  	}
> +
> +	return do_cancel_work;
> +}
> +
> +static void mptcp_close(struct sock *sk, long timeout)
> +{
> +	bool cancel_work;

I think this variable name is a bit confusing, as we already have a
global function with the same name. I think 'do_cancel_work' will be
better (and again will produce a smaller diff and a clearer patch).


Thanks,

Paolo