Before this modification, this function was quite long with many levels
of indentations.

Each case can be split in a dedicated function: fullmesh, non-fullmesh.

To remove one level of indentation, msk->pm.subflows >= subflows_max is
now checked after having added one subflow, and stops the loop if it is
no longer possible to add new subflows. This is fine to do this because
this function should only be called if msk->pm.subflows < subflows_max.

No functional changes intended.

Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
---
 net/mptcp/pm_kernel.c | 112 ++++++++++++++++++++++++++++----------------------
 1 file changed, 63 insertions(+), 49 deletions(-)

diff --git a/net/mptcp/pm_kernel.c b/net/mptcp/pm_kernel.c
index 5bdcfcc26308841c49375ce35205097f30592279..8d5df9b98589e5cd69f16dc54e9140e88a1835e2 100644
--- a/net/mptcp/pm_kernel.c
+++ b/net/mptcp/pm_kernel.c
@@ -159,74 +159,88 @@ select_signal_address(struct pm_nl_pernet *pernet, const struct mptcp_sock *msk,
 	return found;
 }
 
-/* Fill all the remote addresses into the array addrs[],
- * and return the array size.
- */
-static unsigned int fill_remote_addresses_vec(struct mptcp_sock *msk,
-					      struct mptcp_addr_info *local,
-					      bool fullmesh,
-					      struct mptcp_addr_info *addrs)
+static unsigned int
+fill_remote_addr(struct mptcp_sock *msk, struct mptcp_addr_info *local,
+		 struct mptcp_addr_info *addrs)
 {
 	bool deny_id0 = READ_ONCE(msk->pm.remote_deny_join_id0);
+	struct sock *sk = (struct sock *)msk;
+	struct mptcp_addr_info remote = { 0 };
+
+	if (deny_id0)
+		return 0;
+
+	mptcp_remote_address((struct sock_common *)sk, &remote);
+
+	if (!mptcp_pm_addr_families_match(sk, local, &remote))
+		return 0;
+
+	msk->pm.subflows++;
+	*addrs = remote;
+
+	return 1;
+}
+
+static unsigned int
+fill_remote_addresses_fullmesh(struct mptcp_sock *msk,
+			       struct mptcp_addr_info *local,
+			       struct mptcp_addr_info *addrs)
+{
+	bool deny_id0 = READ_ONCE(msk->pm.remote_deny_join_id0);
+	DECLARE_BITMAP(unavail_id, MPTCP_PM_MAX_ADDR_ID + 1);
 	struct sock *sk = (struct sock *)msk, *ssk;
 	struct mptcp_subflow_context *subflow;
-	struct mptcp_addr_info remote = { 0 };
 	unsigned int subflows_max;
 	int i = 0;
 
 	subflows_max = mptcp_pm_get_subflows_max(msk);
-	mptcp_remote_address((struct sock_common *)sk, &remote);
 
-	/* Non-fullmesh endpoint, fill in the single entry
-	 * corresponding to the primary MPC subflow remote address
+	/* Forbid creation of new subflows matching existing ones, possibly
+	 * already created by incoming ADD_ADDR
 	 */
-	if (!fullmesh) {
-		if (deny_id0)
-			return 0;
+	bitmap_zero(unavail_id, MPTCP_PM_MAX_ADDR_ID + 1);
+	mptcp_for_each_subflow(msk, subflow)
+		if (READ_ONCE(subflow->local_id) == local->id)
+			__set_bit(subflow->remote_id, unavail_id);
 
-		if (!mptcp_pm_addr_families_match(sk, local, &remote))
-			return 0;
+	mptcp_for_each_subflow(msk, subflow) {
+		ssk = mptcp_subflow_tcp_sock(subflow);
+		mptcp_remote_address((struct sock_common *)ssk, &addrs[i]);
+		addrs[i].id = READ_ONCE(subflow->remote_id);
+		if (deny_id0 && !addrs[i].id)
+			continue;
 
+		if (test_bit(addrs[i].id, unavail_id))
+			continue;
+
+		if (!mptcp_pm_addr_families_match(sk, local, &addrs[i]))
+			continue;
+
+		/* forbid creating multiple address towards this id */
+		__set_bit(addrs[i].id, unavail_id);
 		msk->pm.subflows++;
-		addrs[i++] = remote;
-	} else {
-		DECLARE_BITMAP(unavail_id, MPTCP_PM_MAX_ADDR_ID + 1);
+		i++;
 
-		/* Forbid creation of new subflows matching existing
-		 * ones, possibly already created by incoming ADD_ADDR
-		 */
-		bitmap_zero(unavail_id, MPTCP_PM_MAX_ADDR_ID + 1);
-		mptcp_for_each_subflow(msk, subflow)
-			if (READ_ONCE(subflow->local_id) == local->id)
-				__set_bit(subflow->remote_id, unavail_id);
-
-		mptcp_for_each_subflow(msk, subflow) {
-			ssk = mptcp_subflow_tcp_sock(subflow);
-			mptcp_remote_address((struct sock_common *)ssk, &addrs[i]);
-			addrs[i].id = READ_ONCE(subflow->remote_id);
-			if (deny_id0 && !addrs[i].id)
-				continue;
-
-			if (test_bit(addrs[i].id, unavail_id))
-				continue;
-
-			if (!mptcp_pm_addr_families_match(sk, local, &addrs[i]))
-				continue;
-
-			if (msk->pm.subflows < subflows_max) {
-				/* forbid creating multiple address towards
-				 * this id
-				 */
-				__set_bit(addrs[i].id, unavail_id);
-				msk->pm.subflows++;
-				i++;
-			}
-		}
+		if (msk->pm.subflows >= subflows_max)
+			break;
 	}
 
 	return i;
 }
 
+/* Fill all the remote addresses into the array addrs[],
+ * and return the array size.
+ */
+static unsigned int
+fill_remote_addresses_vec(struct mptcp_sock *msk, struct mptcp_addr_info *local,
+			  bool fullmesh, struct mptcp_addr_info *addrs)
+{
+	if (fullmesh)
+		return fill_remote_addresses_fullmesh(msk, local, addrs);
+
+	return fill_remote_addr(msk, local, addrs);
+}
+
 static struct mptcp_pm_addr_entry *
 __lookup_addr_by_id(struct pm_nl_pernet *pernet, unsigned int id)
 {

-- 
2.51.0

Hi Matt,

On Thu, 2025-09-18 at 19:42 +0200, Matthieu Baerts (NGI0) wrote:
> Before this modification, this function was quite long with many
> levels
> of indentations.
> 
> Each case can be split in a dedicated function: fullmesh, non-
> fullmesh.

How about splitting this patch into two, one for 'fullmesh', one for
'non-fullmesh'.

> 
> To remove one level of indentation, msk->pm.subflows >= subflows_max
> is
> now checked after having added one subflow, and stops the loop if it
> is
> no longer possible to add new subflows. This is fine to do this
> because
> this function should only be called if msk->pm.subflows <
> subflows_max.
> 
> No functional changes intended.
> 
> Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
> ---
>  net/mptcp/pm_kernel.c | 112 ++++++++++++++++++++++++++++------------
> ----------
>  1 file changed, 63 insertions(+), 49 deletions(-)
> 
> diff --git a/net/mptcp/pm_kernel.c b/net/mptcp/pm_kernel.c
> index
> 5bdcfcc26308841c49375ce35205097f30592279..8d5df9b98589e5cd69f16dc54e9
> 140e88a1835e2 100644
> --- a/net/mptcp/pm_kernel.c
> +++ b/net/mptcp/pm_kernel.c
> @@ -159,74 +159,88 @@ select_signal_address(struct pm_nl_pernet
> *pernet, const struct mptcp_sock *msk,
>  	return found;
>  }
>  
> -/* Fill all the remote addresses into the array addrs[],
> - * and return the array size.
> - */
> -static unsigned int fill_remote_addresses_vec(struct mptcp_sock
> *msk,
> -					      struct mptcp_addr_info
> *local,
> -					      bool fullmesh,
> -					      struct mptcp_addr_info
> *addrs)
> +static unsigned int
> +fill_remote_addr(struct mptcp_sock *msk, struct mptcp_addr_info
> *local,
> +		 struct mptcp_addr_info *addrs)
>  {
>  	bool deny_id0 = READ_ONCE(msk->pm.remote_deny_join_id0);
> +	struct sock *sk = (struct sock *)msk;
> +	struct mptcp_addr_info remote = { 0 };

This breaks 'Reverse X-Mas Tree' order.

> +
> +	if (deny_id0)
> +		return 0;
> +
> +	mptcp_remote_address((struct sock_common *)sk, &remote);
> +
> +	if (!mptcp_pm_addr_families_match(sk, local, &remote))
> +		return 0;
> +
> +	msk->pm.subflows++;
> +	*addrs = remote;
> +
> +	return 1;

How about still using:

addrs[i++] = remote;
return i;

here, to keep it consistent with other fill_*_addr helpers.

> +}
> +
> +static unsigned int
> +fill_remote_addresses_fullmesh(struct mptcp_sock *msk,
> +			       struct mptcp_addr_info *local,
> +			       struct mptcp_addr_info *addrs)
> +{
> +	bool deny_id0 = READ_ONCE(msk->pm.remote_deny_join_id0);
> +	DECLARE_BITMAP(unavail_id, MPTCP_PM_MAX_ADDR_ID + 1);
>  	struct sock *sk = (struct sock *)msk, *ssk;
>  	struct mptcp_subflow_context *subflow;
> -	struct mptcp_addr_info remote = { 0 };
>  	unsigned int subflows_max;
>  	int i = 0;
>  
>  	subflows_max = mptcp_pm_get_subflows_max(msk);
> -	mptcp_remote_address((struct sock_common *)sk, &remote);
>  
> -	/* Non-fullmesh endpoint, fill in the single entry
> -	 * corresponding to the primary MPC subflow remote address
> +	/* Forbid creation of new subflows matching existing ones,
> possibly
> +	 * already created by incoming ADD_ADDR
>  	 */
> -	if (!fullmesh) {
> -		if (deny_id0)
> -			return 0;
> +	bitmap_zero(unavail_id, MPTCP_PM_MAX_ADDR_ID + 1);
> +	mptcp_for_each_subflow(msk, subflow)
> +		if (READ_ONCE(subflow->local_id) == local->id)
> +			__set_bit(subflow->remote_id, unavail_id);
>  
> -		if (!mptcp_pm_addr_families_match(sk, local,
> &remote))
> -			return 0;
> +	mptcp_for_each_subflow(msk, subflow) {
> +		ssk = mptcp_subflow_tcp_sock(subflow);
> +		mptcp_remote_address((struct sock_common *)ssk,
> &addrs[i]);
> +		addrs[i].id = READ_ONCE(subflow->remote_id);
> +		if (deny_id0 && !addrs[i].id)
> +			continue;
>  
> +		if (test_bit(addrs[i].id, unavail_id))
> +			continue;
> +
> +		if (!mptcp_pm_addr_families_match(sk, local,
> &addrs[i]))
> +			continue;
> +
> +		/* forbid creating multiple address towards this id
> */
> +		__set_bit(addrs[i].id, unavail_id);
>  		msk->pm.subflows++;
> -		addrs[i++] = remote;
> -	} else {
> -		DECLARE_BITMAP(unavail_id, MPTCP_PM_MAX_ADDR_ID +
> 1);
> +		i++;
>  
> -		/* Forbid creation of new subflows matching existing
> -		 * ones, possibly already created by incoming
> ADD_ADDR
> -		 */
> -		bitmap_zero(unavail_id, MPTCP_PM_MAX_ADDR_ID + 1);
> -		mptcp_for_each_subflow(msk, subflow)
> -			if (READ_ONCE(subflow->local_id) == local-
> >id)
> -				__set_bit(subflow->remote_id,
> unavail_id);
> -
> -		mptcp_for_each_subflow(msk, subflow) {
> -			ssk = mptcp_subflow_tcp_sock(subflow);
> -			mptcp_remote_address((struct sock_common
> *)ssk, &addrs[i]);
> -			addrs[i].id = READ_ONCE(subflow->remote_id);
> -			if (deny_id0 && !addrs[i].id)
> -				continue;
> -
> -			if (test_bit(addrs[i].id, unavail_id))
> -				continue;
> -
> -			if (!mptcp_pm_addr_families_match(sk, local,
> &addrs[i]))
> -				continue;
> -
> -			if (msk->pm.subflows < subflows_max) {
> -				/* forbid creating multiple address
> towards
> -				 * this id
> -				 */
> -				__set_bit(addrs[i].id, unavail_id);
> -				msk->pm.subflows++;
> -				i++;
> -			}
> -		}
> +		if (msk->pm.subflows >= subflows_max)
> +			break;
>  	}
>  
>  	return i;
>  }
>  
> +/* Fill all the remote addresses into the array addrs[],
> + * and return the array size.
> + */
> +static unsigned int
> +fill_remote_addresses_vec(struct mptcp_sock *msk, struct
> mptcp_addr_info *local,
> +			  bool fullmesh, struct mptcp_addr_info
> *addrs)

How about keeping this unchanged:

static unsigned int fill_remote_addresses_vec(struct mptcp_sock
*msk,
					      struct mptcp_addr_info
*local,
					      bool fullmesh,
					      struct mptcp_addr_info
*addrs)

This can make the patch smaller.

> +{

-	/* Non-fullmesh endpoint, fill in the single entry
-	 * corresponding to the primary MPC subflow remote address

We can keep this comment here.

-	if (!fullmesh) {

... and test "!fullmesh" just like the original code.

Thanks,
-Geliang

> +	if (fullmesh)
> +		return fill_remote_addresses_fullmesh(msk, local,
> addrs);
> +
> +	return fill_remote_addr(msk, local, addrs);
> +}
> +
>  static struct mptcp_pm_addr_entry *
>  __lookup_addr_by_id(struct pm_nl_pernet *pernet, unsigned int id)
>  {

Hi Geliang,

On 19/09/2025 08:52, Geliang Tang wrote:
> Hi Matt,
> 
> On Thu, 2025-09-18 at 19:42 +0200, Matthieu Baerts (NGI0) wrote:
>> Before this modification, this function was quite long with many
>> levels
>> of indentations.
>>
>> Each case can be split in a dedicated function: fullmesh, non-
>> fullmesh.
> 
> How about splitting this patch into two, one for 'fullmesh', one for
> 'non-fullmesh'.

I don't think that's really needed: I'm simply moving what is in the
if-statement in one helper, and the rest in another one. Or do you think
it would really help?

> 
>>
>> To remove one level of indentation, msk->pm.subflows >= subflows_max
>> is
>> now checked after having added one subflow, and stops the loop if it
>> is
>> no longer possible to add new subflows. This is fine to do this
>> because
>> this function should only be called if msk->pm.subflows <
>> subflows_max.
>>
>> No functional changes intended.
>>
>> Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
>> ---
>>  net/mptcp/pm_kernel.c | 112 ++++++++++++++++++++++++++++------------
>> ----------
>>  1 file changed, 63 insertions(+), 49 deletions(-)
>>
>> diff --git a/net/mptcp/pm_kernel.c b/net/mptcp/pm_kernel.c
>> index
>> 5bdcfcc26308841c49375ce35205097f30592279..8d5df9b98589e5cd69f16dc54e9
>> 140e88a1835e2 100644
>> --- a/net/mptcp/pm_kernel.c
>> +++ b/net/mptcp/pm_kernel.c
>> @@ -159,74 +159,88 @@ select_signal_address(struct pm_nl_pernet
>> *pernet, const struct mptcp_sock *msk,
>>  	return found;
>>  }
>>  
>> -/* Fill all the remote addresses into the array addrs[],
>> - * and return the array size.
>> - */
>> -static unsigned int fill_remote_addresses_vec(struct mptcp_sock
>> *msk,
>> -					      struct mptcp_addr_info
>> *local,
>> -					      bool fullmesh,
>> -					      struct mptcp_addr_info
>> *addrs)
>> +static unsigned int
>> +fill_remote_addr(struct mptcp_sock *msk, struct mptcp_addr_info
>> *local,
>> +		 struct mptcp_addr_info *addrs)
>>  {
>>  	bool deny_id0 = READ_ONCE(msk->pm.remote_deny_join_id0);
>> +	struct sock *sk = (struct sock *)msk;
>> +	struct mptcp_addr_info remote = { 0 };
> 
> This breaks 'Reverse X-Mas Tree' order.

Good catch, fixed!

> 
>> +
>> +	if (deny_id0)
>> +		return 0;
>> +
>> +	mptcp_remote_address((struct sock_common *)sk, &remote);
>> +
>> +	if (!mptcp_pm_addr_families_match(sk, local, &remote))
>> +		return 0;
>> +
>> +	msk->pm.subflows++;
>> +	*addrs = remote;
>> +
>> +	return 1;
> 
> How about still using:
> 
> addrs[i++] = remote;
> return i;
> 
> here, to keep it consistent with other fill_*_addr helpers.

We will only fill one address, then no need to declare 'i', no?
It looks clearer to me like that, so we clearly understand only one
address is filled, no?

> 
>> +}
>> +
>> +static unsigned int
>> +fill_remote_addresses_fullmesh(struct mptcp_sock *msk,
>> +			       struct mptcp_addr_info *local,
>> +			       struct mptcp_addr_info *addrs)
>> +{
>> +	bool deny_id0 = READ_ONCE(msk->pm.remote_deny_join_id0);
>> +	DECLARE_BITMAP(unavail_id, MPTCP_PM_MAX_ADDR_ID + 1);
>>  	struct sock *sk = (struct sock *)msk, *ssk;
>>  	struct mptcp_subflow_context *subflow;
>> -	struct mptcp_addr_info remote = { 0 };
>>  	unsigned int subflows_max;
>>  	int i = 0;
>>  
>>  	subflows_max = mptcp_pm_get_subflows_max(msk);
>> -	mptcp_remote_address((struct sock_common *)sk, &remote);
>>  
>> -	/* Non-fullmesh endpoint, fill in the single entry
>> -	 * corresponding to the primary MPC subflow remote address
>> +	/* Forbid creation of new subflows matching existing ones,
>> possibly
>> +	 * already created by incoming ADD_ADDR
>>  	 */
>> -	if (!fullmesh) {
>> -		if (deny_id0)
>> -			return 0;
>> +	bitmap_zero(unavail_id, MPTCP_PM_MAX_ADDR_ID + 1);
>> +	mptcp_for_each_subflow(msk, subflow)
>> +		if (READ_ONCE(subflow->local_id) == local->id)
>> +			__set_bit(subflow->remote_id, unavail_id);
>>  
>> -		if (!mptcp_pm_addr_families_match(sk, local,
>> &remote))
>> -			return 0;
>> +	mptcp_for_each_subflow(msk, subflow) {
>> +		ssk = mptcp_subflow_tcp_sock(subflow);
>> +		mptcp_remote_address((struct sock_common *)ssk,
>> &addrs[i]);
>> +		addrs[i].id = READ_ONCE(subflow->remote_id);
>> +		if (deny_id0 && !addrs[i].id)
>> +			continue;
>>  
>> +		if (test_bit(addrs[i].id, unavail_id))
>> +			continue;
>> +
>> +		if (!mptcp_pm_addr_families_match(sk, local,
>> &addrs[i]))
>> +			continue;
>> +
>> +		/* forbid creating multiple address towards this id
>> */
>> +		__set_bit(addrs[i].id, unavail_id);
>>  		msk->pm.subflows++;
>> -		addrs[i++] = remote;
>> -	} else {
>> -		DECLARE_BITMAP(unavail_id, MPTCP_PM_MAX_ADDR_ID +
>> 1);
>> +		i++;
>>  
>> -		/* Forbid creation of new subflows matching existing
>> -		 * ones, possibly already created by incoming
>> ADD_ADDR
>> -		 */
>> -		bitmap_zero(unavail_id, MPTCP_PM_MAX_ADDR_ID + 1);
>> -		mptcp_for_each_subflow(msk, subflow)
>> -			if (READ_ONCE(subflow->local_id) == local-
>>> id)
>> -				__set_bit(subflow->remote_id,
>> unavail_id);
>> -
>> -		mptcp_for_each_subflow(msk, subflow) {
>> -			ssk = mptcp_subflow_tcp_sock(subflow);
>> -			mptcp_remote_address((struct sock_common
>> *)ssk, &addrs[i]);
>> -			addrs[i].id = READ_ONCE(subflow->remote_id);
>> -			if (deny_id0 && !addrs[i].id)
>> -				continue;
>> -
>> -			if (test_bit(addrs[i].id, unavail_id))
>> -				continue;
>> -
>> -			if (!mptcp_pm_addr_families_match(sk, local,
>> &addrs[i]))
>> -				continue;
>> -
>> -			if (msk->pm.subflows < subflows_max) {
>> -				/* forbid creating multiple address
>> towards
>> -				 * this id
>> -				 */
>> -				__set_bit(addrs[i].id, unavail_id);
>> -				msk->pm.subflows++;
>> -				i++;
>> -			}
>> -		}
>> +		if (msk->pm.subflows >= subflows_max)
>> +			break;
>>  	}
>>  
>>  	return i;
>>  }
>>  
>> +/* Fill all the remote addresses into the array addrs[],
>> + * and return the array size.
>> + */
>> +static unsigned int
>> +fill_remote_addresses_vec(struct mptcp_sock *msk, struct
>> mptcp_addr_info *local,
>> +			  bool fullmesh, struct mptcp_addr_info
>> *addrs)
> 
> How about keeping this unchanged:
> 
> static unsigned int fill_remote_addresses_vec(struct mptcp_sock
> *msk,
> 					      struct mptcp_addr_info
> *local,
> 					      bool fullmesh,
> 					      struct mptcp_addr_info
> *addrs)
> 
> This can make the patch smaller.

I don't think that's needed, for the same reason.

> 
>> +{
> 
> -	/* Non-fullmesh endpoint, fill in the single entry
> -	 * corresponding to the primary MPC subflow remote address
> 
> We can keep this comment here.

Indeed.

> 
> -	if (!fullmesh) {
> 
> ... and test "!fullmesh" just like the original code.

It looks "strange" to treat the opposite condition first, but if I leave
the comment, I can keep '!fullmesh' I guess.

> 
> Thanks,
> -Geliang
> 
>> +	if (fullmesh)
>> +		return fill_remote_addresses_fullmesh(msk, local,
>> addrs);
>> +
>> +	return fill_remote_addr(msk, local, addrs);
>> +}
>> +
>>  static struct mptcp_pm_addr_entry *
>>  __lookup_addr_by_id(struct pm_nl_pernet *pernet, unsigned int id)
>>  {

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