Re: [PATCH mptcp-next v2 00/14] mptcp: pm: code reorganisation

[Geliang Tang <geliang@kernel.org>]

Hi Matt,

On Fri, 2025-02-28 at 14:31 +0100, Matthieu Baerts (NGI0) wrote:
> I have been thinking about that during the past couple of months, but
> now, with the recent modifications done by Geliang, and the
> associated
> reviews, I really think it is time to reorganise the code around the
> path-managers to avoid confusions, and a mix of code used in
> different
> conditions.
> 
> I hope this will not cause too many issues with the in-progress
> patches.
> I tried only to move the code around, and minimise the functions
> renaming. Then, to switch to the new code, it might be easier to
> simply
> copy your modified code from the old place to the new one. In other
> words, duplicate the net/mptcp directory, then do a rebase, resolve
> the
> conflicts with 'git restore --ours', then reapply the modifications
> by
> copying your old code to the new place. If it is too difficult,
> please
> let me know.
> 
> Before this series, the PM code was dispersed in different places:
> 
> - pm.c had common code for all PMs
> 
> - pm_netlink.c was supposed to be about the in-kernel PM, but also
> had
>   exported common helpers, callbacks used by the different PMs, NL
>   events for PM userspace daemon, etc. quite confusing.
> 
> - pm_userspace.c had userspace PM only code, but using specific
>   in-kernel PM helpers
> 
> To clarify the code, a reorganisation is suggested here, only by
> moving
> code around, and small helper renaming to avoid confusions:
> 
> - pm_netlink.c now only contains common PM Netlink code:
>   - PM events: this code was already there
>   - shared helpers around Netlink code that were already there as
> well
>   - shared Netlink commands code from pm.c
> 
> - pm_kernel.c now contains only code that is specific to the in-
> kernel
>   PM. Now all functions are either called from:
>   - pm.c: events coming from the core, when this PM is being used
>   - pm_netlink.c: for shared Netlink commands
>   - mptcp_pm_gen.c: for Netlink commands specific to the in-kernel PM
>   - sockopt.c: for the exported counters per netns
> 
> - pm.c got many code from pm_netlink.c:
>   - helpers used from both PMs and not linked to Netlink
>   - callbacks used by different PMs, e.g. ADD_ADDR management
>   - some helpers have been renamed to remove the '_nl' prefix, and
> they
>     have been marked as 'static'.
> 
> - protocol.h has been updated accordingly:
>   - some helpers no longer need to be exported
>   - new ones needed to be exported: they have been prefixed if
> needed.
> 
> The code around the PM is now less confusing, which should help for
> the
> maintenance in the long term.
> 
> This will certainly impact future backports, but because other
> cleanups
> have already done recently, and more are coming to ease the addition
> of
> a new path-manager controlled with BPF (struct_ops), doing that now
> seems to be a good time. Also, many issues around the PM have been
> fixed
> a few months ago while increasing the code coverage in the selftests,
> so
> such big reorganisation can be done with more confidence now.
> 
> Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
> ---
> Changes in v2:
> - Addressing Geliang's comments
> - The renaming of common helpers have been done in dedicated patches
> - Some helpers have been kept where they were in pm.c, except one now
> in
>   a dedicated patch.
> - The last patch has been split in smaller chunks, only moving code
> from
>   one file to another to help with the reviews.

Thanks for this v2, I have some comments for it:

Patches 1, 2, 5 can be merged into one.
Patch 3 can be merged into patch 9.
Patch 4, change mptcp_pm_rm_addr_recv() as non-static here, instead of
changing it in patch 9. How about renaming it as
__mptcp_pm_nl_rm_addr_received?
Patch 7, how about renaming mptcp_pm_nl_set_flags_all as
__mptcp_pm_nl_set_flags too?
Patch 11, how about dropping this patch, keep
mptcp_pm_addr_families_match() unmoved, but add a new section like "/*
generic PM helpers */" to import the helpers there?
Patch 12
 - in mptcp_remove_anno_list_by_saddr() behavioural is changed, a
separate patch is needed.
 - mptcp_lookup_subflow_by_saddr should be after mptcp_remote_address
 - mptcp_pm_sport_in_anno_list should be after
mptcp_lookup_anno_list_by_saddr and befor mptcp_pm_add_timer.
 - split "move all struct mptcp_pm_add_entry related code into pm.c"
from patch 12 as a separate one.
 - split "move all send_ack related code into pm.c" from patch 12 as a
separate one.
 - make mptcp_pm_is_init_remote_addr() static shoud be merged into
patch 6.
Split "move all pm_nl_pernet related code into pm_kernel.c" from patch
13 as a separate one.
Can the action of deleting three headers in patch 14 be advanced to the
previous patch?

Thanks,
-Geliang

> - Link to v1:
> https://lore.kernel.org/r/20250227-mptcp-pm-reorg-code-v1-0-cb4677096709@kernel.org
> 
> ---
> Matthieu Baerts (NGI0) (14):
>       mptcp: pm: remove '_nl' from mptcp_pm_nl_addr_send_ack
>       mptcp: pm: remove '_nl' from mptcp_pm_nl_mp_prio_send_ack
>       mptcp: pm: remove '_nl' from mptcp_pm_nl_work
>       mptcp: pm: remove '_nl' from mptcp_pm_nl_rm_addr_received
>       mptcp: pm: remove '_nl' from mptcp_pm_nl_subflow_chk_stale()
>       mptcp: pm: remove '_nl' from mptcp_pm_nl_is_init_remote_addr
>       mptcp: pm: kernel: add '_pm' to mptcp_nl_set_flags
>       mptcp: pm: avoid calling PM specific code from core
>       mptcp: pm: worker: split in-kernel and common tasks
>       mptcp: pm: export mptcp_remote_address
>       mptcp: pm: move generic helper at the top
>       mptcp: pm: move generic PM helpers to pm.c
>       mptcp: pm: split in-kernel PM specific code
>       mptcp: pm: move Netlink PM helpers to pm_netlink.c
> 
>  net/mptcp/Makefile       |    2 +-
>  net/mptcp/pm.c           |  644 +++++++++++----
>  net/mptcp/pm_kernel.c    | 1410 +++++++++++++++++++++++++++++++++
>  net/mptcp/pm_netlink.c   | 1934 ++----------------------------------
> ----------
>  net/mptcp/pm_userspace.c |   11 +-
>  net/mptcp/protocol.c     |    5 +-
>  net/mptcp/protocol.h     |   36 +-
>  7 files changed, 2029 insertions(+), 2013 deletions(-)
> ---
> base-commit: 6b32949274eb9e32fe867ba9a8f8cb98587ecf36
> change-id: 20250227-mptcp-pm-reorg-code-13227530bd05
> 
> Best regards,