Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.35 as permitted sender) client-ip=23.128.96.35;
Date:   Tue, 31 Oct 2023 17:11:59 -0400
From:   Willem de Bruijn <willemdebruijn.kernel@gmail.com>
To:     Lucas Karpinski <lkarpins@redhat.com>, davem@davemloft.net,
        edumazet@google.com, kuba@kernel.org, pabeni@redhat.com,
        shuah@kernel.org
Cc:     netdev@vger.kernel.org, linux-kselftest@vger.kernel.org,
        linux-kernel@vger.kernel.org
Message-ID: <65416d9fc1024_bcdbc29418@willemb.c.googlers.com.notmuch>
In-Reply-To: <6ceki76bcv7qz6de5rxc26ot6aezdmeoz2g4ubtve7qwozmyyw@zibbg64wsdjp>
References: <6ceki76bcv7qz6de5rxc26ot6aezdmeoz2g4ubtve7qwozmyyw@zibbg64wsdjp>
Subject: Re: [PATCH v2] selftests/net: synchronize udpgso_bench rx and tx
Mime-Version: 1.0
Content-Type: text/plain;
 charset=utf-8
Content-Transfer-Encoding: 7bit
Precedence: bulk

Lucas Karpinski wrote:
> The sockets used by udpgso_bench_tx aren't always ready when
> udpgso_bench_tx transmits packets. This issue is more prevalent in -rt
> kernels, but can occur in both. Replace the hacky sleep calls with a
> function that checks whether the ports in the namespace are ready for
> use.
> 
> Suggested-by: Paolo Abeni <pabeni@redhat.com>
> Signed-off-by: Lucas Karpinski <lkarpins@redhat.com>
> ---
> https://lore.kernel.org/all/t7v6mmuobrbucyfpwqbcujtvpa3wxnsrc36cz5rr6kzzrzkwtj@toz6mr4ggnyp/
> 
> Changelog v2: 
> - applied synchronization method suggested by Pablo
> - changed commit message to code 
> 
>  tools/testing/selftests/net/udpgro.sh         | 27 ++++++++++++++-----
>  tools/testing/selftests/net/udpgro_bench.sh   | 19 +++++++++++--
>  tools/testing/selftests/net/udpgro_frglist.sh | 19 +++++++++++--
>  3 files changed, 54 insertions(+), 11 deletions(-)

The patch subject mentions UDP GSO, but the patch fixes the udpgro
scripts.

There are separate udpgso testcases. So you probably want to s/gso/gro.


