| Message ID | 608dcc5649b212dc1a37c72fa7b465d3e8619316.1708588977.git.tanggeliang@kylinos.cn (mailing list archive) |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | Matthieu Baerts |
| Headers | show |
| Series | some cleanups | expand |
| Context | Check | Description |
|---|---|---|
| matttbe/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 70 lines checked |
| matttbe/build | success | Build and static analysis OK |
| matttbe/KVM_Validation__normal | success | Success! ✅ |
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
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
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
diff --git a/net/mptcp/pm.c b/net/mptcp/pm.c index 55406720c607..2f5ccafe7e34 100644 --- a/net/mptcp/pm.c +++ b/net/mptcp/pm.c @@ -257,7 +257,7 @@ void mptcp_pm_add_addr_echoed(struct mptcp_sock *msk, spin_lock_bh(&pm->lock); - if (mptcp_lookup_anno_list_by_saddr(msk, addr) && READ_ONCE(pm->work_pending)) + if (mptcp_lookup_anno_list_by_saddr(msk, addr, false) && READ_ONCE(pm->work_pending)) mptcp_pm_schedule_work(msk, MPTCP_PM_SUBFLOW_ESTABLISHED); spin_unlock_bh(&pm->lock); 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 @@ -237,14 +237,15 @@ bool mptcp_pm_nl_check_work_pending(struct mptcp_sock *msk) struct mptcp_pm_add_entry * mptcp_lookup_anno_list_by_saddr(const struct mptcp_sock *msk, - const struct mptcp_addr_info *addr) + const struct mptcp_addr_info *addr, + bool check_id) { struct mptcp_pm_add_entry *entry; lockdep_assert_held(&msk->pm.lock); list_for_each_entry(entry, &msk->pm.anno_list, list) { - if (mptcp_addresses_equal(&entry->addr, addr, true)) + if (mptcp_addresses_equal_check_id(&entry->addr, addr, true, check_id)) return entry; } @@ -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) sk_stop_timer_sync(sk, &entry->add_timer); return entry; @@ -344,7 +345,7 @@ bool mptcp_pm_alloc_anno_list(struct mptcp_sock *msk, lockdep_assert_held(&msk->pm.lock); - add_entry = mptcp_lookup_anno_list_by_saddr(msk, addr); + add_entry = mptcp_lookup_anno_list_by_saddr(msk, addr, false); if (add_entry) { if (mptcp_pm_is_kernel(msk)) diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h index 7905783c95e4..130d4c55e10a 100644 --- a/net/mptcp/protocol.h +++ b/net/mptcp/protocol.h @@ -673,6 +673,13 @@ bool mptcp_addresses_equal(const struct mptcp_addr_info *a, const struct mptcp_addr_info *b, bool use_port); void mptcp_local_address(const struct sock_common *skc, struct mptcp_addr_info *addr); +static inline bool mptcp_addresses_equal_check_id(const struct mptcp_addr_info *a, + const struct mptcp_addr_info *b, + bool use_port, bool check_id) +{ + return mptcp_addresses_equal(a, b, use_port) ? a->id == b->id : false; +} + /* called with sk socket lock held */ int __mptcp_subflow_connect(struct sock *sk, const struct mptcp_addr_info *loc, const struct mptcp_addr_info *remote); @@ -966,7 +973,8 @@ mptcp_pm_del_add_timer(struct mptcp_sock *msk, const struct mptcp_addr_info *addr, bool check_id); struct mptcp_pm_add_entry * mptcp_lookup_anno_list_by_saddr(const struct mptcp_sock *msk, - const struct mptcp_addr_info *addr); + const struct mptcp_addr_info *addr, + bool check_id); int mptcp_pm_get_flags_and_ifindex_by_id(struct mptcp_sock *msk, unsigned int id, u8 *flags, int *ifindex);