diff mbox series

[mptcp-next,v4,5/9] mptcp: pm: add add_addr_received() interface

Message ID 0b18abccee51f68bb1e0016426159df0b2ca1e36.1742804266.git.tanggeliang@kylinos.cn (mailing list archive)
State Superseded
Delegated to: Matthieu Baerts
Headers show
Series BPF path manager, part 6 | expand

Checks

Context Check Description
matttbe/checkpatch success total: 0 errors, 0 warnings, 0 checks, 115 lines checked
matttbe/shellcheck success MPTCP selftests files have not been modified
matttbe/build success Build and static analysis OK
matttbe/KVM_Validation__normal success Success! ✅
matttbe/KVM_Validation__debug success Success! ✅
matttbe/KVM_Validation__btf-normal__only_bpftest_all_ success Success! ✅
matttbe/KVM_Validation__btf-debug__only_bpftest_all_ success Success! ✅

Commit Message

Geliang Tang March 24, 2025, 8:19 a.m. UTC 
From: Geliang Tang <tanggeliang@kylinos.cn>

This patch adds an optional .add_addr_received interface for struct
mptcp_pm_ops and invokes it in mptcp_pm_worker().

This interface is only implemented in the in-kernel PM as a wrapper
of mptcp_pm_nl_add_addr_received().

Signed-off-by: Geliang Tang <tanggeliang@kylinos.cn>
---
 include/net/mptcp.h   |  1 +
 net/mptcp/pm.c        | 18 +++++++++++-------
 net/mptcp/pm_kernel.c | 24 +++++++++++-------------
 net/mptcp/protocol.h  |  1 -
 4 files changed, 23 insertions(+), 21 deletions(-)

Comments

Matthieu Baerts March 24, 2025, 11:02 a.m. UTC | #1 
Hi Geliang,

