[PATCH mptcp-net 0/2] mptcp: remove unneeded lock when listing scheds

[PATCH mptcp-net 0/2] mptcp: remove unneeded lock when listing scheds

[Matthieu Baerts (NGI0)]

A fix suggested by Paolo, and a small addition in the selftests to
increase the code coverage. I guess they can both go to -net.

Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
---
Matthieu Baerts (NGI0) (2):
      mptcp: remove unneeded lock when listing scheds
      selftests: mptcp: list sysctl data

 net/mptcp/sched.c                                  | 2 --
 tools/testing/selftests/net/mptcp/mptcp_connect.sh | 9 +++++++++
 2 files changed, 9 insertions(+), 2 deletions(-)
---
base-commit: c60af6efcac3836a49c583f8a127560697d8c3a8
change-id: 20241017-mptcp-sched-avail-lock-a60714cb3218

Best regards,
-- 
Matthieu Baerts (NGI0) <matttbe@kernel.org>

[PATCH mptcp-net 1/2] mptcp: remove unneeded lock when listing scheds

[Matthieu Baerts (NGI0)]

mptcp_get_available_schedulers() needs to iterate over the schedulers'
list only to read the names: it doesn't modify anything there.

In this case, it is enough to hold the RCU read lock, no need to combine
this with the associated spin lock.

Fixes: 73c900aa3660 ("mptcp: add net.mptcp.available_schedulers")
Suggested-by: Paolo Abeni <pabeni@redhat.com>
Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
---
 net/mptcp/sched.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/net/mptcp/sched.c b/net/mptcp/sched.c
index 5257bc6c8cd6e8ca674c9beccaf7b27c989da76a..6da60e35932ffd8a92ad7a8b128e5eaf09219b82 100644
--- a/net/mptcp/sched.c
+++ b/net/mptcp/sched.c
@@ -60,7 +60,6 @@ void mptcp_get_available_schedulers(char *buf, size_t maxlen)
 	size_t offs = 0;
 
 	rcu_read_lock();
-	spin_lock(&mptcp_sched_list_lock);
 	list_for_each_entry_rcu(sched, &mptcp_sched_list, list) {
 		offs += snprintf(buf + offs, maxlen - offs,
 				 "%s%s",
@@ -69,7 +68,6 @@ void mptcp_get_available_schedulers(char *buf, size_t maxlen)
 		if (WARN_ON_ONCE(offs >= maxlen))
 			break;
 	}
-	spin_unlock(&mptcp_sched_list_lock);
 	rcu_read_unlock();
 }
 

-- 
2.45.2

[PATCH mptcp-net 2/2] selftests: mptcp: list sysctl data

[Matthieu Baerts (NGI0)]

Listing all the values linked to the MPTCP sysctl knobs was not
exercised in MPTCP test suite.

Let's do that to avoid any regressions, but also to have a kernel with a
debug kconfig verifying more assumptions. For the moment, we are not
interested by the output, only to avoid crashes and warnings.

Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
---
 tools/testing/selftests/net/mptcp/mptcp_connect.sh | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/tools/testing/selftests/net/mptcp/mptcp_connect.sh b/tools/testing/selftests/net/mptcp/mptcp_connect.sh
index 57325d57e4c6e3653019db2de09620d692143683..b48b4e56826a9cfdb3501242b707ae2ebe29b220 100755
--- a/tools/testing/selftests/net/mptcp/mptcp_connect.sh
+++ b/tools/testing/selftests/net/mptcp/mptcp_connect.sh
@@ -259,6 +259,15 @@ check_mptcp_disabled()
 	mptcp_lib_ns_init disabled_ns
 
 	print_larger_title "New MPTCP socket can be blocked via sysctl"
