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

[Geliang Tang <geliang@kernel.org>]

Hi Matt,

On Mon, 2025-02-24 at 09:26 +0100, Matthieu Baerts wrote:
> Hi Geliang,
> 
> On 24/02/2025 07:54, Geliang Tang wrote:
> > Hi Matt,
> > 
> > Thanks for the review.
> > 
> > On Fri, 2025-02-21 at 18:23 +0100, Matthieu Baerts wrote:
> > > 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
> > 
> > Do we need to restrict BPF path manager to only extend the
> > userspace
> > type path manager? That is, for all BPF path managers,
> > mptcp_pm_is_userspace() returns true. If this is the case, there is
> > no
> > need to add "type" field in struct mptcp_pm_ops.
> 
> To me, mptcp_pm_is_userspace() should no longer be needed, because
> pm->ops will be used instead. So the "type" field should no longer be

Currently, the path manager must rely on mptcp_pm_is_userspace().
mptcp_pm_is_userspace() is not needed for get_local_id() and
get_priority(), but it's useful for address_announced(),
address_removed(), subflow_created() and subflow_closed(). 

Take address_announced() as an example:

For userspace pm, address_announced() is invoked in
mptcp_pm_nl_announce_doit(), which is the handler of the command
MPTCP_PM_CMD_ANNOUNCE [1].

While for in-kernel pm, address_announced() is invoked in
mptcp_pm_nl_add_addr_doit(), which is the handler of the command
MPTCP_PM_CMD_ADD_ADDR [2].

When we implement a BPF path manager, we must tell the PM core which
path to use to call its address_announced() interface. So I added
"type" in struct mptcp_pm_ops, and each path manager needs to set it to
tell the PM core whether it is a userspace type path manager, and check
this type in mptcp_pm_is_userspace().

[1]
https://patchwork.kernel.org/project/mptcp/patch/6d39ed9364b41f84b273598f198fa1aa226a2cbc.1740047738.git.tanggeliang@kylinos.cn/
[2]
https://patchwork.kernel.org/project/mptcp/patch/5881dc057b4927f30070193bde21703f0079e233.1740047738.git.tanggeliang@kylinos.cn/

> needed. Only the name should be stored, and the net.mptcp.pm_type
> sysctl
> knob should depend only on the name. (I guess a lock will be needed
> to
> handle the case where both the "pm_type" and "path_manager" will be
> set
> at the same time.)
> 
> > Is it necessary to allow BPF to create kernel type path managers,
> > that
> > is, path managers for which mptcp_pm_is_userspace() returns false?
> > Since we registered mptcp_netlink_pm from this version, can the in-
> > kernel type path manager also be extended by BPF? If this is the
> > case,
> > we need to add "type" field in struct mptcp_pm_ops to distinguish
> > whether it is a userspace type path manager.
> 
> I guess a BPF path-manager will have its own mptcp_pm_ops structure,
> and
> it will not extend anything. Of course, it is possible to add kfunc
> to
> share some code. No?
> 
> > > 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.
> > 
> > In-kernel pm is for each MPTCP connection handled by this PM while
> > userspace pm is only for this one being initialized. How should I
> > deal
> > with this? Please give me more detailed suggestions.
> 
> Sorry, I'm not sure to understand your question. If one PM doesn't
> need
> to init and/or release anything, these ops can be set to NULL, no?
> 
> Cheers,
> Matt