[PATCH mptcp-next v7 05/11] mptcp: pm: in-kernel: register mptcp_kernel_pm

Geliang Tang posted 11 patches 11 months, 1 week ago
There is a newer version of this series
[PATCH mptcp-next v7 05/11] mptcp: pm: in-kernel: register mptcp_kernel_pm
Posted by Geliang Tang 11 months, 1 week ago
From: Geliang Tang <tanggeliang@kylinos.cn>

This patch defines the original in-kernel netlink path manager as a
new struct mptcp_pm_ops named "mptcp_kernel_pm", and register it in
mptcp_pm_nl_init().

This mptcp_pm_ops will be skipped in mptcp_pm_unregister().

Only get_local_id() and get_priority() interfaces are implemented here.

Signed-off-by: Geliang Tang <tanggeliang@kylinos.cn>
---
 net/mptcp/pm.c        | 3 +++
 net/mptcp/pm_kernel.c | 9 +++++++++
 net/mptcp/protocol.h  | 2 ++
 3 files changed, 14 insertions(+)

diff --git a/net/mptcp/pm.c b/net/mptcp/pm.c
index 88ff136b3786..e648cb522320 100644
--- a/net/mptcp/pm.c
+++ b/net/mptcp/pm.c
@@ -1075,6 +1075,9 @@ int mptcp_pm_register(struct mptcp_pm_ops *pm)
 
 void mptcp_pm_unregister(struct mptcp_pm_ops *pm)
 {
+	if (pm == &mptcp_kernel_pm)
+		return;
+
 	spin_lock(&mptcp_pm_list_lock);
 	list_del_rcu(&pm->list);
 	spin_unlock(&mptcp_pm_list_lock);
diff --git a/net/mptcp/pm_kernel.c b/net/mptcp/pm_kernel.c
index daf8f98a3164..8a5966e6e3e3 100644
--- a/net/mptcp/pm_kernel.c
+++ b/net/mptcp/pm_kernel.c
@@ -1400,6 +1400,13 @@ static struct pernet_operations mptcp_pm_pernet_ops = {
 	.size = sizeof(struct pm_nl_pernet),
 };
 
+struct mptcp_pm_ops mptcp_kernel_pm = {
+	.get_local_id		= mptcp_pm_nl_get_local_id,
+	.get_priority		= mptcp_pm_nl_is_backup,
+	.name			= "kernel",
+	.owner			= THIS_MODULE,
+};
+
 void __init mptcp_pm_nl_init(void)
 {
 	if (register_pernet_subsys(&mptcp_pm_pernet_ops) < 0)
@@ -1407,4 +1414,6 @@ void __init mptcp_pm_nl_init(void)
 
 	if (genl_register_family(&mptcp_genl_family))
 		panic("Failed to register MPTCP PM netlink family\n");
+
+	mptcp_pm_register(&mptcp_kernel_pm);
 }
diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h
index 9dbfde4027b3..56d3a7457f80 100644
--- a/net/mptcp/protocol.h
+++ b/net/mptcp/protocol.h
@@ -1051,6 +1051,8 @@ int mptcp_pm_remove_addr(struct mptcp_sock *msk, const struct mptcp_rm_list *rm_
 void mptcp_pm_remove_addr_entry(struct mptcp_sock *msk,
 				struct mptcp_pm_addr_entry *entry);
 
+extern struct mptcp_pm_ops mptcp_kernel_pm;
+
 struct mptcp_pm_ops *mptcp_pm_find(const char *name);
 int mptcp_pm_validate(struct mptcp_pm_ops *pm);
 int mptcp_pm_register(struct mptcp_pm_ops *pm);
-- 
2.43.0
Re: [PATCH mptcp-next v7 05/11] mptcp: pm: in-kernel: register mptcp_kernel_pm
Posted by Matthieu Baerts 11 months, 1 week ago
Hi Geliang,

On 03/03/2025 05:22, Geliang Tang wrote:
> From: Geliang Tang <tanggeliang@kylinos.cn>
> 
> This patch defines the original in-kernel netlink path manager as a
> new struct mptcp_pm_ops named "mptcp_kernel_pm", and register it in

Detail: should it not be called "mptcp_pm_kernel" with the usual
"mptcp_pm_" prefix like everywhere else (except in the userspace PM I see)?

> mptcp_pm_nl_init().
> 
> This mptcp_pm_ops will be skipped in mptcp_pm_unregister().

Why this exception here? Please add a comment in the code, and
eventually in the commit message if you need a longer explanation.

Why is it fine to unregister the userspace PM, and not the kernel one?
Can you not check the owner to see if it is an internal module for
example? Or add something in struct mptcp_pm_ops to know if the
unregister part is needed?

Also, mptcp_pm_unregister() is currently unused in this series, is it
normal?

> Only get_local_id() and get_priority() interfaces are implemented here.

Maybe they can all be implemented later on, see my comment on patch 1/11.

> diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h
> index 9dbfde4027b3..56d3a7457f80 100644
> --- a/net/mptcp/protocol.h
> +++ b/net/mptcp/protocol.h
> @@ -1051,6 +1051,8 @@ int mptcp_pm_remove_addr(struct mptcp_sock *msk, const struct mptcp_rm_list *rm_
>  void mptcp_pm_remove_addr_entry(struct mptcp_sock *msk,
>  				struct mptcp_pm_addr_entry *entry);
>  
> +extern struct mptcp_pm_ops mptcp_kernel_pm;

Can you add a comment in the commit message explaining why it needs to
be declared as extern? (or only do that when you need it elsewhere?)

Maybe enough to mention that it needs to be "extern" because it is the
default one?

Cheers,
Matt
-- 
Sponsored by the NGI0 Core fund.