Re: [PATCH mptcp-next 02/10] mptcp: pm: in-kernel: refactor fill_local_addresses_vec

[Matthieu Baerts <matttbe@kernel.org>]

Hi Geliang,

On 19/09/2025 08:51, 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, C flag,
>> any.
> 
> If "C flag" function is squash into patch 1, how about splitting this
> patch into two, one for 'fullmesh' and one for 'any'.

I would prefer not to. I don't think it is really needed to split that
in multiple patches.

(...)

>> +/* Fill all the local addresses into the array addrs[],
>> + * and return the array size.
>> + */
>> +static unsigned int
>> +fill_local_addresses_vec(struct mptcp_sock *msk, struct
>> mptcp_addr_info *remote,
>> +			 struct mptcp_pm_local *locals)
> 
> How about keeping this unchanged:
> 
> static unsigned int fill_local_addresses_vec(struct mptcp_sock *msk,
> 					     struct mptcp_addr_info
> *remote,
> 					     struct mptcp_pm_local
> *locals)
> 
> This can make the patch smaller.

I did these modifications because that's the style I used in the new
functions, plus I think it is better to avoid big indentations.

I don't think we really need to make this patch smaller, it is just a
"pure" refactoring.

> 
>> +{
>> +	bool c_flag_case = remote->id &&
>> mptcp_pm_add_addr_c_flag_case(msk);
>> +	int i;
>> +
>> +	/* If there is at least one MPTCP endpoint with a fullmesh
>> flag */
>> +	i = fill_local_addresses_vec_fullmesh(msk, remote, locals,
>> c_flag_case);
>> +	if (i)
>> +		return i;
>> +
>>  	/* Special case: peer sets the C flag, accept one ADD_ADDR
>> if default
>>  	 * limits are used -- accepting no ADD_ADDR -- and use
>> subflow endpoints
>>  	 */
>> -	if (!i && c_flag_case) {
>> -		unsigned int local_addr_max =
>> mptcp_pm_get_local_addr_max(msk);
>> +	if (c_flag_case)
>> +		return fill_local_addresses_vec_c_flag(msk, remote,
>> locals);
>>  
>> -		while (msk->pm.local_addr_used < local_addr_max &&
>> -		       msk->pm.subflows < subflows_max) {
>> -			struct mptcp_pm_local *local = &locals[i];
>> -
>> -			if (!select_local_address(pernet, msk,
>> local))
>> -				break;
>> -
>> -			__clear_bit(local->addr.id, msk-
>>> pm.id_avail_bitmap);
>> -
>> -			if (!mptcp_pm_addr_families_match(sk,
>> &local->addr,
>> -							  remote))
>> -				continue;
>> -
>> -			if (mptcp_addresses_equal(&local->addr,
>> &mpc_addr,
>> -						  local->addr.port))
>> -				continue;
>> -
>> -			msk->pm.local_addr_used++;
>> -			msk->pm.subflows++;
>> -			i++;
>> -		}
>> -
>> -		return i;
>> -	}
>> -
>> -	/* If the array is empty, fill in the single
>> -	 * 'IPADDRANY' local address
>> -	 */
>> -	if (!i) {
>> -		memset(&locals[i], 0, sizeof(locals[i]));
>> -		locals[i].addr.family =
>> -#if IS_ENABLED(CONFIG_MPTCP_IPV6)
>> -			       remote->family == AF_INET6 &&
>> -			       ipv6_addr_v4mapped(&remote->addr6) ?
>> AF_INET :
>> -#endif
>> -			       remote->family;
>> -
>> -		if (!mptcp_pm_addr_families_match(sk,
>> &locals[i].addr, remote))
>> -			return 0;
>> -
>> -		msk->pm.subflows++;
>> -		i++;
>> -	}
>> -
>> -	return i;
>> +	/* No special case: fill in the single 'IPADDRANY' local
>> address */
>> +	return fill_local_address_any(msk, remote, &locals[0]);
>>  }
>>  
>>  static void mptcp_pm_nl_add_addr_received(struct mptcp_sock *msk)
>> diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h
>> index
>> dd0662defd41c84474e44c559c571e3594b85d9e..0d6dae37c9daf4ec8990b9a8703
>> 6aa393add585c 100644
>> --- a/net/mptcp/protocol.h
>> +++ b/net/mptcp/protocol.h
>> @@ -1200,7 +1200,8 @@ static inline bool
>> mptcp_pm_add_addr_c_flag_case(struct mptcp_sock *msk)
>>  {
>>  	return READ_ONCE(msk->pm.remote_deny_join_id0) &&
>>  	       msk->pm.local_addr_used == 0 &&
>> -	       mptcp_pm_get_add_addr_accept_max(msk) == 0;
>> +	       mptcp_pm_get_add_addr_accept_max(msk) == 0 &&
>> +	       msk->pm.subflows < mptcp_pm_get_subflows_max(msk);
> 
> This needs to be squash to patch 1 too.

It is currently not needed, see this message in the commit message:

> To remove one level of indentation, msk->pm.subflows >= subflows_max is
> now checked upfront.
But I guess it doesn't hurt to move it there, just to be on the safe
side, and avoid this note.

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