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

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

On Wed, 29 Jun 2022, Paolo Abeni wrote:

> 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".
>

Yes, would be good to rebase on that series. Thanks!

--
Mat Martineau
Intel