From: Mat Martineau <mathew.j.martineau@linux.intel.com>
To: Paolo Abeni <pabeni@redhat.com>
Cc: mptcp@lists.linux.dev
Subject: Re: [PATCH RFC 3/4] mptcp: allow the in kernel PM to set MPC subflow priority
Date: Tue, 28 Jun 2022 17:11:57 -0700 (PDT) [thread overview]
Message-ID: <31cf8780-ea9f-fdf7-dffa-b77c831747@linux.intel.com> (raw)
In-Reply-To: <8a80c3bf12e43985e00b4fb04beccd38d69e505b.1656088406.git.pabeni@redhat.com>
On Fri, 24 Jun 2022, Paolo Abeni wrote:
> Any local endpoints configured on the address matching the
> MPC subflow are currently ignored.
>
> Specifically, setting a backup flag on them has no effect
> on the first subflow, as the MPC handshake can't carry such
> info.
>
> This change refactors the MPC endpoint id accounting to
> additionally fetch the priority info from the relevant endpoint
> and eventually trigger the MP_PRIO handshake as needed.
>
> As a result, the MPC subflow now switches to backup priority
> after that the MPTCP socket is fully established, according
> to the local endpoint configuration.
>
> Signed-off-by: Paolo Abeni <pabeni@redhat.com>
> ---
> net/mptcp/pm_netlink.c | 37 +++++++++++++++----------------------
> 1 file changed, 15 insertions(+), 22 deletions(-)
>
> diff --git a/net/mptcp/pm_netlink.c b/net/mptcp/pm_netlink.c
> index 91f6ed2a076a..a6eb501e5031 100644
> --- a/net/mptcp/pm_netlink.c
> +++ b/net/mptcp/pm_netlink.c
> @@ -505,30 +505,14 @@ __lookup_addr(struct pm_nl_pernet *pernet, const struct mptcp_addr_info *info,
> struct mptcp_pm_addr_entry *entry;
>
> list_for_each_entry(entry, &pernet->local_addr_list, list) {
> - if ((!lookup_by_id && mptcp_addresses_equal(&entry->addr, info, true)) ||
> + if ((!lookup_by_id &&
> + mptcp_addresses_equal(&entry->addr, info, entry->addr.port)) ||
It seems like we could have multiple entries in the local_addr_list with
the same address, but with different entries having port==0 or nonzero
ports. If mptcp_nl_cmd_set_flags() is called with a nonzero port, but this
lookup function encounters the port==0 entry first, this will match an
unexpected entry in local_addr_list.
Is there some other constraint preventing this?
> (lookup_by_id && entry->addr.id == info->id))
> return entry;
> }
> return NULL;
> }
>
> -static int
> -lookup_id_by_addr(const struct pm_nl_pernet *pernet, const struct mptcp_addr_info *addr)
> -{
> - const struct mptcp_pm_addr_entry *entry;
> - int ret = -1;
> -
> - rcu_read_lock();
> - list_for_each_entry(entry, &pernet->local_addr_list, list) {
> - if (mptcp_addresses_equal(&entry->addr, addr, entry->addr.port)) {
> - ret = entry->addr.id;
> - break;
> - }
> - }
> - rcu_read_unlock();
> - return ret;
> -}
> -
> static void mptcp_pm_create_subflow_or_signal_addr(struct mptcp_sock *msk)
> {
> struct sock *sk = (struct sock *)msk;
> @@ -546,13 +530,22 @@ static void mptcp_pm_create_subflow_or_signal_addr(struct mptcp_sock *msk)
>
> /* do lazy endpoint usage accounting for the MPC subflows */
> if (unlikely(!(msk->pm.status & BIT(MPTCP_PM_MPC_ENDPOINT_ACCOUNTED))) && msk->first) {
> + struct mptcp_subflow_context *subflow = mptcp_subflow_ctx(msk->first);
> + struct mptcp_pm_addr_entry *entry;
> struct mptcp_addr_info mpc_addr;
> - int mpc_id;
> + bool backup = false;
>
> local_address((struct sock_common *)msk->first, &mpc_addr);
> - mpc_id = lookup_id_by_addr(pernet, &mpc_addr);
> - if (mpc_id >= 0)
> - __clear_bit(mpc_id, msk->pm.id_avail_bitmap);
> + rcu_read_lock();
> + entry = __lookup_addr(pernet, &mpc_addr, false);
> + if (entry) {
> + __clear_bit(entry->addr.id, msk->pm.id_avail_bitmap);
> + backup = !!(entry->flags & MPTCP_PM_ADDR_FLAG_BACKUP);
> + }
> + rcu_read_unlock();
> +
> + if (backup)
> + mptcp_pm_send_ack(msk, subflow, true, backup);
This looks ok to me. I think I follow your example in #285 for how to
create endpoints that would allow the priority to be changed later, but I
think the selftests might make that clearer.
Thanks!
>
> msk->pm.status |= BIT(MPTCP_PM_MPC_ENDPOINT_ACCOUNTED);
> }
> --
> 2.35.3
>
>
>
--
Mat Martineau
Intel
next prev parent reply other threads:[~2022-06-29 0:11 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
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 [this message]
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=31cf8780-ea9f-fdf7-dffa-b77c831747@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).