Depending on the options used, pmtu.sh may launch tcpdump and nettest processes in the background. However it fails to clean them up after the tests complete.
Patch 1 allows the cleanup() function to read the list of PIDs launched by the tests. Patch 2 fixes the way the nettest PIDs are retrieved.
v2: * Use tcpdump's immediate mode to capture packets even in short lived tests. * Add patch 2 to fix the nettest_pids list.
Guillaume Nault (2): selftests: pmtu.sh: Kill tcpdump processes launched by subshell. selftests: pmtu.sh: Kill nettest processes launched in subshell.
tools/testing/selftests/net/pmtu.sh | 21 +++++++++++++++++---- 1 file changed, 17 insertions(+), 4 deletions(-)
The cleanup() function takes care of killing processes launched by the test functions. It relies on variables like ${tcpdump_pids} to get the relevant PIDs. But tests are run in their own subshell, so updated *_pids values are invisible to other shells. Therefore cleanup() never sees any process to kill:
$ ./tools/testing/selftests/net/pmtu.sh -t pmtu_ipv4_exception TEST: ipv4: PMTU exceptions [ OK ] TEST: ipv4: PMTU exceptions - nexthop objects [ OK ]
$ pgrep -af tcpdump 6084 tcpdump -s 0 -i veth_A-R1 -w pmtu_ipv4_exception_veth_A-R1.pcap 6085 tcpdump -s 0 -i veth_R1-A -w pmtu_ipv4_exception_veth_R1-A.pcap 6086 tcpdump -s 0 -i veth_R1-B -w pmtu_ipv4_exception_veth_R1-B.pcap 6087 tcpdump -s 0 -i veth_B-R1 -w pmtu_ipv4_exception_veth_B-R1.pcap 6088 tcpdump -s 0 -i veth_A-R2 -w pmtu_ipv4_exception_veth_A-R2.pcap 6089 tcpdump -s 0 -i veth_R2-A -w pmtu_ipv4_exception_veth_R2-A.pcap 6090 tcpdump -s 0 -i veth_R2-B -w pmtu_ipv4_exception_veth_R2-B.pcap 6091 tcpdump -s 0 -i veth_B-R2 -w pmtu_ipv4_exception_veth_B-R2.pcap 6228 tcpdump -s 0 -i veth_A-R1 -w pmtu_ipv4_exception_veth_A-R1.pcap 6229 tcpdump -s 0 -i veth_R1-A -w pmtu_ipv4_exception_veth_R1-A.pcap 6230 tcpdump -s 0 -i veth_R1-B -w pmtu_ipv4_exception_veth_R1-B.pcap 6231 tcpdump -s 0 -i veth_B-R1 -w pmtu_ipv4_exception_veth_B-R1.pcap 6232 tcpdump -s 0 -i veth_A-R2 -w pmtu_ipv4_exception_veth_A-R2.pcap 6233 tcpdump -s 0 -i veth_R2-A -w pmtu_ipv4_exception_veth_R2-A.pcap 6234 tcpdump -s 0 -i veth_R2-B -w pmtu_ipv4_exception_veth_R2-B.pcap 6235 tcpdump -s 0 -i veth_B-R2 -w pmtu_ipv4_exception_veth_B-R2.pcap
Fix this by running cleanup() in the context of the test subshell. Now that each test cleans the environment after completion, there's no need for calling cleanup() again when the next test starts. So let's drop it from the setup() function. This is okay because cleanup() is also called when pmtu.sh starts, so even the first test starts in a clean environment.
Also, use tcpdump's immediate mode. Otherwise it might not have time to process buffered packets, resulting in missing packets or even empty pcap files for short tests.
Note: PAUSE_ON_FAIL is still evaluated before cleanup(), so one can still inspect the test environment upon failure when using -p.
Fixes: a92a0a7b8e7c ("selftests: pmtu: Simplify cleanup and namespace names") Signed-off-by: Guillaume Nault gnault@redhat.com --- tools/testing/selftests/net/pmtu.sh | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-)
diff --git a/tools/testing/selftests/net/pmtu.sh b/tools/testing/selftests/net/pmtu.sh index 543ad7513a8e..2e8972573d91 100755 --- a/tools/testing/selftests/net/pmtu.sh +++ b/tools/testing/selftests/net/pmtu.sh @@ -865,7 +865,6 @@ setup_ovs_bridge() { setup() { [ "$(id -u)" -ne 0 ] && echo " need to run as root" && return $ksft_skip
- cleanup for arg do eval setup_${arg} || { echo " ${arg} not supported"; return 1; } done @@ -876,7 +875,7 @@ trace() {
for arg do [ "${ns_cmd}" = "" ] && ns_cmd="${arg}" && continue - ${ns_cmd} tcpdump -s 0 -i "${arg}" -w "${name}_${arg}.pcap" 2> /dev/null & + ${ns_cmd} tcpdump --immediate-mode -s 0 -i "${arg}" -w "${name}_${arg}.pcap" 2> /dev/null & tcpdump_pids="${tcpdump_pids} $!" ns_cmd= done @@ -1836,6 +1835,10 @@ run_test() {
unset IFS
+ # Since cleanup() relies on variables modified by this subshell, it + # has to run in this context. + trap cleanup EXIT + if [ "$VERBOSE" = "1" ]; then printf "\n##########################################################################\n\n" fi
When using "run_cmd <command> &", then "$!" refers to the PID of the subshell used to run <command>, not the command itself. Therefore nettest_pids actually doesn't contain the list of the nettest commands running in the background. So cleanup() can't kill them and the nettest processes run until completion (fortunately they have a 5s timeout).
Fix this by defining a new command for running processes in the background, for which "$!" really refers to the PID of the command run.
Also, double quote variables on the modified lines, to avoid shellcheck warnings.
Fixes: ece1278a9b81 ("selftests: net: add ESP-in-UDP PMTU test") Signed-off-by: Guillaume Nault gnault@redhat.com --- tools/testing/selftests/net/pmtu.sh | 14 ++++++++++++-- 1 file changed, 12 insertions(+), 2 deletions(-)
diff --git a/tools/testing/selftests/net/pmtu.sh b/tools/testing/selftests/net/pmtu.sh index 2e8972573d91..694732e4b344 100755 --- a/tools/testing/selftests/net/pmtu.sh +++ b/tools/testing/selftests/net/pmtu.sh @@ -374,6 +374,16 @@ run_cmd() { return $rc }
+run_cmd_bg() { + cmd="$*" + + if [ "$VERBOSE" = "1" ]; then + printf " COMMAND: %s &\n" "${cmd}" + fi + + $cmd 2>&1 & +} + # Find the auto-generated name for this namespace nsname() { eval echo $NS_$1 @@ -670,10 +680,10 @@ setup_nettest_xfrm() { [ ${1} -eq 6 ] && proto="-6" || proto="" port=${2}
- run_cmd ${ns_a} nettest ${proto} -q -D -s -x -p ${port} -t 5 & + run_cmd_bg "${ns_a}" nettest "${proto}" -q -D -s -x -p "${port}" -t 5 nettest_pids="${nettest_pids} $!"
- run_cmd ${ns_b} nettest ${proto} -q -D -s -x -p ${port} -t 5 & + run_cmd_bg "${ns_b}" nettest "${proto}" -q -D -s -x -p "${port}" -t 5 nettest_pids="${nettest_pids} $!" }
On 3/8/22 3:14 PM, Guillaume Nault wrote:
Depending on the options used, pmtu.sh may launch tcpdump and nettest processes in the background. However it fails to clean them up after the tests complete.
Patch 1 allows the cleanup() function to read the list of PIDs launched by the tests. Patch 2 fixes the way the nettest PIDs are retrieved.
v2:
- Use tcpdump's immediate mode to capture packets even in short lived tests.
- Add patch 2 to fix the nettest_pids list.
Guillaume Nault (2): selftests: pmtu.sh: Kill tcpdump processes launched by subshell. selftests: pmtu.sh: Kill nettest processes launched in subshell.
tools/testing/selftests/net/pmtu.sh | 21 +++++++++++++++++---- 1 file changed, 17 insertions(+), 4 deletions(-)
Both of these look good to me. One nit on commit header. Please include net in the patch subject line in the future.
e.g: selftests:net pmtu.sh
Tested them on my system. Seeing these messages even after building nettest:
'nettest' command not found; skipping tests xfrm6udp not supported TEST: vti6: PMTU exceptions (ESP-in-UDP) [SKIP] 'nettest' command not found; skipping tests xfrm4udp not supported TEST: vti4: PMTU exceptions (ESP-in-UDP) [SKIP] 'nettest' command not found; skipping tests xfrm6udprouted not supported TEST: vti6: PMTU exceptions, routed (ESP-in-UDP) [SKIP] 'nettest' command not found; skipping tests xfrm4udprouted not supported
Might not be related to this patch though. I jusr ran pmtu.sh from net directory.
Reviewed-by: Shuah Khan skhan@linuxfoundation.org
thanks, -- Shuah
On Tue, Mar 08, 2022 at 04:51:46PM -0700, Shuah Khan wrote:
On 3/8/22 3:14 PM, Guillaume Nault wrote:
Depending on the options used, pmtu.sh may launch tcpdump and nettest processes in the background. However it fails to clean them up after the tests complete.
Patch 1 allows the cleanup() function to read the list of PIDs launched by the tests. Patch 2 fixes the way the nettest PIDs are retrieved.
v2:
- Use tcpdump's immediate mode to capture packets even in short lived tests.
- Add patch 2 to fix the nettest_pids list.
Guillaume Nault (2): selftests: pmtu.sh: Kill tcpdump processes launched by subshell. selftests: pmtu.sh: Kill nettest processes launched in subshell.
tools/testing/selftests/net/pmtu.sh | 21 +++++++++++++++++---- 1 file changed, 17 insertions(+), 4 deletions(-)
Both of these look good to me. One nit on commit header. Please include net in the patch subject line in the future.
e.g: selftests:net pmtu.sh
Thanks, I'll do that next time (I just reused keywords used by other commits for this file).
Tested them on my system. Seeing these messages even after building nettest:
'nettest' command not found; skipping tests xfrm6udp not supported TEST: vti6: PMTU exceptions (ESP-in-UDP) [SKIP] 'nettest' command not found; skipping tests xfrm4udp not supported TEST: vti4: PMTU exceptions (ESP-in-UDP) [SKIP] 'nettest' command not found; skipping tests xfrm6udprouted not supported TEST: vti6: PMTU exceptions, routed (ESP-in-UDP) [SKIP] 'nettest' command not found; skipping tests xfrm4udprouted not supported
Might not be related to this patch though. I jusr ran pmtu.sh from net directory.
Personally I just modified my PATH before running pmtu.sh. Not sure if there's a better way. But in any case, that's not related to this patch.
Reviewed-by: Shuah Khan skhan@linuxfoundation.org
thanks, -- Shuah
Hello:
This series was applied to netdev/net.git (master) by Jakub Kicinski kuba@kernel.org:
On Tue, 8 Mar 2022 23:14:57 +0100 you wrote:
Depending on the options used, pmtu.sh may launch tcpdump and nettest processes in the background. However it fails to clean them up after the tests complete.
Patch 1 allows the cleanup() function to read the list of PIDs launched by the tests. Patch 2 fixes the way the nettest PIDs are retrieved.
[...]
Here is the summary with links: - [net,1/2] selftests: pmtu.sh: Kill tcpdump processes launched by subshell. https://git.kernel.org/netdev/net/c/18dfc667550f - [net,2/2] selftests: pmtu.sh: Kill nettest processes launched in subshell. https://git.kernel.org/netdev/net/c/94a4a4fe4c69
You are awesome, thank you!
linux-kselftest-mirror@lists.linaro.org
-
Guillaume Nault -
patchwork-bot+netdevbpf＠kernel.org -
Shuah Khan