Re: [PATCH mptcp-next 03/10] mptcp: pm: in-kernel: refactor fill_remote_addresses_vec

[Geliang Tang <geliang@kernel.org>]

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)
>  {