Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:2 as permitted sender) client-ip=2620:137:e000::3:2;
Message-ID: <e8a55d0518da5c1f9aba739359150cad58c03b2b.camel@redhat.com>
Subject: Re: [PATCH v2] selftests/net: synchronize udpgso_bench rx and tx
From:   Paolo Abeni <pabeni@redhat.com>
To:     Lucas Karpinski <lkarpins@redhat.com>, davem@davemloft.net,
        edumazet@google.com, kuba@kernel.org, shuah@kernel.org
Cc:     netdev@vger.kernel.org, linux-kselftest@vger.kernel.org,
        linux-kernel@vger.kernel.org
Date:   Tue, 31 Oct 2023 10:18:28 +0100
In-Reply-To: <6ceki76bcv7qz6de5rxc26ot6aezdmeoz2g4ubtve7qwozmyyw@zibbg64wsdjp>
References: <6ceki76bcv7qz6de5rxc26ot6aezdmeoz2g4ubtve7qwozmyyw@zibbg64wsdjp>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
User-Agent: Evolution 3.46.4 (3.46.4-1.fc37) 
MIME-Version: 1.0
Precedence: bulk

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.
>=20
> Suggested-by: Paolo Abeni <pabeni@redhat.com>
> Signed-off-by: Lucas Karpinski <lkarpins@redhat.com>
> ---
> https://lore.kernel.org/all/t7v6mmuobrbucyfpwqbcujtvpa3wxnsrc36cz5rr6kzzr=
zkwtj@toz6mr4ggnyp/
>=20
> Changelog v2:=20
> - applied synchronization method suggested by Pablo
> - changed commit message to code=20
>=20
>  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(-)
>=20
> diff --git a/tools/testing/selftests/net/udpgro.sh b/tools/testing/selfte=
sts/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
> =20
> +wait_local_port_listen()
> +{
> +	local port=3D"${1}"
> +
> +	local port_hex
> +	port_hex=3D"$(printf "%04X" "${port}")"
> +
> +	local i

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

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

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

> +	for i in $(seq 10); do
> +		ip netns exec "${PEER_NS}" cat /proc/net/udp* | \
> +			awk "BEGIN {rc=3D1} {if (\$2 ~ /:${port_hex}\$/) {rc=3D0; 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