Re: [PATCH mptcp-next v2 6/7] mptcp: add check_id for lookup_anno_list_by_saddr

[Matthieu Baerts <matttbe@kernel.org>]

Hi Geliang,

On 26/02/2024 11:07, Matthieu Baerts wrote:
> Hi Geliang,
> 
> On 22/02/2024 09:03, Geliang Tang wrote:
>> From: Geliang Tang <tanggeliang@kylinos.cn>
>>
>> This patch adds a new helper mptcp_addresses_equal_check_id() to test the
>> address ids, as well as the address. This can be used to test if the two
>> given addresses are identically equal, they have both the same address and
>> the same address id.
>>
>> Add a new parameter check_id for mptcp_lookup_anno_list_by_saddr(), pass
>> it to mptcp_addresses_equal_check_id(). Then in mptcp_pm_del_add_timer(),
>> the input parameter check_id can be passed as the new parameter into
>> mptcp_lookup_anno_list_by_saddr(). After this, this condition:
>>
>>         (!check_id || entry->addr.id == addr->id)
>>
>> can be dropped, only test if 'entry' is NULL is enough.
> 
> Or use an extra variable? Please see below.
> 
> (...)
> 
>> diff --git a/net/mptcp/pm_netlink.c b/net/mptcp/pm_netlink.c
>> index 5c17d39146ea..7d755c0a32bb 100644
>> --- a/net/mptcp/pm_netlink.c
>> +++ b/net/mptcp/pm_netlink.c
> 
> (...)
> 
>> @@ -324,12 +325,12 @@ mptcp_pm_del_add_timer(struct mptcp_sock *msk,
>>  	struct sock *sk = (struct sock *)msk;
>>  
>>  	spin_lock_bh(&msk->pm.lock);
>> -	entry = mptcp_lookup_anno_list_by_saddr(msk, addr);
>> -	if (entry && (!check_id || entry->addr.id == addr->id))
>> +	entry = mptcp_lookup_anno_list_by_saddr(msk, addr, check_id);
>> +	if (entry)
>>  		entry->retrans_times = ADD_ADDR_RETRANS_MAX;
>>  	spin_unlock_bh(&msk->pm.lock);
>>  
>> -	if (entry && (!check_id || entry->addr.id == addr->id))
>> +	if (entry)
> 
> I probably missed something, but why not only do the modification here
> as suggested in the v1 instead of changing the signature of
> mptcp_lookup_anno_list_by_saddr() and introducing
> mptcp_addresses_equal_check_id() which is indirectly only used once from
> here?
> 
> 
>   bool stop_retrans;
> 
>   (...)
> 
>   spin_lock_bh(&msk->pm.lock);
>   entry = mptcp_lookup_anno_list_by_saddr(msk, addr);
>   stop_retrans = entry && (!check_id || entry->addr.id == addr->id);
>   if (stop_retrans)
>       entry->retrans_times = ADD_ADDR_RETRANS_MAX;
>   spin_unlock_bh(&msk->pm.lock);
> 
>   if (stop_retrans)
>       sk_stop_timer_sync(sk, &entry->add_timer);
> 
> The refactoring would make sense if you have multiple users of this long
> conditions. Here, it looks like it is adding more complexity to get
> functions more generic, with more checks, just to handle one particular
> case that can be extracted here, no?

Regarding this patch that is still tracked on Patchwork, just to know
what to do with it: do you plan to send a new version with the suggested
modification here above or should we just drop the patch from Patchwork?

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