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

[Paolo Abeni <pabeni@redhat.com>]

On Tue, 2022-06-28 at 16:28 -0700, Mat Martineau wrote:
> 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().

LGTM! If you agree, I could rebase this series on top of "mptcp: Avoid
acquiring PM lock for subflow priority changes".


Thank!

Paolo