diff mbox series

[mptcp-next,v10,12/12] selftests: mptcp: add path_manager sysctl tests

Message ID d011c0b9ad15e4bc293fcb46b5864515b8239553.1741258415.git.tanggeliang@kylinos.cn (mailing list archive)
State Superseded
Headers show
Series BPF path manager, part 5 | expand

Checks

Context Check Description
matttbe/checkpatch warning total: 0 errors, 1 warnings, 0 checks, 64 lines checked
matttbe/shellcheck success No ShellCheck issues
matttbe/build success Build and static analysis OK
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! ✅

Commit Message

Geliang Tang March 6, 2025, 11:01 a.m. UTC 
From: Geliang Tang <tanggeliang@kylinos.cn>

This patch checks if the newly added net.mptcp.path_manager is mapped
successfully from or to the old net.mptcp.pm_type. And add a new helper
set_path_manager() to set the newly added net.mptcp.path_manager.

Signed-off-by: Geliang Tang <tanggeliang@kylinos.cn>

fix

Signed-off-by: Geliang Tang <tanggeliang@kylinos.cn>
---
 .../selftests/net/mptcp/userspace_pm.sh       | 58 ++++++++++++++++++-
 1 file changed, 55 insertions(+), 3 deletions(-)

Comments

Matthieu Baerts (NGI0) March 10, 2025, 11:25 p.m. UTC | #1 
Hi Geliang,

On 06/03/2025 12:01, Geliang Tang wrote:
> From: Geliang Tang <tanggeliang@kylinos.cn>
> 
> This patch checks if the newly added net.mptcp.path_manager is mapped
> successfully from or to the old net.mptcp.pm_type. And add a new helper
> set_path_manager() to set the newly added net.mptcp.path_manager.
> 
> Signed-off-by: Geliang Tang <tanggeliang@kylinos.cn>
> 
> fix

(typo)

> 
> Signed-off-by: Geliang Tang <tanggeliang@kylinos.cn>
> ---
>  .../selftests/net/mptcp/userspace_pm.sh       | 58 ++++++++++++++++++-
>  1 file changed, 55 insertions(+), 3 deletions(-)
> 
> diff --git a/tools/testing/selftests/net/mptcp/userspace_pm.sh b/tools/testing/selftests/net/mptcp/userspace_pm.sh
> index 3651f73451cf..35ba4edc0fa2 100755
> --- a/tools/testing/selftests/net/mptcp/userspace_pm.sh
> +++ b/tools/testing/selftests/net/mptcp/userspace_pm.sh
> @@ -118,9 +118,61 @@ trap cleanup EXIT
>  
>  # Create and configure network namespaces for testing

I see that you didn't move the "print_title" and "print_test" here. I
think you should do that to be able to print test_fail properly, no?

>  mptcp_lib_ns_init ns1 ns2
> -for i in "$ns1" "$ns2" ;do
> -	ip netns exec "$i" sysctl -q net.mptcp.pm_type=1
> -done
> +
> +set_path_manager()
> +{
> +	local ns=$1
> +	local pm=$2
> +
> +	if ! ip netns exec ${ns} sysctl net.mptcp.available_path_managers |
> +	     grep -wq "${pm}"; then
> +		test_fail "path manager ${pm} not found"
> +		return 1
> +	fi
> +	ip netns exec ${ns} sysctl -q net.mptcp.path_manager="${pm}"
> +}
> +

Maybe add a comment here to explain what's the goal, e.g.

  # check path_manager and pm_type sysctl mapping

> +if [ -f /proc/sys/net/mptcp/path_manager ]; then
> +	ip netns exec "$ns1" sysctl -q net.mptcp.pm_type=0
> +	pm_name="$(ip netns exec "$ns1" sysctl -n net.mptcp.path_manager)"
> +	if [ "${pm_name}" != "kernel" ]; then
> +		test_fail "unexpected pm_name: ${pm_name}"
> +		mptcp_lib_result_print_all_tap
> +		exit ${KSFT_FAIL}
> +	fi
I don't think this step should be moved below, because here we don't
change values: pm_type=0 is the default value.

> +	ip netns exec "$ns1" sysctl -q net.mptcp.pm_type=1
> +	pm_name="$(ip netns exec "$ns1" sysctl -n net.mptcp.path_manager)"
> +	if [ "${pm_name}" != "userspace" ]; then
> +		test_fail "unexpected pm_name: ${pm_name}"
> +		mptcp_lib_result_print_all_tap
> +		exit ${KSFT_FAIL}
> +	fi

I don't think you need this step if you keep the for-loop out of the
'else', like before, see below. It implicitly check that setting pm_type
to 1 also changes "path_manager", otherwise the rest of the tests will fail.

