| Message ID | 20250227-mptcp-pm-reorg-code-v1-0-cb4677096709@kernel.org (mailing list archive) |
|---|---|
| Headers | show |
| Series | mptcp: pm: code reorganisation | expand |
Hi Matthieu, Thank you for your modifications, that's great! But sadly, our CI spotted some issues with it when trying to build it. You can find more details there: https://github.com/multipath-tcp/mptcp_net-next/actions/runs/13575269560 Status: failure Initiator: Patchew Applier Commits: https://github.com/multipath-tcp/mptcp_net-next/commits/423721784d18 Patchwork: https://patchwork.kernel.org/project/mptcp/list/?series=938681 Feel free to reply to this email if you cannot access logs, if you need some support to fix the error, if this doesn't seem to be caused by your modifications or if the error is a false positive one. Cheers, MPTCP GH Action bot Bot operated by Matthieu Baerts (NGI0 Core)
Hi Matthieu,
Thank you for your modifications, that's great!
Our CI did some validations and here is its report:
- KVM Validation: normal: Success! ✅
- KVM Validation: debug: Success! ✅
- KVM Validation: btf-normal (only bpftest_all): Success! ✅
- KVM Validation: btf-debug (only bpftest_all): Success! ✅
- Task: https://github.com/multipath-tcp/mptcp_net-next/actions/runs/13575269556
Initiator: Patchew Applier
Commits: https://github.com/multipath-tcp/mptcp_net-next/commits/423721784d18
Patchwork: https://patchwork.kernel.org/project/mptcp/list/?series=938681
If there are some issues, you can reproduce them using the same environment as
the one used by the CI thanks to a docker image, e.g.:
$ cd [kernel source code]
$ docker run -v "${PWD}:${PWD}:rw" -w "${PWD}" --privileged --rm -it \
--pull always mptcp/mptcp-upstream-virtme-docker:latest \
auto-normal
For more details:
https://github.com/multipath-tcp/mptcp-upstream-virtme-docker
Please note that despite all the efforts that have been already done to have a
stable tests suite when executed on a public CI like here, it is possible some
reported issues are not due to your modifications. Still, do not hesitate to
help us improve that ;-)
Cheers,
MPTCP GH Action bot
Bot operated by Matthieu Baerts (NGI0 Core)
Hello, 27 Feb 2025 21:39:01 MPTCP CI <wpasupplicant.patchew@gmail.com>: > But sadly, our CI spotted some issues with it when trying to build it. > > You can find more details there: > > https://github.com/multipath-tcp/mptcp_net-next/actions/runs/13575269560 One of the errors is a false positive one, just because the ignore list needs to be adapted because two functions have been moved to another fine. The other one is a small issue due to a last minute commit split, I can fix that in a v2 or when applying the patches. Cheers, Matt
Hi Matt, On Thu, 2025-02-27 at 21:08 +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. > > Here is a first proposition, feel free to comment! :) > > Please look at the last patch for more details about the why and why > now. > > 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. > > Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org> > --- > Cc: Geliang Tang <geliang@kernel.org> > > --- > Matthieu Baerts (NGI0) (7): > 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_or_subflow > mptcp: pm: avoid calling PM specific code from core > mptcp: pm: worker: split in-kernel and common tasks > mptcp: pm: code reorganisation > > net/mptcp/Makefile | 2 +- > net/mptcp/pm.c | 685 ++++++++++++---- > net/mptcp/pm_kernel.c | 1410 +++++++++++++++++++++++++++++++++ > net/mptcp/pm_netlink.c | 1934 ++---------------------------------- > ---------- Patch 7 has too many changes to review, so I suggest splitting it into multiple small patches for review, and then merging them into a large patch after review. I think it is possible to make "move the code from pm_netlink.c to pm.c" as a separate patch, and "move the code from pm_netlink.c to pm_kernel.c" as another separate patch. Moreover, these two patches only involve the movement of code, without any renaming or relocation of functions, and these changes should be done by separate patches. In addition, we can try to keep the original positions of the functions that were originally in pm.c, such as mptcp_pm_addr_families_match, mptcp_pm_schedule_work, and mptcp_pm_add_addr_send_ack. WDYT? Thanks, -Geliang > net/mptcp/pm_userspace.c | 11 +- > net/mptcp/protocol.c | 5 +- > net/mptcp/protocol.h | 37 +- > 7 files changed, 2051 insertions(+), 2033 deletions(-) > --- > base-commit: 6b32949274eb9e32fe867ba9a8f8cb98587ecf36 > change-id: 20250227-mptcp-pm-reorg-code-13227530bd05 > > Best regards,
Hi Geliang, On 28/02/2025 04:27, Geliang Tang wrote: > On Thu, 2025-02-27 at 21:08 +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. >> >> Here is a first proposition, feel free to comment! :) >> >> Please look at the last patch for more details about the why and why >> now. >> >> 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. >> >> Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org> >> --- >> Cc: Geliang Tang <geliang@kernel.org> >> >> --- >> Matthieu Baerts (NGI0) (7): >> 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_or_subflow >> mptcp: pm: avoid calling PM specific code from core >> mptcp: pm: worker: split in-kernel and common tasks >> mptcp: pm: code reorganisation >> >> net/mptcp/Makefile | 2 +- >> net/mptcp/pm.c | 685 ++++++++++++---- >> net/mptcp/pm_kernel.c | 1410 +++++++++++++++++++++++++++++++++ >> net/mptcp/pm_netlink.c | 1934 ++---------------------------------- >> ---------- > > Patch 7 has too many changes to review, so I suggest splitting it into > multiple small patches for review, and then merging them into a large > patch after review. Thank you for looking at that. If we split, maybe no need to merge them later. > I think it is possible to make "move the code from pm_netlink.c to > pm.c" as a separate patch, and "move the code from pm_netlink.c to > pm_kernel.c" as another separate patch. Moreover, these two patches > only involve the movement of code, without any renaming or relocation > of functions, and these changes should be done by separate patches. OK, I will try to do that. > In addition, we can try to keep the original positions of the functions > that were originally in pm.c, such as mptcp_pm_addr_families_match, > mptcp_pm_schedule_work, and mptcp_pm_add_addr_send_ack. I moved mptcp_pm_addr_families_match() to be with all the new "addr helpers" coming from pm_netlink.c. It looks better there. Maybe I can do the move in a separated patch. For the two other helpers, that's because mptcp_pm_add_addr_send_ack() is now static, but called by code imported from pm_netlink.c. Maybe I can add a signature above to ease the review here. Would that be OK? Cheers, Matt
Hi Matt, On Fri, 2025-02-28 at 11:09 +0100, Matthieu Baerts wrote: > Hi Geliang, > > On 28/02/2025 04:27, Geliang Tang wrote: > > On Thu, 2025-02-27 at 21:08 +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. > > > > > > Here is a first proposition, feel free to comment! :) > > > > > > Please look at the last patch for more details about the why and > > > why > > > now. > > > > > > 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. I have successfully rebased all of my in-progress patches on this set today. > > > > > > Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org> > > > --- > > > Cc: Geliang Tang <geliang@kernel.org> > > > > > > --- > > > Matthieu Baerts (NGI0) (7): > > > 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_or_subflow > > > mptcp: pm: avoid calling PM specific code from core > > > mptcp: pm: worker: split in-kernel and common tasks > > > mptcp: pm: code reorganisation > > > > > > net/mptcp/Makefile | 2 +- > > > net/mptcp/pm.c | 685 ++++++++++++---- > > > net/mptcp/pm_kernel.c | 1410 > > > +++++++++++++++++++++++++++++++++ > > > net/mptcp/pm_netlink.c | 1934 ++------------------------------ > > > ---- > > > ---------- > > > > Patch 7 has too many changes to review, so I suggest splitting it > > into > > multiple small patches for review, and then merging them into a > > large > > patch after review. > > Thank you for looking at that. If we split, maybe no need to merge > them > later. > > > I think it is possible to make "move the code from pm_netlink.c to > > pm.c" as a separate patch, and "move the code from pm_netlink.c to > > pm_kernel.c" as another separate patch. Moreover, these two patches > > only involve the movement of code, without any renaming or > > relocation > > of functions, and these changes should be done by separate patches. > > OK, I will try to do that. > > > In addition, we can try to keep the original positions of the > > functions > > that were originally in pm.c, such as mptcp_pm_addr_families_match, > > mptcp_pm_schedule_work, and mptcp_pm_add_addr_send_ack. > > I moved mptcp_pm_addr_families_match() to be with all the new "addr > helpers" coming from pm_netlink.c. It looks better there. Maybe I can > do > the move in a separated patch. > > For the two other helpers, that's because > mptcp_pm_add_addr_send_ack() It's better not to change mptcp_pm_add_addr_send_ack() to static. There's such code in mptcp_pm_add_addr_received(): if (mptcp_pm_is_userspace(msk)) { if (mptcp_userspace_pm_active(msk)) { mptcp_pm_announce_addr(msk, addr, true); mptcp_pm_add_addr_send_ack(msk); } else { This part will be moved into pm_userspace.c later, then it will invoke mptcp_pm_add_addr_send_ack() from outside. So it's better to keep mptcp_pm_add_addr_send_ack() as it. Thanks, -Geliang > is now static, but called by code imported from pm_netlink.c. Maybe I > can add a signature above to ease the review here. Would that be OK? > > Cheers, > Matt
On 28/02/2025 11:31, Geliang Tang wrote: > Hi Matt, > > On Fri, 2025-02-28 at 11:09 +0100, Matthieu Baerts wrote: >> Hi Geliang, >> >> On 28/02/2025 04:27, Geliang Tang wrote: >>> On Thu, 2025-02-27 at 21:08 +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. >>>> >>>> Here is a first proposition, feel free to comment! :) >>>> >>>> Please look at the last patch for more details about the why and >>>> why >>>> now. >>>> >>>> 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. > > I have successfully rebased all of my in-progress patches on this set > today. Great, I hope that didn't break too many things. If it did, hopefully, it was for good reasons! > >>>> >>>> Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org> >>>> --- >>>> Cc: Geliang Tang <geliang@kernel.org> >>>> >>>> --- >>>> Matthieu Baerts (NGI0) (7): >>>> 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_or_subflow >>>> mptcp: pm: avoid calling PM specific code from core >>>> mptcp: pm: worker: split in-kernel and common tasks >>>> mptcp: pm: code reorganisation >>>> >>>> net/mptcp/Makefile | 2 +- >>>> net/mptcp/pm.c | 685 ++++++++++++---- >>>> net/mptcp/pm_kernel.c | 1410 >>>> +++++++++++++++++++++++++++++++++ >>>> net/mptcp/pm_netlink.c | 1934 ++------------------------------ >>>> ---- >>>> ---------- >>> >>> Patch 7 has too many changes to review, so I suggest splitting it >>> into >>> multiple small patches for review, and then merging them into a >>> large >>> patch after review. >> >> Thank you for looking at that. If we split, maybe no need to merge >> them >> later. >> >>> I think it is possible to make "move the code from pm_netlink.c to >>> pm.c" as a separate patch, and "move the code from pm_netlink.c to >>> pm_kernel.c" as another separate patch. Moreover, these two patches >>> only involve the movement of code, without any renaming or >>> relocation >>> of functions, and these changes should be done by separate patches. >> >> OK, I will try to do that. >> >>> In addition, we can try to keep the original positions of the >>> functions >>> that were originally in pm.c, such as mptcp_pm_addr_families_match, >>> mptcp_pm_schedule_work, and mptcp_pm_add_addr_send_ack. >> >> I moved mptcp_pm_addr_families_match() to be with all the new "addr >> helpers" coming from pm_netlink.c. It looks better there. Maybe I can >> do >> the move in a separated patch. >> >> For the two other helpers, that's because >> mptcp_pm_add_addr_send_ack() > > It's better not to change mptcp_pm_add_addr_send_ack() to static. > There's such code in mptcp_pm_add_addr_received(): > > if (mptcp_pm_is_userspace(msk)) { > if (mptcp_userspace_pm_active(msk)) { > mptcp_pm_announce_addr(msk, addr, true); > mptcp_pm_add_addr_send_ack(msk); > } else { > > This part will be moved into pm_userspace.c later, then it will invoke > mptcp_pm_add_addr_send_ack() from outside. So it's better to keep > mptcp_pm_add_addr_send_ack() as it. OK, I can undo that. Cheers, Matt
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. Here is a first proposition, feel free to comment! :) Please look at the last patch for more details about the why and why now. 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. Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org> --- Cc: Geliang Tang <geliang@kernel.org> --- Matthieu Baerts (NGI0) (7): 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_or_subflow mptcp: pm: avoid calling PM specific code from core mptcp: pm: worker: split in-kernel and common tasks mptcp: pm: code reorganisation net/mptcp/Makefile | 2 +- net/mptcp/pm.c | 685 ++++++++++++---- 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 | 37 +- 7 files changed, 2051 insertions(+), 2033 deletions(-) --- base-commit: 6b32949274eb9e32fe867ba9a8f8cb98587ecf36 change-id: 20250227-mptcp-pm-reorg-code-13227530bd05 Best regards,