Re: [PATCH mptcp-next v5 2/9] mptcp: pm: add struct mptcp_pm_param

[Matthieu Baerts <matttbe@kernel.org>]

Hi Geliang,

On 20/02/2025 03:57, Geliang Tang wrote:
> From: Geliang Tang <tanggeliang@kylinos.cn>
> 
> Generally, in the path manager interfaces, the local address is defined
> as an mptcp_pm_addr_entry type address, while the remote address is
> defined as an mptcp_addr_info type one:
> 
>     (struct mptcp_pm_addr_entry *local, struct mptcp_addr_info *remote)
> 
> In order to make these interfaces more flexible and extensible, a struct
> mptcp_pm_param is defined here to pass parameters. "entry" can be used
> as the local address entry, and "addr" can be used as the remote address.

Mmh, it is not clear to me why you cannot use only the parameters that
are needed per interfaces, e.g. "local" and "remote". These parameters
are not even always needed, e.g. when an address has been announced,
"remote" doesn't make sense here. Same for get_local_id and
get_priority. Similarly, when an address has been removed, only the ID
is needed for the PM, etc.

I'm not convinced by the "flexible and extensible" reasons for the
future: "local" and "remote" will likely not change. If we want to add
more parameters, will we require them for all interfaces?

Also, if you use similar signatures as the ones being used, no need to
do many changes in the current in-kernel and userspace PM, e.g. no need
to introduce the new helpers in patches 5 and 6. Instead, these patches
could directly set the available helpers, and we can reduce the number
of patches, e.g. patches 8 and 9 (and more) could all come in one, no?

Plus, it feels strange to copy data from one structure to another just
to pass parameters, no?

Are you sure we need this patch? (and the next one?)

> Also add a new helper mptcp_pm_param_set_contexts() to set a struct
> mptcp_pm_param type parameter.
> 
> Signed-off-by: Geliang Tang <tanggeliang@kylinos.cn>
> ---
>  include/net/mptcp.h  | 13 +++++++++++++
>  net/mptcp/pm.c       | 10 ++++++++++
>  net/mptcp/protocol.h | 11 +++--------
>  3 files changed, 26 insertions(+), 8 deletions(-)
> 
> diff --git a/include/net/mptcp.h b/include/net/mptcp.h
> index 72d6e6597add..a41d6c74760f 100644
> --- a/include/net/mptcp.h
> +++ b/include/net/mptcp.h
> @@ -121,6 +121,19 @@ struct mptcp_sched_ops {
>  	void (*release)(struct mptcp_sock *msk);
>  } ____cacheline_aligned_in_smp;
>  
> +struct mptcp_pm_addr_entry {
> +	struct list_head	list;
> +	struct mptcp_addr_info	addr;
> +	u8			flags;
> +	int			ifindex;
> +	struct socket		*lsk;
> +};
> +
> +struct mptcp_pm_param {
> +	struct mptcp_pm_addr_entry	entry;
> +	struct mptcp_addr_info		addr;
If this structure is really needed, it seems clearer to use "local" and
"remote" instead of "entry" and "addr".

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