| Message ID | 63ed0d182630250fda144a5ee3770a34384003fa.1729588019.git.tanggeliang@kylinos.cn (mailing list archive) |
|---|---|
| State | Rejected, archived |
| Delegated to: | Matthieu Baerts |
| Headers | show |
| Series | BPF path manager | expand |
| Context | Check | Description |
|---|---|---|
| matttbe/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 38 lines checked |
| matttbe/shellcheck | success | MPTCP selftests files have not been modified |
| matttbe/build | warning | Build error with: make C=1 net/mptcp/bpf.o |
| matttbe/KVM_Validation__normal | success | Success! ✅ |
| matttbe/KVM_Validation__debug | success | Success! ✅ |
| matttbe/KVM_Validation__btf-normal__only_bpftest_all_ | success | Success! ✅ |
| matttbe/KVM_Validation__btf-debug__only_bpftest_all_ | success | Success! ✅ |
Hi Geliang, On 22/10/2024 11:14, Geliang Tang wrote: > From: Geliang Tang <tanggeliang@kylinos.cn> > > The helper __lookup_addr() can be used in mptcp_pm_nl_get_local_id() > and mptcp_pm_nl_is_backup() to simplify the code if using > list_for_each_entry_rcu() instead of list_for_each_entry() in it. Mmh, please justify why it is OK to use the _rcu() variant without having to modify the caller. Did you check everything was OK when running the tests with these kconfig: CONFIG_RCU_EXPERT=y CONFIG_PROVE_RCU_LIST=y I guess you will get new issues, no? We might need to have __lookup_addr() and __lookup_addr_rcu() if you want to avoid duplicated code. Cheers, Matt
On Tue, 2024-10-22 at 19:09 +0200, Matthieu Baerts wrote: > Hi Geliang, > > On 22/10/2024 11:14, Geliang Tang wrote: > > From: Geliang Tang <tanggeliang@kylinos.cn> > > > > The helper __lookup_addr() can be used in > > mptcp_pm_nl_get_local_id() > > and mptcp_pm_nl_is_backup() to simplify the code if using > > list_for_each_entry_rcu() instead of list_for_each_entry() in it. > > Mmh, please justify why it is OK to use the _rcu() variant without > having to modify the caller. > > Did you check everything was OK when running the tests with these > kconfig: > > CONFIG_RCU_EXPERT=y > CONFIG_PROVE_RCU_LIST=y > > I guess you will get new issues, no? Indeed. > > We might need to have __lookup_addr() and __lookup_addr_rcu() if you > want to avoid duplicated code. Also remove this patch from this series, it has nothing to do with the entire BPF path manager set, and other paths have no dependencies on it. I will release a v2 later separately. Thanks, -Geliang > > Cheers, > Matt
On 23/10/2024 11:59, Geliang Tang wrote: > On Tue, 2024-10-22 at 19:09 +0200, Matthieu Baerts wrote: >> Hi Geliang, >> >> On 22/10/2024 11:14, Geliang Tang wrote: >>> From: Geliang Tang <tanggeliang@kylinos.cn> >>> >>> The helper __lookup_addr() can be used in >>> mptcp_pm_nl_get_local_id() >>> and mptcp_pm_nl_is_backup() to simplify the code if using >>> list_for_each_entry_rcu() instead of list_for_each_entry() in it. >> >> Mmh, please justify why it is OK to use the _rcu() variant without >> having to modify the caller. >> >> Did you check everything was OK when running the tests with these >> kconfig: >> >> CONFIG_RCU_EXPERT=y >> CONFIG_PROVE_RCU_LIST=y >> >> I guess you will get new issues, no? > > Indeed. > >> >> We might need to have __lookup_addr() and __lookup_addr_rcu() if you >> want to avoid duplicated code. > > Also remove this patch from this series, it has nothing to do with the > entire BPF path manager set, and other paths have no dependencies on > it. Will do! Note that you could use __lookup_addr_rcu() that is being added with the following patch (if it is accepted): https://patchwork.kernel.org/project/mptcp/patch/20241022-mptcp-pm-lookup_addr_rcu-v1-1-19d45f26c872@kernel.org/ > I will release a v2 later separately. Please wait for the v3, I'm still looking at the series (... doing that slowly, when I have time :-/) Cheers, Matt
On Wed, 2024-10-23 at 12:03 +0200, Matthieu Baerts wrote: > On 23/10/2024 11:59, Geliang Tang wrote: > > On Tue, 2024-10-22 at 19:09 +0200, Matthieu Baerts wrote: > > > Hi Geliang, > > > > > > On 22/10/2024 11:14, Geliang Tang wrote: > > > > From: Geliang Tang <tanggeliang@kylinos.cn> > > > > > > > > The helper __lookup_addr() can be used in > > > > mptcp_pm_nl_get_local_id() > > > > and mptcp_pm_nl_is_backup() to simplify the code if using > > > > list_for_each_entry_rcu() instead of list_for_each_entry() in > > > > it. > > > > > > Mmh, please justify why it is OK to use the _rcu() variant > > > without > > > having to modify the caller. > > > > > > Did you check everything was OK when running the tests with these > > > kconfig: > > > > > > CONFIG_RCU_EXPERT=y > > > CONFIG_PROVE_RCU_LIST=y > > > > > > I guess you will get new issues, no? > > > > Indeed. > > > > > > > > We might need to have __lookup_addr() and __lookup_addr_rcu() if > > > you > > > want to avoid duplicated code. > > > > Also remove this patch from this series, it has nothing to do with > > the > > entire BPF path manager set, and other paths have no dependencies > > on > > it. > > Will do! > > Note that you could use __lookup_addr_rcu() that is being added with > the > following patch (if it is accepted): > > https://patchwork.kernel.org/project/mptcp/patch/20241022-mptcp-pm-lookup_addr_rcu-v1-1-19d45f26c872@kernel.org/ > > > I will release a v2 later separately. > > Please wait for the v3, I'm still looking at the series (... doing > that > slowly, when I have time :-/) Great, I appreciate it. > > Cheers, > Matt
diff --git a/net/mptcp/pm_netlink.c b/net/mptcp/pm_netlink.c index 618289aac0ab..a60a6fc04bf4 100644 --- a/net/mptcp/pm_netlink.c +++ b/net/mptcp/pm_netlink.c @@ -524,7 +524,7 @@ __lookup_addr(struct pm_nl_pernet *pernet, const struct mptcp_addr_info *info) { struct mptcp_pm_addr_entry *entry; - list_for_each_entry(entry, &pernet->local_addr_list, list) { + list_for_each_entry_rcu(entry, &pernet->local_addr_list, list) { if (mptcp_addresses_equal(&entry->addr, info, entry->addr.port)) return entry; } @@ -1146,12 +1146,9 @@ int mptcp_pm_nl_get_local_id(struct mptcp_sock *msk, struct mptcp_addr_info *skc 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(pernet, skc); + if (entry) + ret = entry->addr.id; rcu_read_unlock(); if (ret >= 0) return ret; @@ -1181,12 +1178,9 @@ bool mptcp_pm_nl_is_backup(struct mptcp_sock *msk, struct mptcp_addr_info *skc) bool backup = false; 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(pernet, skc); + if (entry) + backup = !!(entry->flags & MPTCP_PM_ADDR_FLAG_BACKUP); rcu_read_unlock(); return backup;