> +
> +	set_path_manager "$ns1" "kernel"
> +	pm_type="$(ip netns exec "$ns1" sysctl -n net.mptcp.pm_type)"
> +	if [ "${pm_type}" != "0" ]; then
> +		test_fail "unexpected pm_type: ${pm_type}"
> +		mptcp_lib_result_print_all_tap
> +		exit ${KSFT_FAIL}
> +	fi
> +
> +	set_path_manager "$ns1" "userspace"
> +	pm_type="$(ip netns exec "$ns1" sysctl -n net.mptcp.pm_type)"
> +	if [ "${pm_type}" != "1" ]; then
> +		test_fail "unexpected pm_type: ${pm_type}"
> +		mptcp_lib_result_print_all_tap
> +		exit ${KSFT_FAIL}
> +	fi
> +
> +	set_path_manager "$ns2" "userspace"
> +	print_test "check path_manager and pm_type sysctl mapping"
> +	test_pass
> +else
> +	for i in "$ns1" "$ns2"; do
> +		ip netns exec "$i" sysctl -q net.mptcp.pm_type=1
> +	done
> +fi

I do think there is no need to do repeating tests here, Just having
these 3 steps are enough to cover all cases directly and indirectly,
easier to maintain without the repeat, and only focus on the mapping
aspect, not on the final values we set (so keeping the 'pm_type=1' at
the end for all kernel versions is easier).

In other words:

  print_title "Init"
  print_test "Created network namespaces ns1, ns2"
  mptcp_lib_ns_init ns1 ns2

  # check path_manager and pm_type sysctl mapping
  if [ -f /proc/sys/net/mptcp/path_manager ]; then
      ip netns exec "$ns1" sysctl -q net.mptcp.path_manager=userspace
      pm_type="$(ip netns exec "$ns1" sysctl -n net.mptcp.pm_type)"
      if [ "${pm_type}" != "1" ]; then
          test_fail "unexpected pm_type: ${pm_type}"
          mptcp_lib_result_print_all_tap
          exit ${KSFT_FAIL}
      fi

      ip netns exec "$ns1" sysctl -q net.mptcp.pm_type=0
      pm="$(ip netns exec "$ns1" sysctl -n net.mptcp.path_manager)"
      if [ "${pm}" != "kernel" ]; then
          test_fail "unexpected path-manager: ${pm}"
          mptcp_lib_result_print_all_tap
          exit ${KSFT_FAIL}
      fi
  fi

  for i in "$ns1" "$ns2"; do
      ip netns exec "$i" sysctl -q net.mptcp.pm_type=1
  done

  (...)

Small and enough, no?

(If you want, you can also add an extra test setting "path_manager" to
"error", and checking that "pm_type" is unchanged, e.g. place this in
the middle of the two.)

      ip netns exec "$ns1" sysctl -q net.mptcp.path_manager=error
2>/dev/null
      pm_type="$(ip netns exec "$ns1" sysctl -n net.mptcp.pm_type)"
      if [ "${pm_type}" != "1" ]; then
          test_fail "unexpected pm_type after error: ${pm_type}"
          mptcp_lib_result_print_all_tap
          exit ${KSFT_FAIL}
      fi

And no need to have the same tests on ns2. Easier to keep the for-loop
setting pm_type=1 like before.

Cheers,
Matt
Geliang Tang March 11, 2025, 6:54 a.m. UTC | #2 
Hi Matt,

