Re: [PATCH RFC 2/4] mptcp: introduce and use mptcp_pm_send_ack()

[Mat Martineau <mathew.j.martineau@linux.intel.com>]

On Fri, 24 Jun 2022, Paolo Abeni wrote:

> The in-kernel PM has a bit of duplicate code related to ack
> generation. Create a new helper factoring out the PM-specific
> needs and use it in a couple of places.
>
> Note that this additionally moves a few unsafe subflow socket
> access under the relevant socket lock.
>
> Signed-off-by: Paolo Abeni <pabeni@redhat.com>
> ---
> net/mptcp/pm_netlink.c | 45 ++++++++++++++++++++++++------------------
> net/mptcp/protocol.c   | 11 ++++++++---
> net/mptcp/protocol.h   |  2 +-
> 3 files changed, 35 insertions(+), 23 deletions(-)
>
> diff --git a/net/mptcp/pm_netlink.c b/net/mptcp/pm_netlink.c
> index 2b2bb3599781..91f6ed2a076a 100644
> --- a/net/mptcp/pm_netlink.c
> +++ b/net/mptcp/pm_netlink.c
> @@ -463,6 +463,29 @@ static unsigned int fill_remote_addresses_vec(struct mptcp_sock *msk, bool fullm
> 	return i;
> }
>
> +static void mptcp_pm_send_ack(struct mptcp_sock *msk, struct mptcp_subflow_context *subflow,
> +			      bool prio, bool backup)
> +{
> +	struct sock *ssk = mptcp_subflow_tcp_sock(subflow);
> +	bool slow;
> +
> +	spin_unlock_bh(&msk->pm.lock);
> +	pr_debug("send ack for %s",
> +		 prio ? "mp_prio" : (mptcp_pm_should_add_signal(msk) ? "add_addr" : "rm_addr"));
> +
> +	slow = lock_sock_fast(ssk);
> +	if (prio) {
> +		subflow->send_mp_prio = 1;
> +		subflow->backup = backup;
> +		subflow->request_bkup = backup;
> +	}
> +
> +	__mptcp_subflow_send_ack(ssk);
> +	unlock_sock_fast(ssk, slow);
> +
> +	spin_lock_bh(&msk->pm.lock);
> +}

I was short on time yesterday so I delayed reviewing this RFC series... 
Instead I spent time working on the "Locking fixes for subflow flag 
changes" series which has very, very similar locking changes. MPTCP minds 
think alike?

I guess the lesson is that I should always look at Paolo's pending patches 
before trying to solve a "new" problem :)

In that other series, I remove the pm locking when sending this ack for 
MP_PRIO - but I think this refactoring could still be helpful. The pm lock 
could be released and reacquired by the caller instead of inside 
mptcp_pm_send_ack().

- Mat

> +
> static struct mptcp_pm_addr_entry *
> __lookup_addr_by_id(struct pm_nl_pernet *pernet, unsigned int id)
> {
> @@ -705,16 +728,8 @@ void mptcp_pm_nl_addr_send_ack(struct mptcp_sock *msk)
> 		return;
>
> 	subflow = list_first_entry_or_null(&msk->conn_list, typeof(*subflow), node);
> -	if (subflow) {
> -		struct sock *ssk = mptcp_subflow_tcp_sock(subflow);
> -
> -		spin_unlock_bh(&msk->pm.lock);
> -		pr_debug("send ack for %s",
> -			 mptcp_pm_should_add_signal(msk) ? "add_addr" : "rm_addr");
> -
> -		mptcp_subflow_send_ack(ssk);
> -		spin_lock_bh(&msk->pm.lock);
> -	}
> +	if (subflow)
> +		mptcp_pm_send_ack(msk, subflow, false, false);
> }
>
> static int mptcp_pm_nl_mp_prio_send_ack(struct mptcp_sock *msk,
> @@ -736,16 +751,8 @@ static int mptcp_pm_nl_mp_prio_send_ack(struct mptcp_sock *msk,
>
> 		if (subflow->backup != bkup)
> 			msk->last_snd = NULL;
> -		subflow->backup = bkup;
> -		subflow->send_mp_prio = 1;
> -		subflow->request_bkup = bkup;
> 		__MPTCP_INC_STATS(sock_net(sk), MPTCP_MIB_MPPRIOTX);
> -
> -		spin_unlock_bh(&msk->pm.lock);
> -		pr_debug("send ack for mp_prio");
> -		mptcp_subflow_send_ack(ssk);
> -		spin_lock_bh(&msk->pm.lock);
> -
> +		mptcp_pm_send_ack(msk, subflow, true, bkup);
> 		return 0;
> 	}
>
> diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
> index b1fae2f747c9..874344f7e0fa 100644
> --- a/net/mptcp/protocol.c
> +++ b/net/mptcp/protocol.c
> @@ -502,13 +502,18 @@ static inline bool tcp_can_send_ack(const struct sock *ssk)
> 	       (TCPF_SYN_SENT | TCPF_SYN_RECV | TCPF_TIME_WAIT | TCPF_CLOSE | TCPF_LISTEN));
> }
>
> -void mptcp_subflow_send_ack(struct sock *ssk)
> +void __mptcp_subflow_send_ack(struct sock *ssk)
> +{
> +	if (tcp_can_send_ack(ssk))
> +		tcp_send_ack(ssk);
> +}
> +
> +static void mptcp_subflow_send_ack(struct sock *ssk)
> {
> 	bool slow;
>
> 	slow = lock_sock_fast(ssk);
> -	if (tcp_can_send_ack(ssk))
> -		tcp_send_ack(ssk);
> +	__mptcp_subflow_send_ack(ssk);
> 	unlock_sock_fast(ssk, slow);
> }
>
> diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h
> index 95c9ace1437b..a92b6276a03c 100644
> --- a/net/mptcp/protocol.h
> +++ b/net/mptcp/protocol.h
> @@ -609,7 +609,7 @@ void __init mptcp_subflow_init(void);
> void mptcp_subflow_shutdown(struct sock *sk, struct sock *ssk, int how);
> void mptcp_close_ssk(struct sock *sk, struct sock *ssk,
> 		     struct mptcp_subflow_context *subflow);
> -void mptcp_subflow_send_ack(struct sock *ssk);
> +void __mptcp_subflow_send_ack(struct sock *ssk);
> void mptcp_subflow_reset(struct sock *ssk);
> void mptcp_subflow_queue_clean(struct sock *ssk);
> void mptcp_sock_graft(struct sock *sk, struct socket *parent);
> -- 
> 2.35.3
>
>
>

--
Mat Martineau
Intel