1. Patchew
  2. MPTCP
  3. View series

[PATCH mptcp-net v2] selftests: mptcp: join: fix subflow_send_ack lookup

Geliang Tang posted 1 patch 6 months ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/multipath-tcp/mptcp_net-next tags/patchew/f343a6ca7d627b4092a284dbc6724f34e6f8c491.1699973683.git.geliang.tang@suse.com
Maintainers: Matthieu Baerts <matttbe@kernel.org>, Mat Martineau <martineau@kernel.org>, "David S. Miller" <davem@davemloft.net>, Eric Dumazet <edumazet@google.com>, Jakub Kicinski <kuba@kernel.org>, Paolo Abeni <pabeni@redhat.com>, Shuah Khan <shuah@kernel.org>
tools/testing/selftests/net/mptcp/mptcp_join.sh | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
[PATCH mptcp-net v2] selftests: mptcp: join: fix subflow_send_ack lookup
Posted by Geliang Tang 6 months ago 
MPC backups tests will skip unexpected sometimes, since static functions
like mptcp_subflow_send_ack may also be listed in /proc/kallsyms, with a
't' in front of it, not 'T' ('T' is for a global function):

 > grep "mptcp_subflow_send_ack" /proc/kallsyms

 0000000000000000 T __pfx___mptcp_subflow_send_ack
 0000000000000000 T __mptcp_subflow_send_ack
 0000000000000000 t __pfx_mptcp_subflow_send_ack
 0000000000000000 t mptcp_subflow_send_ack

In this case, mptcp_lib_kallsyms_doesnt_have "mptcp_subflow_send_ack$"
will be false, MPC backups tests will skip. This is not what we expected.

The correct logic here should be: if mptcp_subflow_send_ack is not a
global function in /proc/kallsyms, do these MPC backups tests. So a 'T'
must be added in front of mptcp_subflow_send_ack.

Fixes: 632978f0a961 ("selftests: mptcp: join: skip MPC backups tests if not supported")
Signed-off-by: Geliang Tang <geliang.tang@suse.com>
---
v2:
 - Update commit log, fix typos in it.
---
 tools/testing/selftests/net/mptcp/mptcp_join.sh | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/tools/testing/selftests/net/mptcp/mptcp_join.sh b/tools/testing/selftests/net/mptcp/mptcp_join.sh
index 1606474232f6..6fbc70332caa 100755
--- a/tools/testing/selftests/net/mptcp/mptcp_join.sh
+++ b/tools/testing/selftests/net/mptcp/mptcp_join.sh
@@ -2755,7 +2755,7 @@ backup_tests()
 	fi
 
 	if reset "mpc backup" &&
-	   continue_if mptcp_lib_kallsyms_doesnt_have "mptcp_subflow_send_ack$"; then
+	   continue_if mptcp_lib_kallsyms_doesnt_have "T mptcp_subflow_send_ack$"; then
 		pm_nl_add_endpoint $ns2 10.0.1.2 flags subflow,backup
 		speed=slow \
 			run_tests $ns1 $ns2 10.0.1.1
@@ -2764,7 +2764,7 @@ backup_tests()
 	fi
 
 	if reset "mpc backup both sides" &&
-	   continue_if mptcp_lib_kallsyms_doesnt_have "mptcp_subflow_send_ack$"; then
+	   continue_if mptcp_lib_kallsyms_doesnt_have "T mptcp_subflow_send_ack$"; then
 		pm_nl_add_endpoint $ns1 10.0.1.1 flags subflow,backup
 		pm_nl_add_endpoint $ns2 10.0.1.2 flags subflow,backup
 		speed=slow \
@@ -2774,7 +2774,7 @@ backup_tests()
 	fi
 
 	if reset "mpc switch to backup" &&
-	   continue_if mptcp_lib_kallsyms_doesnt_have "mptcp_subflow_send_ack$"; then
+	   continue_if mptcp_lib_kallsyms_doesnt_have "T mptcp_subflow_send_ack$"; then
 		pm_nl_add_endpoint $ns2 10.0.1.2 flags subflow
 		sflags=backup speed=slow \
 			run_tests $ns1 $ns2 10.0.1.1
@@ -2783,7 +2783,7 @@ backup_tests()
 	fi
 
 	if reset "mpc switch to backup both sides" &&