+
+	# mainly to cover more code
+	if ! ip netns exec ${disabled_ns} sysctl net.mptcp >/dev/null; then
+		mptcp_lib_pr_fail "not able to list net.mptcp sysctl knobs"
+		mptcp_lib_result_fail "not able to list net.mptcp sysctl knobs"
+		ret=${KSFT_FAIL}
+		return 1
+	fi
+
 	# net.mptcp.enabled should be enabled by default
 	if [ "$(ip netns exec ${disabled_ns} sysctl net.mptcp.enabled | awk '{ print $3 }')" -ne 1 ]; then
 		mptcp_lib_pr_fail "net.mptcp.enabled sysctl is not 1 by default"

-- 
2.45.2

Re: [PATCH mptcp-net 0/2] mptcp: remove unneeded lock when listing scheds

[MPTCP CI]

Hi Matthieu,

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/11383138502

Initiator: Patchew Applier
Commits: https://github.com/multipath-tcp/mptcp_net-next/commits/8866d19ec4ef
Patchwork: https://patchwork.kernel.org/project/mptcp/list/?series=900176


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)

Re: [PATCH mptcp-net 1/2] mptcp: remove unneeded lock when listing scheds

[Geliang Tang]

Hi Matt,

Thanks for this fix.

On Thu, 2024-10-17 at 12:16 +0200, Matthieu Baerts (NGI0) wrote:
> mptcp_get_available_schedulers() needs to iterate over the
> schedulers'
> list only to read the names: it doesn't modify anything there.
> 
> In this case, it is enough to hold the RCU read lock, no need to
> combine
> this with the associated spin lock.
> 
> Fixes: 73c900aa3660 ("mptcp: add net.mptcp.available_schedulers")
> Suggested-by: Paolo Abeni <pabeni@redhat.com>
> Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>

Good catch!

Reviewed-by: Geliang Tang <geliang@kernel.org>

-Geliang

> ---
>  net/mptcp/sched.c | 2 --
>  1 file changed, 2 deletions(-)
> 
> diff --git a/net/mptcp/sched.c b/net/mptcp/sched.c
> index
> 5257bc6c8cd6e8ca674c9beccaf7b27c989da76a..6da60e35932ffd8a92ad7a8b128
> e5eaf09219b82 100644
> --- a/net/mptcp/sched.c
> +++ b/net/mptcp/sched.c
> @@ -60,7 +60,6 @@ void mptcp_get_available_schedulers(char *buf,
> size_t maxlen)
>  	size_t offs = 0;
>  
>  	rcu_read_lock();
> -	spin_lock(&mptcp_sched_list_lock);
>  	list_for_each_entry_rcu(sched, &mptcp_sched_list, list) {
>  		offs += snprintf(buf + offs, maxlen - offs,
>  				 "%s%s",
> @@ -69,7 +68,6 @@ void mptcp_get_available_schedulers(char *buf,
> size_t maxlen)
>  		if (WARN_ON_ONCE(offs >= maxlen))
>  			break;
>  	}
> -	spin_unlock(&mptcp_sched_list_lock);
>  	rcu_read_unlock();
>  }
>  
> 

Re: [PATCH mptcp-net 0/2] mptcp: remove unneeded lock when listing scheds

[Matthieu Baerts]

Hi Geliang,

On 17/10/2024 12:16, Matthieu Baerts (NGI0) wrote:
> A fix suggested by Paolo, and a small addition in the selftests to
> increase the code coverage. I guess they can both go to -net.

Thank you for your review on patch 1/2, I just applied the series in our
tree:

New patches for t/upstream-net and t/upstream:
- 7c2d2c795bd4: mptcp: remove unneeded lock when listing scheds
- 5f07b4f41bdd: selftests: mptcp: list sysctl data
- Results: e7969542d0a3..30a6ded06481 (export-net)
- Results: dc99b2bc6131..54b01fd36ac6 (export)

Tests are now in progress:

- export-net:
https://github.com/multipath-tcp/mptcp_net-next/commit/136da0312931deb877b993a5d5a0fa6861e5da54/checks
- export:
https://github.com/multipath-tcp/mptcp_net-next/commit/8d5658f1131d99357c5533d88541880bd97d0539/checks


Cheers,
Matt
-- 
Sponsored by the NGI0 Core fund.