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 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?

>  		sk_stop_timer_sync(sk, &entry->add_timer);
>  
>  	return entry;
(...)

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