| Message ID | 20241107-mptcp-pm-lookup_addr_rcu-v3-0-3c458d025de4@kernel.org (mailing list archive) |
|---|---|
| Headers | show |
| Series | mptcp: pm: use _rcu variant under rcu_read_lock | expand |
Hi Geliang,
Thank you for your modifications, that's great!
Our CI did some validations and here is its report:
- KVM Validation: normal: Success! ✅
- KVM Validation: debug: Success! ✅
- KVM Validation: btf-normal (only bpftest_all): Success! ✅
- KVM Validation: btf-debug (only bpftest_all): Success! ✅
- Task: https://github.com/multipath-tcp/mptcp_net-next/actions/runs/11720123635
Initiator: Patchew Applier
Commits: https://github.com/multipath-tcp/mptcp_net-next/commits/e0cfb9d208fa
Patchwork: https://patchwork.kernel.org/project/mptcp/list/?series=907253
If there are some issues, you can reproduce them using the same environment as
the one used by the CI thanks to a docker image, e.g.:
$ cd [kernel source code]
$ docker run -v "${PWD}:${PWD}:rw" -w "${PWD}" --privileged --rm -it \
--pull always mptcp/mptcp-upstream-virtme-docker:latest \
auto-normal
For more details:
https://github.com/multipath-tcp/mptcp-upstream-virtme-docker
Please note that despite all the efforts that have been already done to have a
stable tests suite when executed on a public CI like here, it is possible some
reported issues are not due to your modifications. Still, do not hesitate to
help us improve that ;-)
Cheers,
MPTCP GH Action bot
Bot operated by Matthieu Baerts (NGI0 Core)
Hi Matt, Thanks for this v3. LGTM! Reviewed-by: Geliang Tang <geliang@kernel.org> nit: How about writing the abbreviation "endp" in the subject of patch 3 as "endpoint"? Just like it in patch 2. Then their subject lengths are the same: mptcp: pm: avoid code duplication to lookup endpoint mptcp: pm: lockless list traversal to dump endpoints Thanks, -Geliang On Thu, 2024-11-07 at 10:19 +0000, MPTCP CI wrote: > Hi Geliang, > > Thank you for your modifications, that's great! > > Our CI did some validations and here is its report: > > - KVM Validation: normal: Success! ✅ > - KVM Validation: debug: Success! ✅ > - KVM Validation: btf-normal (only bpftest_all): Success! ✅ > - KVM Validation: btf-debug (only bpftest_all): Success! ✅ > - Task: > https://github.com/multipath-tcp/mptcp_net-next/actions/runs/11720123635 > > Initiator: Patchew Applier > Commits: > https://github.com/multipath-tcp/mptcp_net-next/commits/e0cfb9d208fa > Patchwork: > https://patchwork.kernel.org/project/mptcp/list/?series=907253 > > > If there are some issues, you can reproduce them using the same > environment as > the one used by the CI thanks to a docker image, e.g.: > > $ cd [kernel source code] > $ docker run -v "${PWD}:${PWD}:rw" -w "${PWD}" --privileged --rm > -it \ > --pull always mptcp/mptcp-upstream-virtme-docker:latest \ > auto-normal > > For more details: > > https://github.com/multipath-tcp/mptcp-upstream-virtme-docker > > > Please note that despite all the efforts that have been already done > to have a > stable tests suite when executed on a public CI like here, it is > possible some > reported issues are not due to your modifications. Still, do not > hesitate to > help us improve that ;-) > > Cheers, > MPTCP GH Action bot > Bot operated by Matthieu Baerts (NGI0 Core) >
On 11/7/24 10:04, Matthieu Baerts (NGI0) wrote: > When looking at something else, I noticed that the local endpoint > entries list was iterated under rcu_read_lock, but using > list_for_each_entry() instead of the _rcu variant. That's what patch 1 > is fixing. > > At the previous meeting, Mat and Christoph mentioned we could use the > RCU variant elsewhere. That's what is done in patch 2, for -next then. > > Patch 3 is a simple change to remove duplicated code. > > Note: I see that we are using spin_lock_bh(), but the RCU read "locks" > are always used without the _bh() variant. Is that OK here, or did we > miss something? > > Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org> > --- > Changes in v3: > - Use list_for_each_entry_rcu() with a 4th parameter: lockdep_is_held() > - See individual changelog in the different patches. > - Link to v2: https://lore.kernel.org/r/20241025-mptcp-pm-lookup_addr_rcu-v2-0-1478f6c4b205@kernel.org > > Changes in v2: > - Add patch 2 and 3 > - Patch 1: avoid > 80 chars per line in __lookup_addr_rcu() + update > commit message. > - Link to v1: https://lore.kernel.org/r/20241022-mptcp-pm-lookup_addr_rcu-v1-1-19d45f26c872@kernel.org > FWIW, LGTM, too. I don't add my explicit formal ack just to avoid possibly weird-looking tag areas after merge on netdev ;) /P
Hi Geliang, On 08/11/2024 02:47, Geliang Tang wrote: > Hi Matt, > > Thanks for this v3. LGTM! > > Reviewed-by: Geliang Tang <geliang@kernel.org> Thank you for the review! > nit: > > How about writing the abbreviation "endp" in the subject of patch 3 as > "endpoint"? Just like it in patch 2. Then their subject lengths are the > same: > > mptcp: pm: avoid code duplication to lookup endpoint > mptcp: pm: lockless list traversal to dump endpoints I suggest the opposite: use 'endp' for both, to have commit titles that are under 50 chars, as it is usually recommended by the 50/72 rules (as long as it is still understandable). Cheers, Matt
Hello, On 07/11/2024 10:04, Matthieu Baerts (NGI0) wrote: > When looking at something else, I noticed that the local endpoint > entries list was iterated under rcu_read_lock, but using > list_for_each_entry() instead of the _rcu variant. That's what patch 1 > is fixing. > > At the previous meeting, Mat and Christoph mentioned we could use the > RCU variant elsewhere. That's what is done in patch 2, for -next then. > > Patch 3 is a simple change to remove duplicated code. Now in our tree (fixes for -net, and feat. for -next): New patches for t/upstream-net and t/upstream: - 562b779666e2: mptcp: pm: use _rcu variant under rcu_read_lock - Results: 41c594936cc0..d67f9238deda (export-net) New patches for t/upstream: - bd3a08dc4788: mptcp: pm: lockless list traversal to dump endpoints - (7045c531565f: tg:msg: title under < 50) - 26fc0949dd21: mptcp: pm: avoid code duplication to lookup endp - Results: 19f842ec6498..17e34effcf7b (export) Tests are now in progress: - export-net: https://github.com/multipath-tcp/mptcp_net-next/commit/5855ee9f27ce230c846422d4106358907c313f09/checks - export: https://github.com/multipath-tcp/mptcp_net-next/commit/9a258dc7072ac24b37a8c6a8d03533981747ba6a/checks Cheers, Matt
When looking at something else, I noticed that the local endpoint entries list was iterated under rcu_read_lock, but using list_for_each_entry() instead of the _rcu variant. That's what patch 1 is fixing. At the previous meeting, Mat and Christoph mentioned we could use the RCU variant elsewhere. That's what is done in patch 2, for -next then. Patch 3 is a simple change to remove duplicated code. Note: I see that we are using spin_lock_bh(), but the RCU read "locks" are always used without the _bh() variant. Is that OK here, or did we miss something? Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org> --- Changes in v3: - Use list_for_each_entry_rcu() with a 4th parameter: lockdep_is_held() - See individual changelog in the different patches. - Link to v2: https://lore.kernel.org/r/20241025-mptcp-pm-lookup_addr_rcu-v2-0-1478f6c4b205@kernel.org Changes in v2: - Add patch 2 and 3 - Patch 1: avoid > 80 chars per line in __lookup_addr_rcu() + update commit message. - Link to v1: https://lore.kernel.org/r/20241022-mptcp-pm-lookup_addr_rcu-v1-1-19d45f26c872@kernel.org --- Geliang Tang (1): mptcp: pm: avoid code duplication to lookup endp Matthieu Baerts (NGI0) (2): mptcp: pm: use _rcu variant under rcu_read_lock mptcp: pm: lockless list traversal to dump endpoints net/mptcp/pm_netlink.c | 36 +++++++++++++++--------------------- 1 file changed, 15 insertions(+), 21 deletions(-) --- base-commit: ca26062fcd85c61922f543674c5dd0382e2059cd change-id: 20241022-mptcp-pm-lookup_addr_rcu-01833ea95155 Best regards,