1. Patchew
  2. MPTCP
  3. [PATCH mptcp-next v11 00/12] update userspace pm mptcp_info fields
  4. View patch

[PATCH mptcp-next v11 06/12] mptcp: update userspace pm infos

Geliang Tang posted 12 patches 1 year, 5 months ago
Maintainers: Matthieu Baerts <matthieu.baerts@tessares.net>, Mat Martineau <martineau@kernel.org>, "David S. Miller" <davem@davemloft.net>, Eric Dumazet <edumazet@google.com>, Jakub Kicinski <kuba@kernel.org>, Paolo Abeni <pabeni@redhat.com>, Shuah Khan <shuah@kernel.org>, Kishen Maloor <kishen.maloor@intel.com>, Geliang Tang <geliang.tang@suse.com>, Florian Westphal <fw@strlen.de>
There is a newer version of this series
[PATCH mptcp-next v11 06/12] mptcp: update userspace pm infos
Posted by Geliang Tang 1 year, 5 months ago 
Increase pm subflows counter on both server side and client side when
userspace pm creates a new subflow, and decrease the counter when it
closes a subflow.

Increase add_addr_signaled counter in mptcp_nl_cmd_announce() when the
address is announced by userspace PM.

This modification is similar to how the in-kernel PM is updating the
counter: when additional subflows are created/removed.

Fixes: 9ab4807c84a4 ("mptcp: netlink: Add MPTCP_PM_CMD_ANNOUNCE")
Fixes: 702c2f646d42 ("mptcp: netlink: allow userspace-driven subflow establishment")
Closes: https://github.com/multipath-tcp/mptcp_net-next/issues/329
Signed-off-by: Geliang Tang <geliang.tang@suse.com>
---
 net/mptcp/pm.c           | 21 +++++++++++++++++----
 net/mptcp/pm_userspace.c |  5 +++++
 2 files changed, 22 insertions(+), 4 deletions(-)

diff --git a/net/mptcp/pm.c b/net/mptcp/pm.c
index 7539137719ef..9ba671902e69 100644
--- a/net/mptcp/pm.c
+++ b/net/mptcp/pm.c
@@ -89,8 +89,15 @@ bool mptcp_pm_allow_new_subflow(struct mptcp_sock *msk)
 	unsigned int subflows_max;
 	int ret = 0;
 
-	if (mptcp_pm_is_userspace(msk))
-		return mptcp_userspace_pm_active(msk);
+	if (mptcp_pm_is_userspace(msk)) {
+		if (mptcp_userspace_pm_active(msk)) {
+			spin_lock_bh(&pm->lock);
+			pm->subflows++;
+			spin_unlock_bh(&pm->lock);
+			return true;
+		}
+		return false;
+	}
 
 	subflows_max = mptcp_pm_get_subflows_max(msk);
 
@@ -183,8 +190,14 @@ void mptcp_pm_subflow_check_next(struct mptcp_sock *msk, const struct sock *ssk,
 	struct mptcp_pm_data *pm = &msk->pm;
 	bool update_subflows;
 
-	update_subflows = (subflow->request_join || subflow->mp_join) &&
-			  mptcp_pm_is_kernel(msk);
+	if (mptcp_pm_is_userspace(msk)) {
+		spin_lock_bh(&pm->lock);
+		pm->subflows--;
+		spin_unlock_bh(&pm->lock);
+		return;
+	}
+
+	update_subflows = (subflow->request_join || subflow->mp_join);
 	if (!READ_ONCE(pm->work_pending) && !update_subflows)
 		return;
 
diff --git a/net/mptcp/pm_userspace.c b/net/mptcp/pm_userspace.c
index c50e1507ae35..98a5c81083be 100644
--- a/net/mptcp/pm_userspace.c
+++ b/net/mptcp/pm_userspace.c
@@ -189,6 +189,7 @@ int mptcp_nl_cmd_announce(struct sk_buff *skb, struct genl_info *info)
 	spin_lock_bh(&msk->pm.lock);
 
 	if (mptcp_pm_alloc_anno_list(msk, &addr_val)) {
+		msk->pm.add_addr_signaled++;
 		mptcp_pm_announce_addr(msk, &addr_val.addr, false);
 		mptcp_pm_nl_addr_send_ack(msk);
 	}
@@ -334,6 +335,7 @@ int mptcp_nl_cmd_sf_create(struct sk_buff *skb, struct genl_info *info)
 		spin_unlock_bh(&msk->pm.lock);
 		goto create_err;
 	}