On Tue, 2025-03-11 at 00:25 +0100, Matthieu Baerts wrote:
> Hi Geliang,
> 
> On 06/03/2025 12:01, Geliang Tang wrote:
> > From: Geliang Tang <tanggeliang@kylinos.cn>
> > 
> > This patch checks if the newly added net.mptcp.path_manager is
> > mapped
> > successfully from or to the old net.mptcp.pm_type. And add a new
> > helper
> > set_path_manager() to set the newly added net.mptcp.path_manager.
> > 
> > Signed-off-by: Geliang Tang <tanggeliang@kylinos.cn>
> > 
> > fix
> 
> (typo)
> 
> > 
> > Signed-off-by: Geliang Tang <tanggeliang@kylinos.cn>
> > ---
> >  .../selftests/net/mptcp/userspace_pm.sh       | 58
> > ++++++++++++++++++-
> >  1 file changed, 55 insertions(+), 3 deletions(-)
> > 
> > diff --git a/tools/testing/selftests/net/mptcp/userspace_pm.sh
> > b/tools/testing/selftests/net/mptcp/userspace_pm.sh
> > index 3651f73451cf..35ba4edc0fa2 100755
> > --- a/tools/testing/selftests/net/mptcp/userspace_pm.sh
> > +++ b/tools/testing/selftests/net/mptcp/userspace_pm.sh
> > @@ -118,9 +118,61 @@ trap cleanup EXIT
> >  
> >  # Create and configure network namespaces for testing
> 
> I see that you didn't move the "print_title" and "print_test" here. I
> think you should do that to be able to print test_fail properly, no?
> 
> >  mptcp_lib_ns_init ns1 ns2
> > -for i in "$ns1" "$ns2" ;do
> > -	ip netns exec "$i" sysctl -q net.mptcp.pm_type=1
> > -done
> > +
> > +set_path_manager()
> > +{
> > +	local ns=$1
> > +	local pm=$2
> > +
> > +	if ! ip netns exec ${ns} sysctl
> > net.mptcp.available_path_managers |
> > +	     grep -wq "${pm}"; then
> > +		test_fail "path manager ${pm} not found"
> > +		return 1
> > +	fi
> > +	ip netns exec ${ns} sysctl -q
> > net.mptcp.path_manager="${pm}"
> > +}
> > +
> 
> Maybe add a comment here to explain what's the goal, e.g.
> 
>   # check path_manager and pm_type sysctl mapping
> 
> > +if [ -f /proc/sys/net/mptcp/path_manager ]; then
> > +	ip netns exec "$ns1" sysctl -q net.mptcp.pm_type=0
> > +	pm_name="$(ip netns exec "$ns1" sysctl -n
> > net.mptcp.path_manager)"
> > +	if [ "${pm_name}" != "kernel" ]; then
> > +		test_fail "unexpected pm_name: ${pm_name}"
> > +		mptcp_lib_result_print_all_tap
> > +		exit ${KSFT_FAIL}
> > +	fi
> I don't think this step should be moved below, because here we don't
> change values: pm_type=0 is the default value.
> 
> > +	ip netns exec "$ns1" sysctl -q net.mptcp.pm_type=1
> > +	pm_name="$(ip netns exec "$ns1" sysctl -n
> > net.mptcp.path_manager)"
> > +	if [ "${pm_name}" != "userspace" ]; then
> > +		test_fail "unexpected pm_name: ${pm_name}"
> > +		mptcp_lib_result_print_all_tap
> > +		exit ${KSFT_FAIL}
> > +	fi
> 
> I don't think you need this step if you keep the for-loop out of the
> 'else', like before, see below. It implicitly check that setting
> pm_type
> to 1 also changes "path_manager", otherwise the rest of the tests
> will fail.
> 
> > +
> > +	set_path_manager "$ns1" "kernel"
> > +	pm_type="$(ip netns exec "$ns1" sysctl -n
> > net.mptcp.pm_type)"
> > +	if [ "${pm_type}" != "0" ]; then
> > +		test_fail "unexpected pm_type: ${pm_type}"
> > +		mptcp_lib_result_print_all_tap
> > +		exit ${KSFT_FAIL}
> > +	fi
> > +
> > +	set_path_manager "$ns1" "userspace"
> > +	pm_type="$(ip netns exec "$ns1" sysctl -n
> > net.mptcp.pm_type)"
> > +	if [ "${pm_type}" != "1" ]; then
> > +		test_fail "unexpected pm_type: ${pm_type}"
> > +		mptcp_lib_result_print_all_tap
> > +		exit ${KSFT_FAIL}
> > +	fi
> > +
> > +	set_path_manager "$ns2" "userspace"
> > +	print_test "check path_manager and pm_type sysctl mapping"
> > +	test_pass
> > +else
> > +	for i in "$ns1" "$ns2"; do
> > +		ip netns exec "$i" sysctl -q net.mptcp.pm_type=1
> > +	done
> > +fi
> 
> I do think there is no need to do repeating tests here, Just having
> these 3 steps are enough to cover all cases directly and indirectly,
> easier to maintain without the repeat, and only focus on the mapping
> aspect, not on the final values we set (so keeping the 'pm_type=1' at
> the end for all kernel versions is easier).
> 
> In other words:
> 
>   print_title "Init"
>   print_test "Created network namespaces ns1, ns2"
>   mptcp_lib_ns_init ns1 ns2

test_pass should be followed this print_test to print a [OK].
So this may be better:

print_title "Init"
mptcp_lib_ns_init ns1 ns2

if []; then
	test_fail "..."
fi

print_test "Created network namespaces ns1, ns2"
test_pass

So in v11, I just moved 'print_title "Init"' forward but kept
"print_test" and "test_pass" unchanged.

Thanks,
-Geliang

