Re: [PATCH mptcp-next v2 23/36] mptcp: drop struct mptcp_pm_local

[Matthieu Baerts <matttbe@kernel.org>]

Hi Geliang,

On 22/10/2024 11:14, Geliang Tang wrote:
> From: Geliang Tang <tanggeliang@kylinos.cn>
> 
> The following code in mptcp_pm_nl_subflow_create_doit() that assigns struct
> mptcp_pm_addr_entry "entry" to the local struct mptcp_pm_local variable
> "local" is not allowed in BPF if we use the same code to implement the
> subflow_create() interface of a BFP path manager:
> 
> 	struct mptcp_pm_local local;
> 
> 	local.addr = entry.addr;
> 	local.flags = entry.flags;
> 	local.ifindex = entry.ifindex;

Here as well, I'm not sure to understand the issue. Is it because you
cannot allocate this structure locally with BPF? No alternatives?

> We should avoid this type of assignment from struct mptcp_pm_addr_entry to
> struct mptcp_pm_local.
> 
> In fact, there is no need to add a dedicated address entry type for local
> address entry. All its fields are the same as struct mptcp_pm_addr_entry,
> except that it lacks a "lsk" for the listening socket. So we can use struct
> mptcp_pm_addr_entry directly. This makes the path manager code simpler.

There was a need for this structure, see this discussion:


https://lore.kernel.org/mptcp/587e096f-0e76-40fd-9220-6d8dd2930838@redhat.com/

and the changelog in v5:


https://lore.kernel.org/mptcp/20240726-mptcp-pm-avail-v5-13-fb1117ddeef6@kernel.org/

In short ...

> 
> Signed-off-by: Geliang Tang <tanggeliang@kylinos.cn>
> ---
>  net/mptcp/pm_netlink.c   | 22 ++++++++--------------
>  net/mptcp/pm_userspace.c |  7 +------
>  net/mptcp/protocol.h     |  8 +-------
>  net/mptcp/subflow.c      |  2 +-
>  4 files changed, 11 insertions(+), 28 deletions(-)
> 
> diff --git a/net/mptcp/pm_netlink.c b/net/mptcp/pm_netlink.c
> index 287e715bcf68..15f97e7af182 100644
> --- a/net/mptcp/pm_netlink.c
> +++ b/net/mptcp/pm_netlink.c

(...)

> @@ -721,7 +715,7 @@ static unsigned int fill_local_addresses_vec(struct mptcp_sock *msk,
>  
>  static void mptcp_pm_nl_add_addr_received(struct mptcp_sock *msk)
>  {
> -	struct mptcp_pm_local locals[MPTCP_PM_ADDR_MAX];
> +	struct mptcp_pm_addr_entry locals[MPTCP_PM_ADDR_MAX];

... we don't want that: that's reserving too much memory, and we don't
need half of it.

*If* it is really needed to change it for BPF (I'm surprised there are
no alternatives), then maybe we can change the signature of
__mptcp_subflow_connect(), but not change the code here.

In this case, only one "struct mptcp_pm_addr_entry" will be needed, and
the "struct mptcp_pm_local" can be defined in this pm_netlink.c file if
it is only used here.

But please mention that in the commit message.

>  	struct sock *sk = (struct sock *)msk;
>  	unsigned int add_addr_accept_max;
>  	struct mptcp_addr_info remote;
(...)

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