Re: [PATCH mptcp-net v2 3/3] mptcp: pm: change to fullmesh only for 'subflow'

[Matthieu Baerts <matttbe@kernel.org>]

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.