| Message ID | 20230131130412.432549-3-andrei.gherzan@canonical.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [v2,1/4] selftests: net: udpgso_bench_rx: Fix 'used uninitialized' compiler warning | expand |
On Tue, Jan 31, 2023 at 8:06 AM Andrei Gherzan <andrei.gherzan@canonical.com> wrote: > > "udpgro_bench.sh" invokes udpgso_bench_rx/udpgso_bench_tx programs > subsequently and while doing so, there is a chance that the rx one is not > ready to accept socket connections. This racing bug could fail the test > with at least one of the following: > > ./udpgso_bench_tx: connect: Connection refused > ./udpgso_bench_tx: sendmsg: Connection refused > ./udpgso_bench_tx: write: Connection refused > > This change addresses this by making udpgro_bench.sh wait for the rx > program to be ready before firing off the tx one - with an exponential back > off algorithm from 1s to 10s. > > Signed-off-by: Andrei Gherzan <andrei.gherzan@canonical.com> please CC: reviewers of previous revisions on new revisions also for upcoming patches: please clearly mark net or net-next. > --- > tools/testing/selftests/net/udpgso_bench.sh | 18 ++++++++++++++++++ > 1 file changed, 18 insertions(+) > > diff --git a/tools/testing/selftests/net/udpgso_bench.sh b/tools/testing/selftests/net/udpgso_bench.sh > index dc932fd65363..20b5db8fcbde 100755 > --- a/tools/testing/selftests/net/udpgso_bench.sh > +++ b/tools/testing/selftests/net/udpgso_bench.sh > @@ -7,6 +7,7 @@ readonly GREEN='\033[0;92m' > readonly YELLOW='\033[0;33m' > readonly RED='\033[0;31m' > readonly NC='\033[0m' # No Color > +readonly TESTPORT=8000 # Keep this in sync with udpgso_bench_rx/tx then also pass explicit -p argument to the processes to keep all three consistent > > readonly KSFT_PASS=0 > readonly KSFT_FAIL=1 > @@ -56,10 +57,27 @@ trap wake_children EXIT > > run_one() { > local -r args=$@ > + local -r init_delay_s=1 > + local -r max_delay_s=10 > + local delay_s=0 > + local nr_socks=0 > > ./udpgso_bench_rx & > ./udpgso_bench_rx -t & > > + # Wait for the above test program to get ready to receive connections. > + delay_s="${init_delay_s}" > + while [ "$delay_s" -lt "$max_delay_s" ]; do > + nr_socks="$(ss -lnHi | grep -c "\*:${TESTPORT}")" > + [ "$nr_socks" -eq 2 ] && break > + sleep "$delay_s" > + delay="$((delay*2))" I don't think we need exponential back-off for something this simple > + done > + if [ "$nr_socks" -ne 2 ]; then > + echo "timed out while waiting for udpgso_bench_rx" > + exit 1 > + fi > + > ./udpgso_bench_tx ${args} > } > > -- > 2.34.1 >
On Tue, 2023-01-31 at 08:33 -0500, Willem de Bruijn wrote: > On Tue, Jan 31, 2023 at 8:06 AM Andrei Gherzan > <andrei.gherzan@canonical.com> wrote: > > > > "udpgro_bench.sh" invokes udpgso_bench_rx/udpgso_bench_tx programs > > subsequently and while doing so, there is a chance that the rx one is not > > ready to accept socket connections. This racing bug could fail the test > > with at least one of the following: > > > > ./udpgso_bench_tx: connect: Connection refused > > ./udpgso_bench_tx: sendmsg: Connection refused > > ./udpgso_bench_tx: write: Connection refused > > > > This change addresses this by making udpgro_bench.sh wait for the rx > > program to be ready before firing off the tx one - with an exponential back > > off algorithm from 1s to 10s. > > > > Signed-off-by: Andrei Gherzan <andrei.gherzan@canonical.com> > > please CC: reviewers of previous revisions on new revisions > > also for upcoming patches: please clearly mark net or net-next. > > --- > > tools/testing/selftests/net/udpgso_bench.sh | 18 ++++++++++++++++++ > > 1 file changed, 18 insertions(+) > > > > diff --git a/tools/testing/selftests/net/udpgso_bench.sh b/tools/testing/selftests/net/udpgso_bench.sh > > index dc932fd65363..20b5db8fcbde 100755 > > --- a/tools/testing/selftests/net/udpgso_bench.sh > > +++ b/tools/testing/selftests/net/udpgso_bench.sh > > @@ -7,6 +7,7 @@ readonly GREEN='\033[0;92m' > > readonly YELLOW='\033[0;33m' > > readonly RED='\033[0;31m' > > readonly NC='\033[0m' # No Color > > +readonly TESTPORT=8000 # Keep this in sync with udpgso_bench_rx/tx > > then also pass explicit -p argument to the processes to keep all three > consistent > > > > > readonly KSFT_PASS=0 > > readonly KSFT_FAIL=1 > > @@ -56,10 +57,27 @@ trap wake_children EXIT > > > > run_one() { > > local -r args=$@ > > + local -r init_delay_s=1 > > + local -r max_delay_s=10 > > + local delay_s=0 > > + local nr_socks=0 > > > > ./udpgso_bench_rx & > > ./udpgso_bench_rx -t & > > > > + # Wait for the above test program to get ready to receive connections. > > + delay_s="${init_delay_s}" > > + while [ "$delay_s" -lt "$max_delay_s" ]; do > > + nr_socks="$(ss -lnHi | grep -c "\*:${TESTPORT}")" > > + [ "$nr_socks" -eq 2 ] && break > > + sleep "$delay_s" > > + delay="$((delay*2))" > > I don't think we need exponential back-off for something this simple Agreed. Additionally you could use constant, sub-second delay (say 0.1) to keep the runtime delta relatively low. Cheers, Paolo
On 23/01/31 08:33AM, Willem de Bruijn wrote: > On Tue, Jan 31, 2023 at 8:06 AM Andrei Gherzan > <andrei.gherzan@canonical.com> wrote: > > > > "udpgro_bench.sh" invokes udpgso_bench_rx/udpgso_bench_tx programs > > subsequently and while doing so, there is a chance that the rx one is not > > ready to accept socket connections. This racing bug could fail the test > > with at least one of the following: > > > > ./udpgso_bench_tx: connect: Connection refused > > ./udpgso_bench_tx: sendmsg: Connection refused > > ./udpgso_bench_tx: write: Connection refused > > > > This change addresses this by making udpgro_bench.sh wait for the rx > > program to be ready before firing off the tx one - with an exponential back > > off algorithm from 1s to 10s. > > > > Signed-off-by: Andrei Gherzan <andrei.gherzan@canonical.com> > > please CC: reviewers of previous revisions on new revisions > > also for upcoming patches: please clearly mark net or net-next. Ack. I'll do all that in next v. > > --- > > tools/testing/selftests/net/udpgso_bench.sh | 18 ++++++++++++++++++ > > 1 file changed, 18 insertions(+) > > > > diff --git a/tools/testing/selftests/net/udpgso_bench.sh b/tools/testing/selftests/net/udpgso_bench.sh > > index dc932fd65363..20b5db8fcbde 100755 > > --- a/tools/testing/selftests/net/udpgso_bench.sh > > +++ b/tools/testing/selftests/net/udpgso_bench.sh > > @@ -7,6 +7,7 @@ readonly GREEN='\033[0;92m' > > readonly YELLOW='\033[0;33m' > > readonly RED='\033[0;31m' > > readonly NC='\033[0m' # No Color > > +readonly TESTPORT=8000 # Keep this in sync with udpgso_bench_rx/tx > > then also pass explicit -p argument to the processes to keep all three > consistent I have no idea how I've missed it. It will be in v4. > > > > > readonly KSFT_PASS=0 > > readonly KSFT_FAIL=1 > > @@ -56,10 +57,27 @@ trap wake_children EXIT > > > > run_one() { > > local -r args=$@ > > + local -r init_delay_s=1 > > + local -r max_delay_s=10 > > + local delay_s=0 > > + local nr_socks=0 > > > > ./udpgso_bench_rx & > > ./udpgso_bench_rx -t & > > > > + # Wait for the above test program to get ready to receive connections. > > + delay_s="${init_delay_s}" > > + while [ "$delay_s" -lt "$max_delay_s" ]; do > > + nr_socks="$(ss -lnHi | grep -c "\*:${TESTPORT}")" > > + [ "$nr_socks" -eq 2 ] && break > > + sleep "$delay_s" > > + delay="$((delay*2))" > > I don't think we need exponential back-off for something this simple I'm happy to drop/simplify. I'll go for a simple sleep 1 in a timeout loop. > > > + done > > + if [ "$nr_socks" -ne 2 ]; then > > + echo "timed out while waiting for udpgso_bench_rx" > > + exit 1 > > + fi > > + > > ./udpgso_bench_tx ${args} > > } > > > > -- > > 2.34.1 > >
diff --git a/tools/testing/selftests/net/udpgso_bench.sh b/tools/testing/selftests/net/udpgso_bench.sh index dc932fd65363..20b5db8fcbde 100755 --- a/tools/testing/selftests/net/udpgso_bench.sh +++ b/tools/testing/selftests/net/udpgso_bench.sh @@ -7,6 +7,7 @@ readonly GREEN='\033[0;92m' readonly YELLOW='\033[0;33m' readonly RED='\033[0;31m' readonly NC='\033[0m' # No Color +readonly TESTPORT=8000 # Keep this in sync with udpgso_bench_rx/tx readonly KSFT_PASS=0 readonly KSFT_FAIL=1 @@ -56,10 +57,27 @@ trap wake_children EXIT run_one() { local -r args=$@ + local -r init_delay_s=1 + local -r max_delay_s=10 + local delay_s=0 + local nr_socks=0 ./udpgso_bench_rx & ./udpgso_bench_rx -t & + # Wait for the above test program to get ready to receive connections. + delay_s="${init_delay_s}" + while [ "$delay_s" -lt "$max_delay_s" ]; do + nr_socks="$(ss -lnHi | grep -c "\*:${TESTPORT}")" + [ "$nr_socks" -eq 2 ] && break + sleep "$delay_s" + delay="$((delay*2))" + done + if [ "$nr_socks" -ne 2 ]; then + echo "timed out while waiting for udpgso_bench_rx" + exit 1 + fi + ./udpgso_bench_tx ${args} }
"udpgro_bench.sh" invokes udpgso_bench_rx/udpgso_bench_tx programs subsequently and while doing so, there is a chance that the rx one is not ready to accept socket connections. This racing bug could fail the test with at least one of the following: ./udpgso_bench_tx: connect: Connection refused ./udpgso_bench_tx: sendmsg: Connection refused ./udpgso_bench_tx: write: Connection refused This change addresses this by making udpgro_bench.sh wait for the rx program to be ready before firing off the tx one - with an exponential back off algorithm from 1s to 10s. Signed-off-by: Andrei Gherzan <andrei.gherzan@canonical.com> --- tools/testing/selftests/net/udpgso_bench.sh | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+)