On 24/03/2025 09:19, Geliang Tang wrote:
> From: Geliang Tang <tanggeliang@kylinos.cn>
> 
> This patch adds an optional .add_addr_received interface for struct
> mptcp_pm_ops and invokes it in mptcp_pm_worker().
> 
> This interface is only implemented in the in-kernel PM as a wrapper
> of mptcp_pm_nl_add_addr_received().
> 
> Signed-off-by: Geliang Tang <tanggeliang@kylinos.cn>
> ---
>  include/net/mptcp.h   |  1 +
>  net/mptcp/pm.c        | 18 +++++++++++-------
>  net/mptcp/pm_kernel.c | 24 +++++++++++-------------
>  net/mptcp/protocol.h  |  1 -
>  4 files changed, 23 insertions(+), 21 deletions(-)
> 
> diff --git a/include/net/mptcp.h b/include/net/mptcp.h
> index 4ac936e4ce0d..5118d11d2ee9 100644
> --- a/include/net/mptcp.h
> +++ b/include/net/mptcp.h
> @@ -127,6 +127,7 @@ struct mptcp_pm_ops {
>  	/* optional */
>  	void (*established)(struct mptcp_sock *msk);
>  	void (*subflow_established)(struct mptcp_sock *msk);
> +	void (*add_addr_received)(struct mptcp_sock *msk);
>  
>  	char			name[MPTCP_PM_NAME_MAX];
>  	struct module		*owner;
> diff --git a/net/mptcp/pm.c b/net/mptcp/pm.c
> index 8efb47331f79..71589cd5dee7 100644
> --- a/net/mptcp/pm.c
> +++ b/net/mptcp/pm.c
> @@ -607,10 +607,11 @@ void mptcp_pm_add_addr_received(const struct sock *ssk,
>  		   (addr->id > 0 && !READ_ONCE(pm->accept_addr))) {

How do you plan to remove the 'if (mptcp_pm_is_userspace(msk))' here above.

I guess this code is there to force the other peer to retransmit the
ADD_ADDR, hoping a userspace will be launched in between. Either we
remove this exception for the userspace PM (other events will not be
retransmitted: RM_ADDR, subflow closed, etc.), or we have another hook
but it feels wrong.

EDIT: I just saw your patch 7/9. Maybe we should avoid adding this
add_addr_echo hook, no? It is not clear what should be done here. I need
to think about that too.

An alternative is to send the ADD_ADDR echo from the worker, if
pm->ops->add_addr_received() returned true. If
pm->ops->add_addr_received is not implemented, then the ADD_ADDR echo is
scheduled from here. WDYT?

>  		mptcp_pm_announce_addr(msk, addr, true);
>  		mptcp_pm_add_addr_send_ack(msk);
> -	} else if (mptcp_pm_schedule_work(msk, MPTCP_PM_ADD_ADDR_RECEIVED)) {
> -		pm->remote = *addr;
> -	} else {
> -		ret = -EINVAL;
> +	} else if (pm->ops->add_addr_received) {
> +		if (mptcp_pm_schedule_work(msk, MPTCP_PM_ADD_ADDR_RECEIVED))
> +			pm->remote = *addr;
> +		else
> +			ret = -EINVAL;
>  	}
>  
>  	if (ret)
> @@ -948,6 +949,12 @@ void mptcp_pm_worker(struct mptcp_sock *msk)
>  		return;
>  
>  	pr_debug("msk=%p status=%x\n", msk, pm->status);
> +	if (pm->status & BIT(MPTCP_PM_ADD_ADDR_RECEIVED)) {
> +		spin_lock_bh(&pm->lock);
> +		pm->status &= ~BIT(MPTCP_PM_ADD_ADDR_RECEIVED);
> +		spin_unlock_bh(&pm->lock);
> +		pm->ops->add_addr_received(msk);
> +	}
>  	if (pm->status & BIT(MPTCP_PM_ADD_ADDR_SEND_ACK)) {
>  		spin_lock_bh(&pm->lock);
>  		pm->status &= ~BIT(MPTCP_PM_ADD_ADDR_SEND_ACK);
> @@ -972,9 +979,6 @@ void mptcp_pm_worker(struct mptcp_sock *msk)
>  		spin_unlock_bh(&pm->lock);
>  		pm->ops->subflow_established(msk);
>  	}
> -	spin_lock_bh(&pm->lock);
> -	__mptcp_pm_kernel_worker(msk);
> -	spin_unlock_bh(&pm->lock);
>  }
>  
>  static void mptcp_pm_ops_init(struct mptcp_sock *msk,
> diff --git a/net/mptcp/pm_kernel.c b/net/mptcp/pm_kernel.c
> index 2e181224bccb..4f4791620072 100644
> --- a/net/mptcp/pm_kernel.c
> +++ b/net/mptcp/pm_kernel.c
> @@ -461,12 +461,13 @@ static void mptcp_pm_nl_add_addr_received(struct mptcp_sock *msk)
>  		 msk->pm.add_addr_accepted, add_addr_accept_max,
>  		 msk->pm.remote.family);
>  
> +	spin_lock_bh(&msk->pm.lock);
>  	remote = msk->pm.remote;
>  	mptcp_pm_announce_addr(msk, &remote, true);
>  	mptcp_pm_addr_send_ack(msk);
>  
>  	if (lookup_subflow_by_daddr(&msk->conn_list, &remote))
> -		return;
> +		goto out;
>  
>  	/* pick id 0 port, if none is provided the remote address */
>  	if (!remote.port)
> @@ -477,7 +478,7 @@ static void mptcp_pm_nl_add_addr_received(struct mptcp_sock *msk)
>  	 */
>  	nr = fill_local_addresses_vec(msk, &remote, locals);
>  	if (nr == 0)
> -		return;
> +		goto out;
>  
>  	spin_unlock_bh(&msk->pm.lock);
>  	for (i = 0; i < nr; i++)
> @@ -493,6 +494,8 @@ static void mptcp_pm_nl_add_addr_received(struct mptcp_sock *msk)
>  		    msk->pm.subflows >= subflows_max)
>  			WRITE_ONCE(msk->pm.accept_addr, false);
>  	}
> +out:
> +	spin_unlock_bh(&msk->pm.lock);
>  }
>  
>  void mptcp_pm_nl_rm_addr(struct mptcp_sock *msk, u8 rm_id)
> @@ -1342,17 +1345,6 @@ bool mptcp_pm_nl_check_work_pending(struct mptcp_sock *msk)
>  	return true;
>  }
>  
> -/* Called under PM lock */
> -void __mptcp_pm_kernel_worker(struct mptcp_sock *msk)
> -{
> -	struct mptcp_pm_data *pm = &msk->pm;
> -
> -	if (pm->status & BIT(MPTCP_PM_ADD_ADDR_RECEIVED)) {
> -		pm->status &= ~BIT(MPTCP_PM_ADD_ADDR_RECEIVED);
> -		mptcp_pm_nl_add_addr_received(msk);
> -	}
> -}
> -
>  static int __net_init pm_nl_init_net(struct net *net)
>  {
>  	struct pm_nl_pernet *pernet = pm_nl_get_pernet(net);
> @@ -1394,6 +1386,11 @@ static struct pernet_operations mptcp_pm_pernet_ops = {
>  	.size = sizeof(struct pm_nl_pernet),
>  };
>  
> +static void mptcp_pm_kernel_add_addr_received(struct mptcp_sock *msk)
> +{
> +	mptcp_pm_nl_add_addr_received(msk);

No need to add a new static function only calling another static
function with the same arguments.

Simply rename mptcp_pm_nl_add_addr_received() to
mptcp_pm_kernel_add_addr_received().

> +}
> +
>  static void mptcp_pm_kernel_init(struct mptcp_sock *msk)
>  {
>  	bool subflows_allowed = !!mptcp_pm_get_subflows_max(msk);
> @@ -1419,6 +1416,7 @@ struct mptcp_pm_ops mptcp_pm_kernel = {
>  	.get_priority		= mptcp_pm_kernel_get_priority,
>  	.established		= mptcp_pm_kernel_established,
>  	.subflow_established	= mptcp_pm_kernel_subflow_established,
> +	.add_addr_received	= mptcp_pm_kernel_add_addr_received,
>  	.init			= mptcp_pm_kernel_init,
>  	.name			= "kernel",
>  	.owner			= THIS_MODULE,
> diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h
> index 64aa091cb685..7fa26c49fbed 100644
> --- a/net/mptcp/protocol.h
> +++ b/net/mptcp/protocol.h
> @@ -1164,7 +1164,6 @@ void __init mptcp_pm_kernel_register(void);
>  void __init mptcp_pm_userspace_register(void);
>  void __init mptcp_pm_nl_init(void);
>  void mptcp_pm_worker(struct mptcp_sock *msk);
> -void __mptcp_pm_kernel_worker(struct mptcp_sock *msk);
>  unsigned int mptcp_pm_get_add_addr_signal_max(const struct mptcp_sock *msk);
>  unsigned int mptcp_pm_get_add_addr_accept_max(const struct mptcp_sock *msk);
>  unsigned int mptcp_pm_get_subflows_max(const struct mptcp_sock *msk);

Cheers,
Matt
diff mbox series

Patch

diff --git a/include/net/mptcp.h b/include/net/mptcp.h
index 4ac936e4ce0d..5118d11d2ee9 100644
--- a/include/net/mptcp.h
+++ b/include/net/mptcp.h
@@ -127,6 +127,7 @@  struct mptcp_pm_ops {
 	/* optional */
 	void (*established)(struct mptcp_sock *msk);
 	void (*subflow_established)(struct mptcp_sock *msk);
+	void (*add_addr_received)(struct mptcp_sock *msk);
 
 	char			name[MPTCP_PM_NAME_MAX];
 	struct module		*owner;
diff --git a/net/mptcp/pm.c b/net/mptcp/pm.c
index 8efb47331f79..71589cd5dee7 100644
--- a/net/mptcp/pm.c
+++ b/net/mptcp/pm.c
@@ -607,10 +607,11 @@  void mptcp_pm_add_addr_received(const struct sock *ssk,
 		   (addr->id > 0 && !READ_ONCE(pm->accept_addr))) {
 		mptcp_pm_announce_addr(msk, addr, true);
 		mptcp_pm_add_addr_send_ack(msk);
-	} else if (mptcp_pm_schedule_work(msk, MPTCP_PM_ADD_ADDR_RECEIVED)) {
-		pm->remote = *addr;
-	} else {
-		ret = -EINVAL;
+	} else if (pm->ops->add_addr_received) {
+		if (mptcp_pm_schedule_work(msk, MPTCP_PM_ADD_ADDR_RECEIVED))
+			pm->remote = *addr;
+		else
+			ret = -EINVAL;
 	}
 
 	if (ret)
@@ -948,6 +949,12 @@  void mptcp_pm_worker(struct mptcp_sock *msk)
 		return;
 
 	pr_debug("msk=%p status=%x\n", msk, pm->status);
+	if (pm->status & BIT(MPTCP_PM_ADD_ADDR_RECEIVED)) {
+		spin_lock_bh(&pm->lock);
+		pm->status &= ~BIT(MPTCP_PM_ADD_ADDR_RECEIVED);
+		spin_unlock_bh(&pm->lock);
+		pm->ops->add_addr_received(msk);
+	}
 	if (pm->status & BIT(MPTCP_PM_ADD_ADDR_SEND_ACK)) {
 		spin_lock_bh(&pm->lock);
 		pm->status &= ~BIT(MPTCP_PM_ADD_ADDR_SEND_ACK);
@@ -972,9 +979,6 @@  void mptcp_pm_worker(struct mptcp_sock *msk)
 		spin_unlock_bh(&pm->lock);
 		pm->ops->subflow_established(msk);
 	}
-	spin_lock_bh(&pm->lock);
-	__mptcp_pm_kernel_worker(msk);
-	spin_unlock_bh(&pm->lock);
 }
 
 static void mptcp_pm_ops_init(struct mptcp_sock *msk,
diff --git a/net/mptcp/pm_kernel.c b/net/mptcp/pm_kernel.c
index 2e181224bccb..4f4791620072 100644
--- a/net/mptcp/pm_kernel.c
+++ b/net/mptcp/pm_kernel.c
@@ -461,12 +461,13 @@  static void mptcp_pm_nl_add_addr_received(struct mptcp_sock *msk)
 		 msk->pm.add_addr_accepted, add_addr_accept_max,
 		 msk->pm.remote.family);
 
+	spin_lock_bh(&msk->pm.lock);
 	remote = msk->pm.remote;
 	mptcp_pm_announce_addr(msk, &remote, true);
 	mptcp_pm_addr_send_ack(msk);
 
 	if (lookup_subflow_by_daddr(&msk->conn_list, &remote))
-		return;
+		goto out;
 
 	/* pick id 0 port, if none is provided the remote address */
 	if (!remote.port)
@@ -477,7 +478,7 @@  static void mptcp_pm_nl_add_addr_received(struct mptcp_sock *msk)
 	 */
 	nr = fill_local_addresses_vec(msk, &remote, locals);
 	if (nr == 0)
-		return;
+		goto out;
 
 	spin_unlock_bh(&msk->pm.lock);
 	for (i = 0; i < nr; i++)
@@ -493,6 +494,8 @@  static void mptcp_pm_nl_add_addr_received(struct mptcp_sock *msk)
 		    msk->pm.subflows >= subflows_max)
 			WRITE_ONCE(msk->pm.accept_addr, false);
 	}
+out:
+	spin_unlock_bh(&msk->pm.lock);
 }
 
 void mptcp_pm_nl_rm_addr(struct mptcp_sock *msk, u8 rm_id)
