[v2] selftests/net: synchronize udpgso_bench rx and tx

  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(-)
  

On Mon, 2023-10-30 at 13:40 -0400, 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(-)
> 
> 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

Minor nit: I think the code would be more readable, if you will group
the variable together:

	local port="${1}"
	local port_hex
	local i

	port_hex="$(printf "%04X" "${port}")"
	# ...

> +	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
> +}

Since you wrote the same function verbatim in 3 different files, I
think it would be better place it in separate, new, net_helper.sh file
and include such file from the various callers. Possibly additionally
rename the function as wait_local_udp_port_listen.

Thanks!

Paolo
  

On Mon, 2023-10-30 at 13:40 -0400, 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/
> 
I almost forgot ...
> Changelog v2: 
> - applied synchronization method suggested by Pablo
                                                ^^^^^ most common typo
since match 2022 ;)

Less irrelevant, please include the target tree in the next submission,
in this case 'net-next'.

Thanks,

Paolo
  

> Since you wrote the same function verbatim in 3 different files, I
> think it would be better place it in separate, new, net_helper.sh file
> and include such file from the various callers. Possibly additionally
> rename the function as wait_local_udp_port_listen.
>
Thanks, I'll move it over. I think it would be best though to leave udp
out of the name and to just pass the protocol as an argument. That way
any future tcp tests can also take advantage of it.

Lucas
  

On Tue, 2023-10-31 at 09:26 -0400, Lucas Karpinski wrote:
> > Since you wrote the same function verbatim in 3 different files, I
> > think it would be better place it in separate, new, net_helper.sh
> > file
> > and include such file from the various callers. Possibly
> > additionally
> > rename the function as wait_local_udp_port_listen.
> > 
> Thanks, I'll move it over. I think it would be best though to leave
> udp out of the name and to just pass the protocol as an argument.

Indeed. I suggested the other option just to keep it the simpler, but
if you have time and will, please go ahead!

Cheers,

Paolo
  

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
>
  

On Tue, Oct 31, 2023 at 05:11:59PM -0400, Willem de Bruijn wrote:
> 
> 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.
> 
The patch synchronizes the connection between the two binaries;
udpgso_bench_rx and udpgso_bench_tx, which are launched by the udpgro
tests. I can remove their names and just specify "synchronize udpgro
tests' tx and rx connection." 
> 
> 
> Might a grep be shorter and more readable?
> 
Noted, will change it.

Lucas