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

[Geliang Tang <geliang@kernel.org>]

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