+	msk->pm.local_addr_used++;
 	spin_unlock_bh(&msk->pm.lock);
 
 	lock_sock(sk);
@@ -344,8 +346,11 @@ int mptcp_nl_cmd_sf_create(struct sk_buff *skb, struct genl_info *info)
 
 	spin_lock_bh(&msk->pm.lock);
 	if (err) {
+		msk->pm.local_addr_used--;
 		mptcp_pm_remove_anno_list_by_saddr(msk, &addr_l);
 		mptcp_userspace_pm_delete_local_addr(msk, &local);
+	} else {
+		msk->pm.subflows++;
 	}
 	spin_unlock_bh(&msk->pm.lock);
 
-- 
2.35.3
Re: [PATCH mptcp-next v11 06/12] mptcp: update userspace pm infos
Posted by Matthieu Baerts 1 year, 5 months ago 
Hi Geliang,

On 04/05/2023 12:20, Geliang Tang wrote:
> Increase pm subflows counter on both server side and client side when
> userspace pm creates a new subflow, and decrease the counter when it
> closes a subflow.
> 
> Increase add_addr_signaled counter in mptcp_nl_cmd_announce() when the
> address is announced by userspace PM.
> 
> This modification is similar to how the in-kernel PM is updating the
> counter: when additional subflows are created/removed.
> 
> Fixes: 9ab4807c84a4 ("mptcp: netlink: Add MPTCP_PM_CMD_ANNOUNCE")
> Fixes: 702c2f646d42 ("mptcp: netlink: allow userspace-driven subflow establishment")
> Closes: https://github.com/multipath-tcp/mptcp_net-next/issues/329
> Signed-off-by: Geliang Tang <geliang.tang@suse.com>
> ---
>  net/mptcp/pm.c           | 21 +++++++++++++++++----
>  net/mptcp/pm_userspace.c |  5 +++++
>  2 files changed, 22 insertions(+), 4 deletions(-)
> 
> diff --git a/net/mptcp/pm.c b/net/mptcp/pm.c
> index 7539137719ef..9ba671902e69 100644
> --- a/net/mptcp/pm.c
> +++ b/net/mptcp/pm.c
> @@ -89,8 +89,15 @@ bool mptcp_pm_allow_new_subflow(struct mptcp_sock *msk)
>  	unsigned int subflows_max;
>  	int ret = 0;
>  
> -	if (mptcp_pm_is_userspace(msk))
> -		return mptcp_userspace_pm_active(msk);
> +	if (mptcp_pm_is_userspace(msk)) {
> +		if (mptcp_userspace_pm_active(msk)) {
> +			spin_lock_bh(&pm->lock);
> +			pm->subflows++;
> +			spin_unlock_bh(&pm->lock);
> +			return true;
> +		}
> +		return false;
> +	}
>  
>  	subflows_max = mptcp_pm_get_subflows_max(msk);
>  
> @@ -183,8 +190,14 @@ void mptcp_pm_subflow_check_next(struct mptcp_sock *msk, const struct sock *ssk,
>  	struct mptcp_pm_data *pm = &msk->pm;
>  	bool update_subflows;
>  
> -	update_subflows = (subflow->request_join || subflow->mp_join) &&
> -			  mptcp_pm_is_kernel(msk);
> +	if (mptcp_pm_is_userspace(msk)) {
> +		spin_lock_bh(&pm->lock);
> +		pm->subflows--;
> +		spin_unlock_bh(&pm->lock);
> +		return;
> +	}
> +
> +	update_subflows = (subflow->request_join || subflow->mp_join);

See my question from v7: Should you not decrement the counter only if
one of these conditions is met to keep the same behaviour as what is
done with the in-kernel PM: 'subflows' counter only look at the
additional subflows?

e.g.

  update_subflows = (subflow->request_join || subflow->mp_join);

  if (mptcp_pm_is_userspace(msk)) {
      if (update_subflows) {
          (...) → pm->subflows--;
      }
      return;
  }

  if (!READ_ONCE(pm->work_pending) && !update_subflows)
      return;

I'm not a big fan to count only the additional subflows but I think it
is important to keep the same behaviour with both PMs.

>  	if (!READ_ONCE(pm->work_pending) && !update_subflows)
>  		return;
>  
> diff --git a/net/mptcp/pm_userspace.c b/net/mptcp/pm_userspace.c
> index c50e1507ae35..98a5c81083be 100644
> --- a/net/mptcp/pm_userspace.c
> +++ b/net/mptcp/pm_userspace.c
> @@ -189,6 +189,7 @@ int mptcp_nl_cmd_announce(struct sk_buff *skb, struct genl_info *info)
>  	spin_lock_bh(&msk->pm.lock);
>  
>  	if (mptcp_pm_alloc_anno_list(msk, &addr_val)) {
> +		msk->pm.add_addr_signaled++;
>  		mptcp_pm_announce_addr(msk, &addr_val.addr, false);
>  		mptcp_pm_nl_addr_send_ack(msk);
>  	}
> @@ -334,6 +335,7 @@ int mptcp_nl_cmd_sf_create(struct sk_buff *skb, struct genl_info *info)
>  		spin_unlock_bh(&msk->pm.lock);
>  		goto create_err;
>  	}
> +	msk->pm.local_addr_used++;

