[PATCH mptcp-next] mptcp: pm: drop redundant mptcp_pm_send_ack

[PATCH mptcp-next] mptcp: pm: drop redundant mptcp_pm_send_ack

[Geliang Tang]

From: Geliang Tang <tanggeliang@kylinos.cn>

mptcp_pm_send_ack() is called twice in __mptcp_pm_addr_send_ack(), which
makes the code a bit redundant. The first call to mptcp_pm_send_ack() when
a non-stale subflow is found in the loop can be removed. Instead, we can
break the loop to use the second mptcp_pm_send_ack().

Signed-off-by: Geliang Tang <tanggeliang@kylinos.cn>
---
 net/mptcp/pm.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/net/mptcp/pm.c b/net/mptcp/pm.c
index be1e27ee393e..a4f9cb113018 100644
--- a/net/mptcp/pm.c
+++ b/net/mptcp/pm.c
@@ -225,8 +225,8 @@ void mptcp_pm_addr_send_ack(struct mptcp_sock *msk)
 	mptcp_for_each_subflow(msk, subflow) {
 		if (__mptcp_subflow_active(subflow)) {
 			if (!subflow->stale) {
-				mptcp_pm_send_ack(msk, subflow, false, false);
-				return;
+				alt = subflow;
+				break;
 			}
 
 			if (!alt)
-- 
2.43.0

Re: [PATCH mptcp-next] mptcp: pm: drop redundant mptcp_pm_send_ack

[MPTCP CI]

Hi Geliang,

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/14282993422

Initiator: Patchew Applier
Commits: https://github.com/multipath-tcp/mptcp_net-next/commits/7673e997429f
Patchwork: https://patchwork.kernel.org/project/mptcp/list/?series=950175


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)

Re: [PATCH mptcp-next] mptcp: pm: drop redundant mptcp_pm_send_ack

[Matthieu Baerts]

Hi Geliang,

On 05/04/2025 16:33, Geliang Tang wrote:
> From: Geliang Tang <tanggeliang@kylinos.cn>
> 
> mptcp_pm_send_ack() is called twice in __mptcp_pm_addr_send_ack(), which
> makes the code a bit redundant. The first call to mptcp_pm_send_ack() when
> a non-stale subflow is found in the loop can be removed. Instead, we can
> break the loop to use the second mptcp_pm_send_ack().
> 
> Signed-off-by: Geliang Tang <tanggeliang@kylinos.cn>
> ---
>  net/mptcp/pm.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/net/mptcp/pm.c b/net/mptcp/pm.c
> index be1e27ee393e..a4f9cb113018 100644
> --- a/net/mptcp/pm.c
> +++ b/net/mptcp/pm.c
> @@ -225,8 +225,8 @@ void mptcp_pm_addr_send_ack(struct mptcp_sock *msk)
>  	mptcp_for_each_subflow(msk, subflow) {
>  		if (__mptcp_subflow_active(subflow)) {
>  			if (!subflow->stale) {
> -				mptcp_pm_send_ack(msk, subflow, false, false);
> -				return;
> +				alt = subflow;
> +				break;

Is this patch going to simplify another one later on? Because the
modification sounds strange: here 'alt' stands for 'alternative'. So to
be complete, better to rename the variable as well.

But... is this really worth it? If you need to modify this helper later
on, maybe. If not, I guess it is fine to keep 'mptcp_pm_send_ack()'
duplicated for the moment. WDYT?

This is also to be aligned with:

 https://docs.kernel.org/process/maintainer-netdev.html#clean-up-patches

(trying to avoid cleanup patches "alone", except if it is part of
another set of changes related to that)

>  			}
>  
>  			if (!alt)
Cheers,
Matt
-- 
Sponsored by the NGI0 Core fund.

Re: [PATCH mptcp-next] mptcp: pm: drop redundant mptcp_pm_send_ack

[Geliang Tang]

Hi Matt,

Thanks for the review.

On Thu, 2025-04-10 at 19:38 +0200, Matthieu Baerts wrote:
> Hi Geliang,
> 
> On 05/04/2025 16:33, Geliang Tang wrote:
> > From: Geliang Tang <tanggeliang@kylinos.cn>
> > 
> > mptcp_pm_send_ack() is called twice in __mptcp_pm_addr_send_ack(),
> > which
> > makes the code a bit redundant. The first call to
> > mptcp_pm_send_ack() when
> > a non-stale subflow is found in the loop can be removed. Instead,
> > we can
> > break the loop to use the second mptcp_pm_send_ack().
> > 
> > Signed-off-by: Geliang Tang <tanggeliang@kylinos.cn>
> > ---
> >  net/mptcp/pm.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git a/net/mptcp/pm.c b/net/mptcp/pm.c
> > index be1e27ee393e..a4f9cb113018 100644
> > --- a/net/mptcp/pm.c
> > +++ b/net/mptcp/pm.c
> > @@ -225,8 +225,8 @@ void mptcp_pm_addr_send_ack(struct mptcp_sock
> > *msk)
> >  	mptcp_for_each_subflow(msk, subflow) {
> >  		if (__mptcp_subflow_active(subflow)) {
> >  			if (!subflow->stale) {
> > -				mptcp_pm_send_ack(msk, subflow,
> > false, false);
> > -				return;
> > +				alt = subflow;
> > +				break;
> 
> Is this patch going to simplify another one later on? Because the
> modification sounds strange: here 'alt' stands for 'alternative'. So
> to
> be complete, better to rename the variable as well.

Better to rename it as "pick" or something.

> 
> But... is this really worth it? If you need to modify this helper
> later
> on, maybe. If not, I guess it is fine to keep 'mptcp_pm_send_ack()'
> duplicated for the moment. WDYT?

Sure. Let's drop this patch now, and resend it with other patches in
the future if we have other modifications in this helper.

Thanks,
-Geliang

> 
> This is also to be aligned with:
> 
>  
> https://docs.kernel.org/process/maintainer-netdev.html#clean-up-patche
> s
> 
> (trying to avoid cleanup patches "alone", except if it is part of
> another set of changes related to that)
> 
> >  			}
> >  
> >  			if (!alt)
> Cheers,
> Matt