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

[Geliang Tang <geliang@kernel.org>]

Hi Matt,

On Thu, 2024-03-28 at 19:27 +0100, Matthieu Baerts wrote:
> 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?

Please drop this patch from Patchwork.

Thanks,
-Geliang

> 
> Cheers,
> Matt