Re: [PATCH mptcp-next v5 4/9] mptcp: pm: define struct mptcp_pm_ops

[Matthieu Baerts <matttbe@kernel.org>]

Hi Geliang,

On 20/02/2025 03:57, Geliang Tang wrote:
> From: Geliang Tang <tanggeliang@kylinos.cn>
> 
> In order to allow users to develop their own BPF-based path manager,
> this patch defines a struct ops "mptcp_pm_ops" for a userspace path
> manager, which contains a set of interfaces.
> 
> Add a set of functions to register, unregister, find and validate a
> given struct ops.
> 
> Signed-off-by: Geliang Tang <tanggeliang@kylinos.cn>
> ---
>  include/net/mptcp.h  | 29 ++++++++++++++++++++++
>  net/mptcp/pm.c       | 59 ++++++++++++++++++++++++++++++++++++++++++++
>  net/mptcp/protocol.h |  5 ++++
>  3 files changed, 93 insertions(+)
> 
> diff --git a/include/net/mptcp.h b/include/net/mptcp.h
> index a41d6c74760f..f51e75d3882d 100644
> --- a/include/net/mptcp.h
> +++ b/include/net/mptcp.h
> @@ -134,6 +134,35 @@ struct mptcp_pm_param {
>  	struct mptcp_addr_info		addr;
>  };
>  
> +struct mptcp_pm_ops {
> +	int (*created)(struct mptcp_sock *msk);
> +	int (*established)(struct mptcp_sock *msk);
> +	int (*closed)(struct mptcp_sock *msk);
> +	int (*address_announced)(struct mptcp_sock *msk,
> +				 struct mptcp_pm_param *param);
> +	int (*address_removed)(struct mptcp_sock *msk,
> +			       struct mptcp_pm_param *param);
> +	int (*subflow_established)(struct mptcp_sock *msk,
> +				   struct mptcp_pm_param *param);
> +	int (*subflow_closed)(struct mptcp_sock *msk,
> +			      struct mptcp_pm_param *param);
> +	int (*get_local_id)(struct mptcp_sock *msk,
> +			    struct mptcp_pm_param *param);
> +	bool (*get_priority)(struct mptcp_sock *msk,
> +			     struct mptcp_pm_param *param);
> +	int (*set_priority)(struct mptcp_sock *msk,
> +			    struct mptcp_pm_param *param);
> +	int (*listener_created)(struct mptcp_sock *msk);
> +	int (*listener_closed)(struct mptcp_sock *msk);
> +
> +	u8			type;

I guess the type matches net.mptcp.pm_type sysctl knob, right?

I wonder if we should not deprecate this sysctl, and use a string like
with the scheduler. So instead, we could have:

  char			name[MPTCP_PM_NAME_MAX];

And on ctrl.c, we could map pm_type for the moment with a custom
proc_handler (and even remove this sysctl knob in a few releases):

 - 0  → in-kernel
 - 1  → userspace
 - >1 → bpf

WDYT? Would it not be clearer for devs and users?

Note that if for the implementation, if it is easier to keep this "type"
entry for the moment for the sysctl stuff, I'm fine with that. But if we
don't need it, let's not introduce it.

> +	struct module		*owner;
> +	struct list_head	list;
> +
> +	void (*init)(struct mptcp_sock *msk);
> +	void (*release)(struct mptcp_sock *msk);

To answer my question from the v3 review: the init/release is done for
each MPTCP connection handled by this PM.

> +} ____cacheline_aligned_in_smp;
> +
>  #ifdef CONFIG_MPTCP
>  void mptcp_init(void);
>  
> diff --git a/net/mptcp/pm.c b/net/mptcp/pm.c
> index e3457f34621c..f56b2d1e3409 100644
> --- a/net/mptcp/pm.c
> +++ b/net/mptcp/pm.c
> @@ -6,12 +6,17 @@
>  #define pr_fmt(fmt) "MPTCP: " fmt
>  
>  #include <linux/kernel.h>
> +#include <linux/rculist.h>
> +#include <linux/spinlock.h>
>  #include <net/mptcp.h>
>  #include "protocol.h"
>  
>  #include "mib.h"
>  #include "mptcp_pm_gen.h"
>  
> +static DEFINE_SPINLOCK(mptcp_pm_list_lock);
> +static LIST_HEAD(mptcp_pm_list);
> +
>  /* path manager command handlers */
>  
>  int mptcp_pm_announce_addr(struct mptcp_sock *msk,
> @@ -661,3 +666,57 @@ void __init mptcp_pm_init(void)
>  {
>  	mptcp_pm_nl_init();
>  }
> +
> +/* Must be called with rcu read lock held */
> +struct mptcp_pm_ops *mptcp_pm_find(enum mptcp_pm_type type)
> +{
> +	struct mptcp_pm_ops *pm;
> +
> +	list_for_each_entry_rcu(pm, &mptcp_pm_list, list) {
> +		if (pm->type == type)
> +			return pm;
> +	}
> +
> +	return NULL;
> +}
> +
> +int mptcp_pm_validate(struct mptcp_pm_ops *pm)
> +{
> +	if (!pm->created && !pm->established && !pm->closed &&
> +	    !pm->address_announced && !pm->address_removed &&
> +	    !pm->subflow_established && !pm->subflow_closed &&
> +	    !pm->get_local_id && !pm->get_priority && !pm->set_priority &&
> +	    !pm->listener_created && !pm->listener_closed) {

I'm not sure to understand the purpose of this validation.

Why not forcing some or all of them?

(...)

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