@@ -1342,17 +1345,6 @@  bool mptcp_pm_nl_check_work_pending(struct mptcp_sock *msk)
 	return true;
 }
 
-/* Called under PM lock */
-void __mptcp_pm_kernel_worker(struct mptcp_sock *msk)
-{
-	struct mptcp_pm_data *pm = &msk->pm;
-
-	if (pm->status & BIT(MPTCP_PM_ADD_ADDR_RECEIVED)) {
-		pm->status &= ~BIT(MPTCP_PM_ADD_ADDR_RECEIVED);
-		mptcp_pm_nl_add_addr_received(msk);
-	}
-}
-
 static int __net_init pm_nl_init_net(struct net *net)
 {
 	struct pm_nl_pernet *pernet = pm_nl_get_pernet(net);
@@ -1394,6 +1386,11 @@  static struct pernet_operations mptcp_pm_pernet_ops = {
 	.size = sizeof(struct pm_nl_pernet),
 };
 
+static void mptcp_pm_kernel_add_addr_received(struct mptcp_sock *msk)
+{
+	mptcp_pm_nl_add_addr_received(msk);
+}
+
 static void mptcp_pm_kernel_init(struct mptcp_sock *msk)
 {
 	bool subflows_allowed = !!mptcp_pm_get_subflows_max(msk);
@@ -1419,6 +1416,7 @@  struct mptcp_pm_ops mptcp_pm_kernel = {
 	.get_priority		= mptcp_pm_kernel_get_priority,
 	.established		= mptcp_pm_kernel_established,
 	.subflow_established	= mptcp_pm_kernel_subflow_established,
+	.add_addr_received	= mptcp_pm_kernel_add_addr_received,
 	.init			= mptcp_pm_kernel_init,
 	.name			= "kernel",
 	.owner			= THIS_MODULE,
diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h
index 64aa091cb685..7fa26c49fbed 100644
--- a/net/mptcp/protocol.h
+++ b/net/mptcp/protocol.h
@@ -1164,7 +1164,6 @@  void __init mptcp_pm_kernel_register(void);
 void __init mptcp_pm_userspace_register(void);
 void __init mptcp_pm_nl_init(void);
 void mptcp_pm_worker(struct mptcp_sock *msk);
-void __mptcp_pm_kernel_worker(struct mptcp_sock *msk);
 unsigned int mptcp_pm_get_add_addr_signal_max(const struct mptcp_sock *msk);
 unsigned int mptcp_pm_get_add_addr_accept_max(const struct mptcp_sock *msk);
 unsigned int mptcp_pm_get_subflows_max(const struct mptcp_sock *msk);