-	   continue_if mptcp_lib_kallsyms_doesnt_have "mptcp_subflow_send_ack$"; then
+	   continue_if mptcp_lib_kallsyms_doesnt_have "T mptcp_subflow_send_ack$"; then
 		pm_nl_add_endpoint $ns1 10.0.1.1 flags subflow
 		pm_nl_add_endpoint $ns2 10.0.1.2 flags subflow
 		sflags=backup speed=slow \
-- 
2.35.3
Re: [PATCH mptcp-net v2] selftests: mptcp: join: fix subflow_send_ack lookup
Posted by Matthieu Baerts 5 months, 1 week ago 
Hi Geliang,

On 14/11/2023 15:57, Geliang Tang wrote:
> MPC backups tests will skip unexpected sometimes, since static functions
> like mptcp_subflow_send_ack may also be listed in /proc/kallsyms, with a
> 't' in front of it, not 'T' ('T' is for a global function):
> 
>  > grep "mptcp_subflow_send_ack" /proc/kallsyms
> 
>  0000000000000000 T __pfx___mptcp_subflow_send_ack
>  0000000000000000 T __mptcp_subflow_send_ack
>  0000000000000000 t __pfx_mptcp_subflow_send_ack
>  0000000000000000 t mptcp_subflow_send_ack
> 
> In this case, mptcp_lib_kallsyms_doesnt_have "mptcp_subflow_send_ack$"
> will be false, MPC backups tests will skip. This is not what we expected.
> 
> The correct logic here should be: if mptcp_subflow_send_ack is not a
> global function in /proc/kallsyms, do these MPC backups tests. So a 'T'
> must be added in front of mptcp_subflow_send_ack.

With a bit of delay -- sorry for that -- I just applied your patch in
our tree (fixes for -net):

New patches for t/upstream-net and t/upstream:
- 6fb578086535: selftests: mptcp: join: fix subflow_send_ack lookup
- Results: 77d946064d59..8ec51043b53f (export-net)
- Results: 5a37296ad9fd..8fa5c227980d (export)

Tests are now in progress:

https://cirrus-ci.com/github/multipath-tcp/mptcp_net-next/export-net/20231204T170318
https://cirrus-ci.com/github/multipath-tcp/mptcp_net-next/export/20231204T170318

Cheers,
Matt
-- 
Sponsored by the NGI0 Core fund.
Re: [PATCH mptcp-net v2] selftests: mptcp: join: fix subflow_send_ack lookup
Posted by Mat Martineau 5 months, 2 weeks ago 
On Tue, 14 Nov 2023, Geliang Tang wrote:

> MPC backups tests will skip unexpected sometimes, since static functions
> like mptcp_subflow_send_ack may also be listed in /proc/kallsyms, with a
> 't' in front of it, not 'T' ('T' is for a global function):
>
> > grep "mptcp_subflow_send_ack" /proc/kallsyms
>
> 0000000000000000 T __pfx___mptcp_subflow_send_ack
> 0000000000000000 T __mptcp_subflow_send_ack
> 0000000000000000 t __pfx_mptcp_subflow_send_ack
> 0000000000000000 t mptcp_subflow_send_ack
>
> In this case, mptcp_lib_kallsyms_doesnt_have "mptcp_subflow_send_ack$"
> will be false, MPC backups tests will skip. This is not what we expected.
>
> The correct logic here should be: if mptcp_subflow_send_ack is not a
> global function in /proc/kallsyms, do these MPC backups tests. So a 'T'
> must be added in front of mptcp_subflow_send_ack.
>
> Fixes: 632978f0a961 ("selftests: mptcp: join: skip MPC backups tests if not supported")
> Signed-off-by: Geliang Tang <geliang.tang@suse.com>
> ---
> v2:
> - Update commit log, fix typos in it.

