Re: [PATCH mptcp-next v1 5/6] mptcp: pm: add remote parameter for set_flags

[Matthieu Baerts <matttbe@kernel.org>]

Hi Geliang,

On 27/02/2025 07:43, Geliang Tang wrote:
> From: Geliang Tang <tanggeliang@kylinos.cn>
> 
> The remote address of set_flags() interface is useful for userspace PM,
> but unused in in-kernel PM.
> 
> But an additional "changed" parameter needs to be passed to set_flags() of
> in-kernel PM. One option is to add a "u8 changed" parameter to set_flags()
> interface:
> 
> 	set_flags(struct mptcp_pm_addr_entry *local,
> 		  struct mptcp_addr_info *remote,
> 		  u8 changed)
> 
> A better option is to add a struct mptcp_pm_addr_entry "remote" parameter
> for set_flags(), so that "remote->addr" can be used for userspace PM, and
> "remote->flags" can be used for in-kernel PM to replace the additional
> "changed" parameter.

I don't think I understand why we need this modification.

The 'set_flags' netlink command is there to change the behaviour of the
in-kernel PM and the userspace PM. It will certainly not be useful for a
BPF PM because all its configurations will be done via BPF, not via
Netlink. If the userspace send this Netlink command when a BPF PM will
be used, nothing will happen:
- if a token is specified, the userspace PM should not do anything if it
is not the designated PM for this connection.
- if not, the in-kernel PM will only modify the connections handled by
the in-kernel PM.

The in-kernel and userspace PMs use the same command, but for different
things, hence requiring different parameters: e.g. the userspace one
requires the token because the modification will be only for one
connection, while the in-kernel will be for a specific endpoint.

So the current code looks OK: in the common part (pm.c), only the common
attributes are parsed. On the userspace side, extra parameter will be
parsed, that's fine. On the in-kernel side, 'changed' will be looked at
simply because there is no need to iterate over all the connections if
nothing has changed on the corresponding endpoint side. In other words,
I don't think the code should be changed, no?

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