If an entrypoint has no type -- so not 'subflow', 'signal', 'implicit' --
there are then no subflows to re-create from this local endpoint.

In this case, there is then no need to iterate over all connections to
do nothing. So stop early when this case is present.

Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
---
 net/mptcp/pm_netlink.c | 15 ++++++++-------
 1 file changed, 8 insertions(+), 7 deletions(-)

diff --git a/net/mptcp/pm_netlink.c b/net/mptcp/pm_netlink.c
index ff1e5695dc1db5e32d5f45bef7cf22e43aea0ef1..b1fe2a74fcfe97896de8f9eaee9a1afa5378fabb 100644
--- a/net/mptcp/pm_netlink.c
+++ b/net/mptcp/pm_netlink.c
@@ -1923,11 +1923,16 @@ static void mptcp_pm_nl_fullmesh(struct mptcp_sock *msk,
 }
 
 static void mptcp_nl_set_flags(struct net *net, struct mptcp_addr_info *addr,
-			       u8 bkup, u8 changed)
+			       u8 flags, u8 changed)
 {
+	u8 is_subflow = !!(flags & MPTCP_PM_ADDR_FLAG_SUBFLOW);
+	u8 bkup = !!(flags & MPTCP_PM_ADDR_FLAG_BACKUP);
 	long s_slot = 0, s_num = 0;
 	struct mptcp_sock *msk;
 
+	if (changed == MPTCP_PM_ADDR_FLAG_FULLMESH && !is_subflow)
+		return;
+
 	while ((msk = mptcp_token_iter_next(net, &s_slot, &s_num)) != NULL) {
 		struct sock *sk = (struct sock *)msk;
 
@@ -1937,7 +1942,7 @@ static void mptcp_nl_set_flags(struct net *net, struct mptcp_addr_info *addr,
 		lock_sock(sk);
 		if (changed & MPTCP_PM_ADDR_FLAG_BACKUP)
 			mptcp_pm_nl_mp_prio_send_ack(msk, addr, NULL, bkup);
-		if (changed & MPTCP_PM_ADDR_FLAG_FULLMESH)
+		if (is_subflow && (changed & MPTCP_PM_ADDR_FLAG_FULLMESH))
 			mptcp_pm_nl_fullmesh(msk, addr);
 		release_sock(sk);
 
@@ -1959,7 +1964,6 @@ int mptcp_pm_nl_set_flags(struct mptcp_pm_addr_entry *local,
 	struct mptcp_pm_addr_entry *entry;
 	struct pm_nl_pernet *pernet;
 	u8 lookup_by_id = 0;
-	u8 bkup = 0;
 
 	pernet = pm_nl_get_pernet(net);
 
@@ -1972,9 +1976,6 @@ int mptcp_pm_nl_set_flags(struct mptcp_pm_addr_entry *local,
 		}
 	}
 
-	if (local->flags & MPTCP_PM_ADDR_FLAG_BACKUP)
-		bkup = 1;
-
 	spin_lock_bh(&pernet->lock);
 	entry = lookup_by_id ? __lookup_addr_by_id(pernet, local->addr.id) :
 			       __lookup_addr(pernet, &local->addr);
@@ -1996,7 +1997,7 @@ int mptcp_pm_nl_set_flags(struct mptcp_pm_addr_entry *local,
 	*local = *entry;
 	spin_unlock_bh(&pernet->lock);
 
-	mptcp_nl_set_flags(net, &local->addr, bkup, changed);
+	mptcp_nl_set_flags(net, &local->addr, entry->flags, changed);
 	return 0;
 }
 

-- 
2.47.1

On Fri, 17 Jan 2025, Matthieu Baerts (NGI0) wrote:

Hi Matthieu -

> If an entrypoint

Did you mean "endpoint" here?

> has no type -- so not 'subflow', 'signal', 'implicit' --
> there are then no subflows to re-create from this local endpoint.
>
> In this case, there is then no need to iterate over all connections to
> do nothing. So stop early when this case is present.
>
> Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
> ---
> net/mptcp/pm_netlink.c | 15 ++++++++-------
> 1 file changed, 8 insertions(+), 7 deletions(-)
>
> diff --git a/net/mptcp/pm_netlink.c b/net/mptcp/pm_netlink.c
> index ff1e5695dc1db5e32d5f45bef7cf22e43aea0ef1..b1fe2a74fcfe97896de8f9eaee9a1afa5378fabb 100644
> --- a/net/mptcp/pm_netlink.c
> +++ b/net/mptcp/pm_netlink.c
> @@ -1923,11 +1923,16 @@ static void mptcp_pm_nl_fullmesh(struct mptcp_sock *msk,
> }
>
> static void mptcp_nl_set_flags(struct net *net, struct mptcp_addr_info *addr,
> -			       u8 bkup, u8 changed)
> +			       u8 flags, u8 changed)
> {
> +	u8 is_subflow = !!(flags & MPTCP_PM_ADDR_FLAG_SUBFLOW);
> +	u8 bkup = !!(flags & MPTCP_PM_ADDR_FLAG_BACKUP);
> 	long s_slot = 0, s_num = 0;
> 	struct mptcp_sock *msk;
>
> +	if (changed == MPTCP_PM_ADDR_FLAG_FULLMESH && !is_subflow)
> +		return;
> +
> 	while ((msk = mptcp_token_iter_next(net, &s_slot, &s_num)) != NULL) {
> 		struct sock *sk = (struct sock *)msk;
>
> @@ -1937,7 +1942,7 @@ static void mptcp_nl_set_flags(struct net *net, struct mptcp_addr_info *addr,
> 		lock_sock(sk);
> 		if (changed & MPTCP_PM_ADDR_FLAG_BACKUP)
> 			mptcp_pm_nl_mp_prio_send_ack(msk, addr, NULL, bkup);
> -		if (changed & MPTCP_PM_ADDR_FLAG_FULLMESH)
> +		if (is_subflow && (changed & MPTCP_PM_ADDR_FLAG_FULLMESH))

Could you add something to the beginning of the commit message explaining 
this part of the change? It matches the subject line but the body of the 
commit message only explains the loop optimization above.

Thanks,
Mat

> 			mptcp_pm_nl_fullmesh(msk, addr);
> 		release_sock(sk);
>
> @@ -1959,7 +1964,6 @@ int mptcp_pm_nl_set_flags(struct mptcp_pm_addr_entry *local,
> 	struct mptcp_pm_addr_entry *entry;
> 	struct pm_nl_pernet *pernet;
> 	u8 lookup_by_id = 0;
> -	u8 bkup = 0;
>
> 	pernet = pm_nl_get_pernet(net);
>
> @@ -1972,9 +1976,6 @@ int mptcp_pm_nl_set_flags(struct mptcp_pm_addr_entry *local,
> 		}
> 	}
>
> -	if (local->flags & MPTCP_PM_ADDR_FLAG_BACKUP)
> -		bkup = 1;
> -
> 	spin_lock_bh(&pernet->lock);
> 	entry = lookup_by_id ? __lookup_addr_by_id(pernet, local->addr.id) :
> 			       __lookup_addr(pernet, &local->addr);
> @@ -1996,7 +1997,7 @@ int mptcp_pm_nl_set_flags(struct mptcp_pm_addr_entry *local,
> 	*local = *entry;
> 	spin_unlock_bh(&pernet->lock);
>
> -	mptcp_nl_set_flags(net, &local->addr, bkup, changed);
> +	mptcp_nl_set_flags(net, &local->addr, entry->flags, changed);
> 	return 0;
> }
>
>
> -- 
> 2.47.1
>
>
>

Hi Mat,

Thank you for the different reviews!

On 22/01/2025 00:40, Mat Martineau wrote:
> On Fri, 17 Jan 2025, Matthieu Baerts (NGI0) wrote:
> 
> Hi Matthieu -
> 
>> If an entrypoint
> 
> Did you mean "endpoint" here?

Yes indeed, good catch, that's not the first time I mix up the two words :)

>> has no type -- so not 'subflow', 'signal', 'implicit' --
>> there are then no subflows to re-create from this local endpoint.
>>
>> In this case, there is then no need to iterate over all connections to
>> do nothing. So stop early when this case is present.
>>
>> Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
>> ---
>> net/mptcp/pm_netlink.c | 15 ++++++++-------
>> 1 file changed, 8 insertions(+), 7 deletions(-)
>>
>> diff --git a/net/mptcp/pm_netlink.c b/net/mptcp/pm_netlink.c
>> index
>> ff1e5695dc1db5e32d5f45bef7cf22e43aea0ef1..b1fe2a74fcfe97896de8f9eaee9a1afa5378fabb 100644
>> --- a/net/mptcp/pm_netlink.c
>> +++ b/net/mptcp/pm_netlink.c
>> @@ -1923,11 +1923,16 @@ static void mptcp_pm_nl_fullmesh(struct
>> mptcp_sock *msk,
>> }
>>
>> static void mptcp_nl_set_flags(struct net *net, struct mptcp_addr_info
>> *addr,
>> -                   u8 bkup, u8 changed)
>> +                   u8 flags, u8 changed)
>> {
>> +    u8 is_subflow = !!(flags & MPTCP_PM_ADDR_FLAG_SUBFLOW);
>> +    u8 bkup = !!(flags & MPTCP_PM_ADDR_FLAG_BACKUP);
>>     long s_slot = 0, s_num = 0;
>>     struct mptcp_sock *msk;
>>
>> +    if (changed == MPTCP_PM_ADDR_FLAG_FULLMESH && !is_subflow)
>> +        return;
>> +
>>     while ((msk = mptcp_token_iter_next(net, &s_slot, &s_num)) != NULL) {
>>         struct sock *sk = (struct sock *)msk;
>>
>> @@ -1937,7 +1942,7 @@ static void mptcp_nl_set_flags(struct net *net,
>> struct mptcp_addr_info *addr,
>>         lock_sock(sk);
>>         if (changed & MPTCP_PM_ADDR_FLAG_BACKUP)
>>             mptcp_pm_nl_mp_prio_send_ack(msk, addr, NULL, bkup);
>> -        if (changed & MPTCP_PM_ADDR_FLAG_FULLMESH)
>> +        if (is_subflow && (changed & MPTCP_PM_ADDR_FLAG_FULLMESH))
> 
> Could you add something to the beginning of the commit message
> explaining this part of the change? It matches the subject line but the
> body of the commit message only explains the loop optimization above.

Ah yes, I see. The first paragraph was supposed to describe that:

> If an endpoint has no type -- so not 'subflow', 'signal', 'implicit' --
> there are then no subflows to re-create from this local endpoint.

Maybe clearer if I add this comment just above?

  /* Subflows will only be "re-created" if the SUBFLOW flag is set */

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