> 
>   # check path_manager and pm_type sysctl mapping
>   if [ -f /proc/sys/net/mptcp/path_manager ]; then
>       ip netns exec "$ns1" sysctl -q net.mptcp.path_manager=userspace
>       pm_type="$(ip netns exec "$ns1" sysctl -n net.mptcp.pm_type)"
>       if [ "${pm_type}" != "1" ]; then
>           test_fail "unexpected pm_type: ${pm_type}"
>           mptcp_lib_result_print_all_tap
>           exit ${KSFT_FAIL}
>       fi
> 
>       ip netns exec "$ns1" sysctl -q net.mptcp.pm_type=0
>       pm="$(ip netns exec "$ns1" sysctl -n net.mptcp.path_manager)"
>       if [ "${pm}" != "kernel" ]; then
>           test_fail "unexpected path-manager: ${pm}"
>           mptcp_lib_result_print_all_tap
>           exit ${KSFT_FAIL}
>       fi
>   fi
> 
>   for i in "$ns1" "$ns2"; do
>       ip netns exec "$i" sysctl -q net.mptcp.pm_type=1
>   done
> 
>   (...)
> 
> Small and enough, no?
> 
> (If you want, you can also add an extra test setting "path_manager"
> to
> "error", and checking that "pm_type" is unchanged, e.g. place this in
> the middle of the two.)
> 
>       ip netns exec "$ns1" sysctl -q net.mptcp.path_manager=error
> 2>/dev/null
>       pm_type="$(ip netns exec "$ns1" sysctl -n net.mptcp.pm_type)"
>       if [ "${pm_type}" != "1" ]; then
>           test_fail "unexpected pm_type after error: ${pm_type}"
>           mptcp_lib_result_print_all_tap
>           exit ${KSFT_FAIL}
>       fi
> 
> And no need to have the same tests on ns2. Easier to keep the for-
> loop
> setting pm_type=1 like before.
> 
> Cheers,
> Matt
diff mbox series

Patch

diff --git a/tools/testing/selftests/net/mptcp/userspace_pm.sh b/tools/testing/selftests/net/mptcp/userspace_pm.sh
index 3651f73451cf..35ba4edc0fa2 100755
--- a/tools/testing/selftests/net/mptcp/userspace_pm.sh
+++ b/tools/testing/selftests/net/mptcp/userspace_pm.sh
@@ -118,9 +118,61 @@  trap cleanup EXIT
 
 # Create and configure network namespaces for testing
 mptcp_lib_ns_init ns1 ns2
-for i in "$ns1" "$ns2" ;do
-	ip netns exec "$i" sysctl -q net.mptcp.pm_type=1
-done
+
+set_path_manager()
+{
+	local ns=$1
+	local pm=$2
+
+	if ! ip netns exec ${ns} sysctl net.mptcp.available_path_managers |
+	     grep -wq "${pm}"; then
+		test_fail "path manager ${pm} not found"
+		return 1
+	fi
+	ip netns exec ${ns} sysctl -q net.mptcp.path_manager="${pm}"
+}
+
+if [ -f /proc/sys/net/mptcp/path_manager ]; then
+	ip netns exec "$ns1" sysctl -q net.mptcp.pm_type=0
+	pm_name="$(ip netns exec "$ns1" sysctl -n net.mptcp.path_manager)"
+	if [ "${pm_name}" != "kernel" ]; then
+		test_fail "unexpected pm_name: ${pm_name}"
+		mptcp_lib_result_print_all_tap
+		exit ${KSFT_FAIL}
+	fi
+
+	ip netns exec "$ns1" sysctl -q net.mptcp.pm_type=1
+	pm_name="$(ip netns exec "$ns1" sysctl -n net.mptcp.path_manager)"
+	if [ "${pm_name}" != "userspace" ]; then
+		test_fail "unexpected pm_name: ${pm_name}"
+		mptcp_lib_result_print_all_tap
+		exit ${KSFT_FAIL}
+	fi
+
+	set_path_manager "$ns1" "kernel"
+	pm_type="$(ip netns exec "$ns1" sysctl -n net.mptcp.pm_type)"
+	if [ "${pm_type}" != "0" ]; then
+		test_fail "unexpected pm_type: ${pm_type}"
+		mptcp_lib_result_print_all_tap
+		exit ${KSFT_FAIL}
+	fi
+
+	set_path_manager "$ns1" "userspace"
+	pm_type="$(ip netns exec "$ns1" sysctl -n net.mptcp.pm_type)"
+	if [ "${pm_type}" != "1" ]; then
+		test_fail "unexpected pm_type: ${pm_type}"
+		mptcp_lib_result_print_all_tap
+		exit ${KSFT_FAIL}
+	fi
+
+	set_path_manager "$ns2" "userspace"
+	print_test "check path_manager and pm_type sysctl mapping"
+	test_pass
+else
+	for i in "$ns1" "$ns2"; do
+		ip netns exec "$i" sysctl -q net.mptcp.pm_type=1
+	done
+fi
 
 #  "$ns1"              ns2
 #     ns1eth2    ns2eth1