Maybe better to increment this counter in
mptcp_userspace_pm_append_new_local_addr() so we do it once if the local
doesn't already exist (when the TODO will be done).

>  	spin_unlock_bh(&msk->pm.lock);
>  
>  	lock_sock(sk);
> @@ -344,8 +346,11 @@ int mptcp_nl_cmd_sf_create(struct sk_buff *skb, struct genl_info *info)
>  
>  	spin_lock_bh(&msk->pm.lock);
>  	if (err) {
> +		msk->pm.local_addr_used--;

Similar here, maybe better to do that in
mptcp_userspace_pm_delete_local_addr()?

Also by doing that, the counter will also be decremented when the
userspace asks to destroy a subflow (and the refcount is 0).

>  		mptcp_pm_remove_anno_list_by_saddr(msk, &addr_l);
>  		mptcp_userspace_pm_delete_local_addr(msk, &local);

Cheers,
Matt
-- 
Tessares | Belgium | Hybrid Access Solutions
www.tessares.net
Re: [PATCH mptcp-next v11 06/12] mptcp: update userspace pm infos
Posted by Geliang Tang 1 year, 5 months ago 
On Thu, May 04, 2023 at 06:20:11PM +0800, Geliang Tang wrote:
> Increase pm subflows counter on both server side and client side when
> userspace pm creates a new subflow, and decrease the counter when it
> closes a subflow.
> 
> Increase add_addr_signaled counter in mptcp_nl_cmd_announce() when the
> address is announced by userspace PM.
> 
> This modification is similar to how the in-kernel PM is updating the
> counter: when additional subflows are created/removed.
> 
> Fixes: 9ab4807c84a4 ("mptcp: netlink: Add MPTCP_PM_CMD_ANNOUNCE")
> Fixes: 702c2f646d42 ("mptcp: netlink: allow userspace-driven subflow establishment")
> Closes: https://github.com/multipath-tcp/mptcp_net-next/issues/329

And CI reported a warning here, should use 'Link:' instead of 'Closes'.

> Signed-off-by: Geliang Tang <geliang.tang@suse.com>
> ---
>  net/mptcp/pm.c           | 21 +++++++++++++++++----
>  net/mptcp/pm_userspace.c |  5 +++++
>  2 files changed, 22 insertions(+), 4 deletions(-)
> 
> diff --git a/net/mptcp/pm.c b/net/mptcp/pm.c
> index 7539137719ef..9ba671902e69 100644
> --- a/net/mptcp/pm.c
> +++ b/net/mptcp/pm.c
> @@ -89,8 +89,15 @@ bool mptcp_pm_allow_new_subflow(struct mptcp_sock *msk)
>  	unsigned int subflows_max;
>  	int ret = 0;
>  
> -	if (mptcp_pm_is_userspace(msk))
> -		return mptcp_userspace_pm_active(msk);
> +	if (mptcp_pm_is_userspace(msk)) {
> +		if (mptcp_userspace_pm_active(msk)) {
> +			spin_lock_bh(&pm->lock);
> +			pm->subflows++;
> +			spin_unlock_bh(&pm->lock);
> +			return true;
> +		}
> +		return false;
> +	}
>  
>  	subflows_max = mptcp_pm_get_subflows_max(msk);
>  
> @@ -183,8 +190,14 @@ void mptcp_pm_subflow_check_next(struct mptcp_sock *msk, const struct sock *ssk,
>  	struct mptcp_pm_data *pm = &msk->pm;
>  	bool update_subflows;
>  
> -	update_subflows = (subflow->request_join || subflow->mp_join) &&
> -			  mptcp_pm_is_kernel(msk);
> +	if (mptcp_pm_is_userspace(msk)) {
> +		spin_lock_bh(&pm->lock);
> +		pm->subflows--;
> +		spin_unlock_bh(&pm->lock);
> +		return;
> +	}
> +
> +	update_subflows = (subflow->request_join || subflow->mp_join);
>  	if (!READ_ONCE(pm->work_pending) && !update_subflows)
>  		return;
>  
> diff --git a/net/mptcp/pm_userspace.c b/net/mptcp/pm_userspace.c
> index c50e1507ae35..98a5c81083be 100644
> --- a/net/mptcp/pm_userspace.c
> +++ b/net/mptcp/pm_userspace.c
> @@ -189,6 +189,7 @@ int mptcp_nl_cmd_announce(struct sk_buff *skb, struct genl_info *info)
>  	spin_lock_bh(&msk->pm.lock);
>  
>  	if (mptcp_pm_alloc_anno_list(msk, &addr_val)) {
> +		msk->pm.add_addr_signaled++;
>  		mptcp_pm_announce_addr(msk, &addr_val.addr, false);
>  		mptcp_pm_nl_addr_send_ack(msk);
>  	}
> @@ -334,6 +335,7 @@ int mptcp_nl_cmd_sf_create(struct sk_buff *skb, struct genl_info *info)
>  		spin_unlock_bh(&msk->pm.lock);
>  		goto create_err;
>  	}
> +	msk->pm.local_addr_used++;
>  	spin_unlock_bh(&msk->pm.lock);
>  
>  	lock_sock(sk);
> @@ -344,8 +346,11 @@ int mptcp_nl_cmd_sf_create(struct sk_buff *skb, struct genl_info *info)
>  
>  	spin_lock_bh(&msk->pm.lock);
>  	if (err) {
> +		msk->pm.local_addr_used--;
>  		mptcp_pm_remove_anno_list_by_saddr(msk, &addr_l);
>  		mptcp_userspace_pm_delete_local_addr(msk, &local);
> +	} else {
> +		msk->pm.subflows++;
>  	}
>  	spin_unlock_bh(&msk->pm.lock);
>  
> -- 
> 2.35.3
>
Re: [PATCH mptcp-next v11 06/12] mptcp: update userspace pm infos
Posted by Matthieu Baerts 1 year, 5 months ago 
Hi Geliang,

On 04/05/2023 12:59, Geliang Tang wrote:
> On Thu, May 04, 2023 at 06:20:11PM +0800, Geliang Tang wrote:
>> Increase pm subflows counter on both server side and client side when
>> userspace pm creates a new subflow, and decrease the counter when it
>> closes a subflow.
>>
>> Increase add_addr_signaled counter in mptcp_nl_cmd_announce() when the
>> address is announced by userspace PM.
>>
>> This modification is similar to how the in-kernel PM is updating the
>> counter: when additional subflows are created/removed.
>>
>> Fixes: 9ab4807c84a4 ("mptcp: netlink: Add MPTCP_PM_CMD_ANNOUNCE")
>> Fixes: 702c2f646d42 ("mptcp: netlink: allow userspace-driven subflow establishment")
>> Closes: https://github.com/multipath-tcp/mptcp_net-next/issues/329
> 
> And CI reported a warning here, should use 'Link:' instead of 'Closes'.

You can ignore this warning, there is a "fix" for that that recently
landed in Linus tree, we just need to wait for the net tree to sync with
it to get it ;-)

Cheers,
Matt
-- 
Tessares | Belgium | Hybrid Access Solutions
www.tessares.net

Patchew 2.1-devel-b67e4c2

© 2016 - 2024 Red Hat, Inc.