> diff --git a/tools/testing/selftests/net/udpgro.sh b/tools/testing/selftests/net/udpgro.sh
> index 0c743752669a..04792a315729 100755
> --- a/tools/testing/selftests/net/udpgro.sh
> +++ b/tools/testing/selftests/net/udpgro.sh
> @@ -24,6 +24,22 @@ cleanup() {
>  }
>  trap cleanup EXIT
>  
> +wait_local_port_listen()
> +{
> +	local port="${1}"
> +
> +	local port_hex
> +	port_hex="$(printf "%04X" "${port}")"
> +
> +	local i
> +	for i in $(seq 10); do
> +		ip netns exec "${PEER_NS}" cat /proc/net/udp* | \
> +			awk "BEGIN {rc=1} {if (\$2 ~ /:${port_hex}\$/) {rc=0; exit}} END {exit rc}" &&

Might a grep be shorter and more readable?

> +			break
> +		sleep 0.1
> +	done
> +}
> +
>  cfg_veth() {
>  	ip netns add "${PEER_NS}"
>  	ip -netns "${PEER_NS}" link set lo up
> @@ -51,8 +67,7 @@ run_one() {
>  		echo "ok" || \
>  		echo "failed" &
>  
> -	# Hack: let bg programs complete the startup
> -	sleep 0.2
> +	wait_local_port_listen 8000
>  	./udpgso_bench_tx ${tx_args}
>  	ret=$?
>  	wait $(jobs -p)
> @@ -97,7 +112,7 @@ run_one_nat() {
>  		echo "ok" || \
>  		echo "failed"&
>  
> -	sleep 0.1
> +	wait_local_port_listen 8000
>  	./udpgso_bench_tx ${tx_args}
>  	ret=$?
>  	kill -INT $pid
> @@ -118,11 +133,9 @@ run_one_2sock() {
>  		echo "ok" || \
>  		echo "failed" &
>  
> -	# Hack: let bg programs complete the startup
> -	sleep 0.2
> +	wait_local_port_listen 12345
>  	./udpgso_bench_tx ${tx_args} -p 12345
> -	sleep 0.1
> -	# first UDP GSO socket should be closed at this point
> +	wait_local_port_listen 8000
>  	./udpgso_bench_tx ${tx_args}
>  	ret=$?
>  	wait $(jobs -p)
> diff --git a/tools/testing/selftests/net/udpgro_bench.sh b/tools/testing/selftests/net/udpgro_bench.sh
> index 894972877e8b..91833518e80b 100755
> --- a/tools/testing/selftests/net/udpgro_bench.sh
> +++ b/tools/testing/selftests/net/udpgro_bench.sh
> @@ -16,6 +16,22 @@ cleanup() {
>  }
>  trap cleanup EXIT
>  
> +wait_local_port_listen()
> +{
> +	local port="${1}"
> +
> +	local port_hex
> +	port_hex="$(printf "%04X" "${port}")"
> +
> +	local i
> +	for i in $(seq 10); do
> +		ip netns exec "${PEER_NS}" cat /proc/net/udp* | \
> +			awk "BEGIN {rc=1} {if (\$2 ~ /:${port_hex}\$/) {rc=0; exit}} END {exit rc}" &&
> +			break
> +		sleep 0.1
> +	done
> +}
> +
>  run_one() {
>  	# use 'rx' as separator between sender args and receiver args
>  	local -r all="$@"
> @@ -40,8 +56,7 @@ run_one() {
>  	ip netns exec "${PEER_NS}" ./udpgso_bench_rx ${rx_args} -r &
>  	ip netns exec "${PEER_NS}" ./udpgso_bench_rx -t ${rx_args} -r &
>  
> -	# Hack: let bg programs complete the startup
> -	sleep 0.2
> +	wait_local_port_listen 8000
>  	./udpgso_bench_tx ${tx_args}
>  }
>  
> diff --git a/tools/testing/selftests/net/udpgro_frglist.sh b/tools/testing/selftests/net/udpgro_frglist.sh
> index 0a6359bed0b9..0aa2068f122c 100755
> --- a/tools/testing/selftests/net/udpgro_frglist.sh
> +++ b/tools/testing/selftests/net/udpgro_frglist.sh
> @@ -16,6 +16,22 @@ cleanup() {
>  }
>  trap cleanup EXIT
>  
> +wait_local_port_listen()
> +{
> +	local port="${1}"
> +
> +	local port_hex
> +	port_hex="$(printf "%04X" "${port}")"
> +
> +	local i
> +	for i in $(seq 10); do
> +		ip netns exec "${PEER_NS}" cat /proc/net/udp* | \
> +			awk "BEGIN {rc=1} {if (\$2 ~ /:${port_hex}\$/) {rc=0; exit}} END {exit rc}" &&
> +			break
> +		sleep 0.1
> +	done
> +}
> +
>  run_one() {
>  	# use 'rx' as separator between sender args and receiver args
>  	local -r all="$@"
> @@ -45,8 +61,7 @@ run_one() {
>          echo ${rx_args}
>  	ip netns exec "${PEER_NS}" ./udpgso_bench_rx ${rx_args} -r &
>  
> -	# Hack: let bg programs complete the startup
> -	sleep 0.2
> +	wait_local_port_listen 8000
>  	./udpgso_bench_tx ${tx_args}
>  }
>  
> -- 
> 2.41.0
>