v2 looks good, with Geliang's commit message change from the follow-up 
message 
(https://lore.kernel.org/mptcp/20231117053305.GA26319@localhost.localdomain/)

Reviewed-by: Mat Martineau <martineau@kernel.org>

> ---
> tools/testing/selftests/net/mptcp/mptcp_join.sh | 8 ++++----
> 1 file changed, 4 insertions(+), 4 deletions(-)
>
Re: [PATCH mptcp-net v2] selftests: mptcp: join: fix subflow_send_ack lookup
Posted by Geliang Tang 6 months ago 
On Tue, Nov 14, 2023 at 10:57:08PM +0800, Geliang Tang wrote:
> MPC backups tests will skip unexpected sometimes, since static functions
> like mptcp_subflow_send_ack may also be listed in /proc/kallsyms, with a
> 't' in front of it, not 'T' ('T' is for a global function):
> 
>  > grep "mptcp_subflow_send_ack" /proc/kallsyms
> 
>  0000000000000000 T __pfx___mptcp_subflow_send_ack
>  0000000000000000 T __mptcp_subflow_send_ack
>  0000000000000000 t __pfx_mptcp_subflow_send_ack
>  0000000000000000 t mptcp_subflow_send_ack

I did some tests again, and found that whether to list the static
functions in /proc/kallsyms depends on the version of gcc:

Use gcc-8, "t mptcp_subflow_send_ack" is in /proc/kallsyms.

> sudo dmesg | head
[    0.000000] Linux version 6.6.0-mptcp+ (tgl@localhost.localdomain) (gcc (SUSE Linux) 8.5.0, GNU ld (GNU Binutils; SUSE Linux Enterprise 15) 2.39.0.20220810-150100.7.40) #5 SMP PREEMPT_DYNAMIC Thu Nov 16 10:27:33 CST 2023
[    0.000000] Command line: BOOT_IMAGE=/boot/vmlinuz-6.6.0-mptcp+ root=/dev/mapper/vg0-lvol0 splash=silent mitigations=auto quiet
[    0.000000] BIOS-provided physical RAM map:
[    0.000000] BIOS-e820: [mem 0x0000000000000000-0x000000000009efff] usable
[    0.000000] BIOS-e820: [mem 0x000000000009f000-0x00000000000fffff] reserved
[    0.000000] BIOS-e820: [mem 0x0000000000100000-0x000000006a5b6fff] usable
[    0.000000] BIOS-e820: [mem 0x000000006a5b7000-0x000000006e794fff] reserved
[    0.000000] BIOS-e820: [mem 0x000000006e795000-0x000000006fb1efff] ACPI NVS
[    0.000000] BIOS-e820: [mem 0x000000006fb1f000-0x000000006fc4efff] ACPI data
[    0.000000] BIOS-e820: [mem 0x000000006fc4f000-0x000000006fc4ffff] usable

> sudo cat /proc/kallsyms | grep subflow_send_ack
ffffffffa1c0df80 T __pfx___mptcp_subflow_send_ack
ffffffffa1c0df90 T __mptcp_subflow_send_ack
ffffffffa1c0dfd0 t __pfx_mptcp_subflow_send_ack
ffffffffa1c0dfe0 t mptcp_subflow_send_ack

Use gcc-11, no mptcp_subflow_send_ack is in /proc/kallsyms.

> sudo dmesg | head
[sudo] password for root:
[    0.000000] Linux version 6.6.0-mptcp+ (tgl@bogon) (gcc (SUSE Linux) 11.4.1 20230630 [revision f03b182f7f23e14d587cfa1daec13ef7c3044ac0], GNU ld (GNU Binutils; SUSE Linux Enterprise 15) 2.39.0.20220810-150100.7.40) #25 SMP PREEMPT_DYNAMIC Thu Nov 16 22:26:41 CST 2023
[    0.000000] Command line: BOOT_IMAGE=/boot/vmlinuz-6.6.0-mptcp+ root=/dev/mapper/vg0-lvol0 splash=silent mitigations=auto quiet
[    0.000000] BIOS-provided physical RAM map:
[    0.000000] BIOS-e820: [mem 0x0000000000000000-0x000000000009efff] usable
[    0.000000] BIOS-e820: [mem 0x000000000009f000-0x00000000000fffff] reserved
[    0.000000] BIOS-e820: [mem 0x0000000000100000-0x000000006a5b6fff] usable
[    0.000000] BIOS-e820: [mem 0x000000006a5b7000-0x000000006e794fff] reserved
[    0.000000] BIOS-e820: [mem 0x000000006e795000-0x000000006fb1efff] ACPI NVS
[    0.000000] BIOS-e820: [mem 0x000000006fb1f000-0x000000006fc4efff] ACPI data
[    0.000000] BIOS-e820: [mem 0x000000006fc4f000-0x000000006fc4ffff] usable
> sudo cat /proc/kallsyms | grep subflow_send_ack
ffffffffaca54e80 T __pfx___mptcp_subflow_send_ack
ffffffffaca54e90 T __mptcp_subflow_send_ack

So MPC backups tests will skip unexpected when using gcc-8. This patch
does fix this issue. Only commit log needs to be updated:

'''
MPC backups tests will skip unexpected sometimes (For example, when
compiling kernel with an older version of gcc, such as gcc-8), since
static functions like mptcp_subflow_send_ack also be listed in
/proc/kallsyms, with a 't' in front of it, not 'T' ('T' is for a global
function):

 > grep "mptcp_subflow_send_ack" /proc/kallsyms

 0000000000000000 T __pfx___mptcp_subflow_send_ack
 0000000000000000 T __mptcp_subflow_send_ack
 0000000000000000 t __pfx_mptcp_subflow_send_ack
 0000000000000000 t mptcp_subflow_send_ack

In this case, mptcp_lib_kallsyms_doesnt_have "mptcp_subflow_send_ack$"
will be false, MPC backups tests will skip. This is not what we expected.

The correct logic here should be: if mptcp_subflow_send_ack is not a
global function in /proc/kallsyms, do these MPC backups tests. So a 'T'
must be added in front of mptcp_subflow_send_ack.
'''

Thanks,
-Geliang

> 
> In this case, mptcp_lib_kallsyms_doesnt_have "mptcp_subflow_send_ack$"
> will be false, MPC backups tests will skip. This is not what we expected.
> 
> The correct logic here should be: if mptcp_subflow_send_ack is not a
> global function in /proc/kallsyms, do these MPC backups tests. So a 'T'
> must be added in front of mptcp_subflow_send_ack.
> 
> Fixes: 632978f0a961 ("selftests: mptcp: join: skip MPC backups tests if not supported")
> Signed-off-by: Geliang Tang <geliang.tang@suse.com>
> ---
> v2:
>  - Update commit log, fix typos in it.
> ---
>  tools/testing/selftests/net/mptcp/mptcp_join.sh | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/tools/testing/selftests/net/mptcp/mptcp_join.sh b/tools/testing/selftests/net/mptcp/mptcp_join.sh
> index 1606474232f6..6fbc70332caa 100755
> --- a/tools/testing/selftests/net/mptcp/mptcp_join.sh
> +++ b/tools/testing/selftests/net/mptcp/mptcp_join.sh
> @@ -2755,7 +2755,7 @@ backup_tests()
>  	fi
>  
>  	if reset "mpc backup" &&
> -	   continue_if mptcp_lib_kallsyms_doesnt_have "mptcp_subflow_send_ack$"; then
> +	   continue_if mptcp_lib_kallsyms_doesnt_have "T mptcp_subflow_send_ack$"; then
>  		pm_nl_add_endpoint $ns2 10.0.1.2 flags subflow,backup
>  		speed=slow \
>  			run_tests $ns1 $ns2 10.0.1.1
> @@ -2764,7 +2764,7 @@ backup_tests()
>  	fi
>  
>  	if reset "mpc backup both sides" &&
> -	   continue_if mptcp_lib_kallsyms_doesnt_have "mptcp_subflow_send_ack$"; then
> +	   continue_if mptcp_lib_kallsyms_doesnt_have "T mptcp_subflow_send_ack$"; then
>  		pm_nl_add_endpoint $ns1 10.0.1.1 flags subflow,backup
>  		pm_nl_add_endpoint $ns2 10.0.1.2 flags subflow,backup
>  		speed=slow \
> @@ -2774,7 +2774,7 @@ backup_tests()
>  	fi
>  
>  	if reset "mpc switch to backup" &&
> -	   continue_if mptcp_lib_kallsyms_doesnt_have "mptcp_subflow_send_ack$"; then
> +	   continue_if mptcp_lib_kallsyms_doesnt_have "T mptcp_subflow_send_ack$"; then
>  		pm_nl_add_endpoint $ns2 10.0.1.2 flags subflow
>  		sflags=backup speed=slow \
>  			run_tests $ns1 $ns2 10.0.1.1
> @@ -2783,7 +2783,7 @@ backup_tests()
>  	fi
>  
>  	if reset "mpc switch to backup both sides" &&
> -	   continue_if mptcp_lib_kallsyms_doesnt_have "mptcp_subflow_send_ack$"; then
> +	   continue_if mptcp_lib_kallsyms_doesnt_have "T mptcp_subflow_send_ack$"; then
>  		pm_nl_add_endpoint $ns1 10.0.1.1 flags subflow
>  		pm_nl_add_endpoint $ns2 10.0.1.2 flags subflow
>  		sflags=backup speed=slow \
> -- 
> 2.35.3
>

Patchew 2.1-devel-0870f84

© 2016 - 2024 Red Hat, Inc.