From: Mat Martineau <mathew.j.martineau@linux.intel.com>
To: Paolo Abeni <pabeni@redhat.com>
Cc: mptcp@lists.linux.dev
Subject: Re: [PATCH RFC 2/4] mptcp: introduce and use mptcp_pm_send_ack()
Date: Tue, 28 Jun 2022 16:28:34 -0700 (PDT) [thread overview]
Message-ID: <12f7b059-d6c8-cd75-1e1-33abe8b2b8fd@linux.intel.com> (raw)
In-Reply-To: <a59c3eb1e47c5c32f97c87d66c52f6c0021846bc.1656088406.git.pabeni@redhat.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
next prev parent reply other threads:[~2022-06-28 23:28 UTC|newest]
Thread overview: 13+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-06-24 16:38 [PATCH RFC 0/4] mptcp: let the in kernel PM control the MPC Paolo Abeni
2022-06-24 16:38 ` [PATCH RFC 1/4] mptcp: fix local endpoint acconting Paolo Abeni
2022-06-24 16:38 ` [PATCH RFC 2/4] mptcp: introduce and use mptcp_pm_send_ack() Paolo Abeni
2022-06-28 23:28 ` Mat Martineau [this message]
2022-06-29 10:10 ` Paolo Abeni
2022-06-29 15:59 ` Mat Martineau
2022-06-24 16:38 ` [PATCH RFC 3/4] mptcp: allow the in kernel PM to set MPC subflow priority Paolo Abeni
2022-06-29 0:11 ` Mat Martineau
2022-06-29 10:55 ` Paolo Abeni
2022-06-29 16:07 ` Mat Martineau
2022-06-24 16:38 ` [PATCH RFC 4/4] mptcp: more accurate MPC endpoint tracking Paolo Abeni
2022-06-24 16:49 ` mptcp: more accurate MPC endpoint tracking: Build Failure MPTCP CI
2022-06-24 18:36 ` mptcp: more accurate MPC endpoint tracking: Tests Results MPTCP CI
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=12f7b059-d6c8-cd75-1e1-33abe8b2b8fd@linux.intel.com \
--to=mathew.j.martineau@linux.intel.com \
--cc=mptcp@lists.linux.dev \
--cc=pabeni@redhat.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).