From: Geliang Tang <tanggeliang@kylinos.cn>
The helper __lookup_addr_rcu() can be used in mptcp_pm_nl_get_local_id()
and mptcp_pm_nl_is_backup() to simplify the code, and avoid code
duplication.
Co-developed-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
Signed-off-by: Geliang Tang <tanggeliang@kylinos.cn>
---
Notes:
- This is also for -next.
---
net/mptcp/pm_netlink.c | 20 ++++++--------------
1 file changed, 6 insertions(+), 14 deletions(-)
diff --git a/net/mptcp/pm_netlink.c b/net/mptcp/pm_netlink.c
index f38e1ccd34e95cd88b179a8b50e6965731542871..7c6e664b236d1659a554d003c78c72ec91895ba5 100644
--- a/net/mptcp/pm_netlink.c
+++ b/net/mptcp/pm_netlink.c
@@ -1154,17 +1154,13 @@ int mptcp_pm_nl_get_local_id(struct mptcp_sock *msk, struct mptcp_addr_info *skc
{
struct mptcp_pm_addr_entry *entry;
struct pm_nl_pernet *pernet;
- int ret = -1;
+ int ret;
pernet = pm_nl_get_pernet_from_msk(msk);
rcu_read_lock();
- list_for_each_entry_rcu(entry, &pernet->local_addr_list, list) {
- if (mptcp_addresses_equal(&entry->addr, skc, entry->addr.port)) {
- ret = entry->addr.id;
- break;
- }
- }
+ entry = __lookup_addr_rcu(pernet, skc);
+ ret = entry ? entry->addr.id : -1;
rcu_read_unlock();
if (ret >= 0)
return ret;
@@ -1191,15 +1187,11 @@ bool mptcp_pm_nl_is_backup(struct mptcp_sock *msk, struct mptcp_addr_info *skc)
{
struct pm_nl_pernet *pernet = pm_nl_get_pernet_from_msk(msk);
struct mptcp_pm_addr_entry *entry;
- bool backup = false;
+ bool backup;
rcu_read_lock();
- list_for_each_entry_rcu(entry, &pernet->local_addr_list, list) {
- if (mptcp_addresses_equal(&entry->addr, skc, entry->addr.port)) {
- backup = !!(entry->flags & MPTCP_PM_ADDR_FLAG_BACKUP);
- break;
- }
- }
+ entry = __lookup_addr_rcu(pernet, skc);
+ backup = entry && !!(entry->flags & MPTCP_PM_ADDR_FLAG_BACKUP);
rcu_read_unlock();
return backup;
--
2.45.2
Hi Matt, Thanks for this patch. On Fri, 2024-10-25 at 11:32 +0200, Matthieu Baerts (NGI0) wrote: > From: Geliang Tang <tanggeliang@kylinos.cn> > > The helper __lookup_addr_rcu() can be used in > mptcp_pm_nl_get_local_id() > and mptcp_pm_nl_is_backup() to simplify the code, and avoid code > duplication. > > Co-developed-by: Matthieu Baerts (NGI0) <matttbe@kernel.org> > Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org> > Signed-off-by: Geliang Tang <tanggeliang@kylinos.cn> > --- > Notes: > - This is also for -next. > --- > net/mptcp/pm_netlink.c | 20 ++++++-------------- > 1 file changed, 6 insertions(+), 14 deletions(-) > > diff --git a/net/mptcp/pm_netlink.c b/net/mptcp/pm_netlink.c > index > f38e1ccd34e95cd88b179a8b50e6965731542871..7c6e664b236d1659a554d003c78 > c72ec91895ba5 100644 > --- a/net/mptcp/pm_netlink.c > +++ b/net/mptcp/pm_netlink.c > @@ -1154,17 +1154,13 @@ int mptcp_pm_nl_get_local_id(struct > mptcp_sock *msk, struct mptcp_addr_info *skc > { > struct mptcp_pm_addr_entry *entry; > struct pm_nl_pernet *pernet; > - int ret = -1; > + int ret; > > pernet = pm_nl_get_pernet_from_msk(msk); > > rcu_read_lock(); > - list_for_each_entry_rcu(entry, &pernet->local_addr_list, > list) { > - if (mptcp_addresses_equal(&entry->addr, skc, entry- > >addr.port)) { > - ret = entry->addr.id; > - break; > - } > - } > + entry = __lookup_addr_rcu(pernet, skc); > + ret = entry ? entry->addr.id : -1; > rcu_read_unlock(); > if (ret >= 0) > return ret; > @@ -1191,15 +1187,11 @@ bool mptcp_pm_nl_is_backup(struct mptcp_sock > *msk, struct mptcp_addr_info *skc) > { > struct pm_nl_pernet *pernet = > pm_nl_get_pernet_from_msk(msk); > struct mptcp_pm_addr_entry *entry; > - bool backup = false; > + bool backup; > > rcu_read_lock(); > - list_for_each_entry_rcu(entry, &pernet->local_addr_list, > list) { > - if (mptcp_addresses_equal(&entry->addr, skc, entry- > >addr.port)) { > - backup = !!(entry->flags & > MPTCP_PM_ADDR_FLAG_BACKUP); > - break; > - } > - } > + entry = __lookup_addr_rcu(pernet, skc); > + backup = entry && !!(entry->flags & > MPTCP_PM_ADDR_FLAG_BACKUP); I think we should check whether entry is NULL here too. No? -Geliang > rcu_read_unlock(); > > return backup; >
On Fri, 2024-10-25 at 18:37 +0800, Geliang Tang wrote: > Hi Matt, > > Thanks for this patch. > > On Fri, 2024-10-25 at 11:32 +0200, Matthieu Baerts (NGI0) wrote: > > From: Geliang Tang <tanggeliang@kylinos.cn> > > > > The helper __lookup_addr_rcu() can be used in > > mptcp_pm_nl_get_local_id() > > and mptcp_pm_nl_is_backup() to simplify the code, and avoid code > > duplication. > > > > Co-developed-by: Matthieu Baerts (NGI0) <matttbe@kernel.org> > > Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org> > > Signed-off-by: Geliang Tang <tanggeliang@kylinos.cn> > > --- > > Notes: > > - This is also for -next. > > --- > > net/mptcp/pm_netlink.c | 20 ++++++-------------- > > 1 file changed, 6 insertions(+), 14 deletions(-) > > > > diff --git a/net/mptcp/pm_netlink.c b/net/mptcp/pm_netlink.c > > index > > f38e1ccd34e95cd88b179a8b50e6965731542871..7c6e664b236d1659a554d003c > > 78 > > c72ec91895ba5 100644 > > --- a/net/mptcp/pm_netlink.c > > +++ b/net/mptcp/pm_netlink.c > > @@ -1154,17 +1154,13 @@ int mptcp_pm_nl_get_local_id(struct > > mptcp_sock *msk, struct mptcp_addr_info *skc > > { > > struct mptcp_pm_addr_entry *entry; > > struct pm_nl_pernet *pernet; > > - int ret = -1; > > + int ret; > > > > pernet = pm_nl_get_pernet_from_msk(msk); > > > > rcu_read_lock(); > > - list_for_each_entry_rcu(entry, &pernet->local_addr_list, > > list) { > > - if (mptcp_addresses_equal(&entry->addr, skc, > > entry- > > > addr.port)) { > > - ret = entry->addr.id; > > - break; > > - } > > - } > > + entry = __lookup_addr_rcu(pernet, skc); > > + ret = entry ? entry->addr.id : -1; > > rcu_read_unlock(); > > if (ret >= 0) > > return ret; > > @@ -1191,15 +1187,11 @@ bool mptcp_pm_nl_is_backup(struct > > mptcp_sock > > *msk, struct mptcp_addr_info *skc) > > { > > struct pm_nl_pernet *pernet = > > pm_nl_get_pernet_from_msk(msk); > > struct mptcp_pm_addr_entry *entry; > > - bool backup = false; > > + bool backup; > > > > rcu_read_lock(); > > - list_for_each_entry_rcu(entry, &pernet->local_addr_list, > > list) { > > - if (mptcp_addresses_equal(&entry->addr, skc, > > entry- > > > addr.port)) { > > - backup = !!(entry->flags & > > MPTCP_PM_ADDR_FLAG_BACKUP); > > - break; > > - } > > - } > > + entry = __lookup_addr_rcu(pernet, skc); > > + backup = entry && !!(entry->flags & > > MPTCP_PM_ADDR_FLAG_BACKUP); > > I think we should check whether entry is NULL here too. No? Sorry, ignore my comment, your code is correct. :) -Geliang > > -Geliang > > > rcu_read_unlock(); > > > > return backup; > > > >
Hi Geliang, Thank you for the review! On 25/10/2024 12:37, Geliang Tang wrote: > On Fri, 2024-10-25 at 11:32 +0200, Matthieu Baerts (NGI0) wrote: >> From: Geliang Tang <tanggeliang@kylinos.cn> >> >> The helper __lookup_addr_rcu() can be used in >> mptcp_pm_nl_get_local_id() >> and mptcp_pm_nl_is_backup() to simplify the code, and avoid code >> duplication. (...) >> diff --git a/net/mptcp/pm_netlink.c b/net/mptcp/pm_netlink.c >> index >> f38e1ccd34e95cd88b179a8b50e6965731542871..7c6e664b236d1659a554d003c78 >> c72ec91895ba5 100644 >> --- a/net/mptcp/pm_netlink.c >> +++ b/net/mptcp/pm_netlink.c (...) >> @@ -1191,15 +1187,11 @@ bool mptcp_pm_nl_is_backup(struct mptcp_sock >> *msk, struct mptcp_addr_info *skc) >> { >> struct pm_nl_pernet *pernet = >> pm_nl_get_pernet_from_msk(msk); >> struct mptcp_pm_addr_entry *entry; >> - bool backup = false; >> + bool backup; >> >> rcu_read_lock(); >> - list_for_each_entry_rcu(entry, &pernet->local_addr_list, >> list) { >> - if (mptcp_addresses_equal(&entry->addr, skc, entry- >>> addr.port)) { >> - backup = !!(entry->flags & >> MPTCP_PM_ADDR_FLAG_BACKUP); >> - break; >> - } >> - } >> + entry = __lookup_addr_rcu(pernet, skc); >> + backup = entry && !!(entry->flags & >> MPTCP_PM_ADDR_FLAG_BACKUP); > > I think we should check whether entry is NULL here too. No? Yes, but that's what I did, no? backup = entry && (entry->flags & BACKUP) "backup" is set to "true" if entry is not NULL and the backup flag is set. Cheers, Matt -- Sponsored by the NGI0 Core fund.
© 2016 - 2025 Red Hat, Inc.