* [PATCH 1/7] test/perf: Always use /sbin/sysctl in tcp test
2022-09-21 20:55 [PATCH 0/7] Fixes and workarounds for tests, Coverity warnings Stefano Brivio
@ 2022-09-21 20:55 ` Stefano Brivio
2022-09-22 6:28 ` David Gibson
2022-09-21 20:55 ` [PATCH 2/7] test/perf: Switch performance test duration to 10 seconds instead of 30 Stefano Brivio
` (5 subsequent siblings)
6 siblings, 1 reply; 19+ messages in thread
From: Stefano Brivio @ 2022-09-21 20:55 UTC (permalink / raw)
To: passt-dev
[-- Attachment #1: Type: text/plain, Size: 1676 bytes --]
Signed-off-by: Stefano Brivio <sbrivio(a)redhat.com>
---
test/perf/passt_tcp | 6 +++---
test/perf/pasta_tcp | 6 +++---
2 files changed, 6 insertions(+), 6 deletions(-)
diff --git a/test/perf/passt_tcp b/test/perf/passt_tcp
index f0cfa1b..bee4f9e 100644
--- a/test/perf/passt_tcp
+++ b/test/perf/passt_tcp
@@ -30,9 +30,9 @@ guest /sbin/sysctl -w net.ipv4.tcp_rmem="4096 131072 268435456"
guest /sbin/sysctl -w net.ipv4.tcp_wmem="4096 131072 268435456"
guest /sbin/sysctl -w net.ipv4.tcp_timestamps=0
-ns sysctl -w net.ipv4.tcp_rmem="4096 524288 134217728"
-ns sysctl -w net.ipv4.tcp_wmem="4096 524288 134217728"
-ns sysctl -w net.ipv4.tcp_timestamps=0
+ns /sbin/sysctl -w net.ipv4.tcp_rmem="4096 524288 134217728"
+ns /sbin/sysctl -w net.ipv4.tcp_wmem="4096 524288 134217728"
+ns /sbin/sysctl -w net.ipv4.tcp_timestamps=0
gout GW ip -j -4 route show|jq -rM '.[] | select(.dst == "default").gateway'
gout GW6 ip -j -6 route show|jq -rM '.[] | select(.dst == "default").gateway'
diff --git a/test/perf/pasta_tcp b/test/perf/pasta_tcp
index 602ce52..cc21075 100644
--- a/test/perf/pasta_tcp
+++ b/test/perf/pasta_tcp
@@ -16,9 +16,9 @@ nstools /sbin/sysctl nproc ip seq sleep iperf3 tcp_rr tcp_crr jq sed
test pasta: throughput and latency (local connections)
-ns sysctl -w net.ipv4.tcp_rmem="131072 524288 134217728"
-ns sysctl -w net.ipv4.tcp_wmem="131072 524288 134217728"
-ns sysctl -w net.ipv4.tcp_timestamps=0
+ns /sbin/sysctl -w net.ipv4.tcp_rmem="131072 524288 134217728"
+ns /sbin/sysctl -w net.ipv4.tcp_wmem="131072 524288 134217728"
+ns /sbin/sysctl -w net.ipv4.tcp_timestamps=0
set THREADS 2
--
@@ -16,9 +16,9 @@ nstools /sbin/sysctl nproc ip seq sleep iperf3 tcp_rr tcp_crr jq sed
test pasta: throughput and latency (local connections)
-ns sysctl -w net.ipv4.tcp_rmem="131072 524288 134217728"
-ns sysctl -w net.ipv4.tcp_wmem="131072 524288 134217728"
-ns sysctl -w net.ipv4.tcp_timestamps=0
+ns /sbin/sysctl -w net.ipv4.tcp_rmem="131072 524288 134217728"
+ns /sbin/sysctl -w net.ipv4.tcp_wmem="131072 524288 134217728"
+ns /sbin/sysctl -w net.ipv4.tcp_timestamps=0
set THREADS 2
--
2.35.1
^ permalink raw reply related [flat|nested] 19+ messages in thread
* Re: [PATCH 1/7] test/perf: Always use /sbin/sysctl in tcp test
2022-09-21 20:55 ` [PATCH 1/7] test/perf: Always use /sbin/sysctl in tcp test Stefano Brivio
@ 2022-09-22 6:28 ` David Gibson
0 siblings, 0 replies; 19+ messages in thread
From: David Gibson @ 2022-09-22 6:28 UTC (permalink / raw)
To: passt-dev
[-- Attachment #1: Type: text/plain, Size: 2065 bytes --]
On Wed, Sep 21, 2022 at 10:55:01PM +0200, Stefano Brivio wrote:
> Signed-off-by: Stefano Brivio <sbrivio(a)redhat.com>
Reviewed-by: David Gibson <david(a)gibson.dropbear.id.au>
> ---
> test/perf/passt_tcp | 6 +++---
> test/perf/pasta_tcp | 6 +++---
> 2 files changed, 6 insertions(+), 6 deletions(-)
>
> diff --git a/test/perf/passt_tcp b/test/perf/passt_tcp
> index f0cfa1b..bee4f9e 100644
> --- a/test/perf/passt_tcp
> +++ b/test/perf/passt_tcp
> @@ -30,9 +30,9 @@ guest /sbin/sysctl -w net.ipv4.tcp_rmem="4096 131072 268435456"
> guest /sbin/sysctl -w net.ipv4.tcp_wmem="4096 131072 268435456"
> guest /sbin/sysctl -w net.ipv4.tcp_timestamps=0
>
> -ns sysctl -w net.ipv4.tcp_rmem="4096 524288 134217728"
> -ns sysctl -w net.ipv4.tcp_wmem="4096 524288 134217728"
> -ns sysctl -w net.ipv4.tcp_timestamps=0
> +ns /sbin/sysctl -w net.ipv4.tcp_rmem="4096 524288 134217728"
> +ns /sbin/sysctl -w net.ipv4.tcp_wmem="4096 524288 134217728"
> +ns /sbin/sysctl -w net.ipv4.tcp_timestamps=0
>
> gout GW ip -j -4 route show|jq -rM '.[] | select(.dst == "default").gateway'
> gout GW6 ip -j -6 route show|jq -rM '.[] | select(.dst == "default").gateway'
> diff --git a/test/perf/pasta_tcp b/test/perf/pasta_tcp
> index 602ce52..cc21075 100644
> --- a/test/perf/pasta_tcp
> +++ b/test/perf/pasta_tcp
> @@ -16,9 +16,9 @@ nstools /sbin/sysctl nproc ip seq sleep iperf3 tcp_rr tcp_crr jq sed
>
> test pasta: throughput and latency (local connections)
>
> -ns sysctl -w net.ipv4.tcp_rmem="131072 524288 134217728"
> -ns sysctl -w net.ipv4.tcp_wmem="131072 524288 134217728"
> -ns sysctl -w net.ipv4.tcp_timestamps=0
> +ns /sbin/sysctl -w net.ipv4.tcp_rmem="131072 524288 134217728"
> +ns /sbin/sysctl -w net.ipv4.tcp_wmem="131072 524288 134217728"
> +ns /sbin/sysctl -w net.ipv4.tcp_timestamps=0
>
>
> set THREADS 2
--
David Gibson | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH 2/7] test/perf: Switch performance test duration to 10 seconds instead of 30
2022-09-21 20:55 [PATCH 0/7] Fixes and workarounds for tests, Coverity warnings Stefano Brivio
2022-09-21 20:55 ` [PATCH 1/7] test/perf: Always use /sbin/sysctl in tcp test Stefano Brivio
@ 2022-09-21 20:55 ` Stefano Brivio
2022-09-22 6:29 ` David Gibson
2022-09-21 20:55 ` [PATCH 3/7] tap: Check return value of accept4() before calling getsockopt() Stefano Brivio
` (4 subsequent siblings)
6 siblings, 1 reply; 19+ messages in thread
From: Stefano Brivio @ 2022-09-21 20:55 UTC (permalink / raw)
To: passt-dev
[-- Attachment #1: Type: text/plain, Size: 2132 bytes --]
It looks like the workaround for the virtio_net TX hang issue is
working less reliably with the new command dispatch mechanism, I'm
not sure why. Switch to 10 seconds, at least for the moment.
Signed-off-by: Stefano Brivio <sbrivio(a)redhat.com>
---
test/perf/passt_tcp | 2 +-
test/perf/passt_udp | 2 +-
test/perf/pasta_tcp | 2 +-
test/perf/pasta_udp | 2 +-
4 files changed, 4 insertions(+), 4 deletions(-)
diff --git a/test/perf/passt_tcp b/test/perf/passt_tcp
index bee4f9e..5f0aa3a 100644
--- a/test/perf/passt_tcp
+++ b/test/perf/passt_tcp
@@ -44,7 +44,7 @@ hout FREQ [ -n "__FREQ_CPUFREQ__" ] && echo __FREQ_CPUFREQ__ || echo __FREQ_PROC
set THREADS 1
set STREAMS 8
-set TIME 30
+set TIME 10
hout OMIT echo __TIME__ / 6 | bc -l
set OPTS -Z -P __STREAMS__ -l 1M -i1 -O__OMIT__ --pacing-timer 1000000
diff --git a/test/perf/passt_udp b/test/perf/passt_udp
index 80731d1..6bd86ff 100644
--- a/test/perf/passt_udp
+++ b/test/perf/passt_udp
@@ -37,7 +37,7 @@ hout FREQ [ -n "__FREQ_CPUFREQ__" ] && echo __FREQ_CPUFREQ__ || echo __FREQ_PROC
set THREADS 4
set STREAMS 1
-set TIME 30
+set TIME 10
set OPTS -u -i1 -P __STREAMS__ --pacing-timer 1000
info Throughput in Gbps, latency in µs, __THREADS__ threads at __FREQ__ GHz, one stream each
diff --git a/test/perf/pasta_tcp b/test/perf/pasta_tcp
index cc21075..44c5e54 100644
--- a/test/perf/pasta_tcp
+++ b/test/perf/pasta_tcp
@@ -23,7 +23,7 @@ ns /sbin/sysctl -w net.ipv4.tcp_timestamps=0
set THREADS 2
set STREAMS 2
-set TIME 30
+set TIME 10
hout OMIT echo __TIME__ / 6 | bc -l
set OPTS -Z -w 4M -l 1M -P __STREAMS__ -i1 -O__OMIT__ --pacing-timer 10000
diff --git a/test/perf/pasta_udp b/test/perf/pasta_udp
index ae898b1..abb88b0 100644
--- a/test/perf/pasta_udp
+++ b/test/perf/pasta_udp
@@ -22,7 +22,7 @@ hout FREQ [ -n "__FREQ_CPUFREQ__" ] && echo __FREQ_CPUFREQ__ || echo __FREQ_PROC
set THREADS 1
set STREAMS 4
-set TIME 30
+set TIME 10
set OPTS -u -i1 -P __STREAMS__
info Throughput in Gbps, latency in µs, one thread at __FREQ__ GHz, __STREAMS__ streams
--
@@ -22,7 +22,7 @@ hout FREQ [ -n "__FREQ_CPUFREQ__" ] && echo __FREQ_CPUFREQ__ || echo __FREQ_PROC
set THREADS 1
set STREAMS 4
-set TIME 30
+set TIME 10
set OPTS -u -i1 -P __STREAMS__
info Throughput in Gbps, latency in µs, one thread at __FREQ__ GHz, __STREAMS__ streams
--
2.35.1
^ permalink raw reply related [flat|nested] 19+ messages in thread
* Re: [PATCH 2/7] test/perf: Switch performance test duration to 10 seconds instead of 30
2022-09-21 20:55 ` [PATCH 2/7] test/perf: Switch performance test duration to 10 seconds instead of 30 Stefano Brivio
@ 2022-09-22 6:29 ` David Gibson
0 siblings, 0 replies; 19+ messages in thread
From: David Gibson @ 2022-09-22 6:29 UTC (permalink / raw)
To: passt-dev
[-- Attachment #1: Type: text/plain, Size: 2569 bytes --]
On Wed, Sep 21, 2022 at 10:55:02PM +0200, Stefano Brivio wrote:
> It looks like the workaround for the virtio_net TX hang issue is
> working less reliably with the new command dispatch mechanism, I'm
> not sure why. Switch to 10 seconds, at least for the moment.
>
> Signed-off-by: Stefano Brivio <sbrivio(a)redhat.com>
Reviewed-by: David Gibson <david(a)gibson.dropbear.id.au>
> ---
> test/perf/passt_tcp | 2 +-
> test/perf/passt_udp | 2 +-
> test/perf/pasta_tcp | 2 +-
> test/perf/pasta_udp | 2 +-
> 4 files changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/test/perf/passt_tcp b/test/perf/passt_tcp
> index bee4f9e..5f0aa3a 100644
> --- a/test/perf/passt_tcp
> +++ b/test/perf/passt_tcp
> @@ -44,7 +44,7 @@ hout FREQ [ -n "__FREQ_CPUFREQ__" ] && echo __FREQ_CPUFREQ__ || echo __FREQ_PROC
>
> set THREADS 1
> set STREAMS 8
> -set TIME 30
> +set TIME 10
> hout OMIT echo __TIME__ / 6 | bc -l
> set OPTS -Z -P __STREAMS__ -l 1M -i1 -O__OMIT__ --pacing-timer 1000000
>
> diff --git a/test/perf/passt_udp b/test/perf/passt_udp
> index 80731d1..6bd86ff 100644
> --- a/test/perf/passt_udp
> +++ b/test/perf/passt_udp
> @@ -37,7 +37,7 @@ hout FREQ [ -n "__FREQ_CPUFREQ__" ] && echo __FREQ_CPUFREQ__ || echo __FREQ_PROC
>
> set THREADS 4
> set STREAMS 1
> -set TIME 30
> +set TIME 10
> set OPTS -u -i1 -P __STREAMS__ --pacing-timer 1000
>
> info Throughput in Gbps, latency in µs, __THREADS__ threads at __FREQ__ GHz, one stream each
> diff --git a/test/perf/pasta_tcp b/test/perf/pasta_tcp
> index cc21075..44c5e54 100644
> --- a/test/perf/pasta_tcp
> +++ b/test/perf/pasta_tcp
> @@ -23,7 +23,7 @@ ns /sbin/sysctl -w net.ipv4.tcp_timestamps=0
>
> set THREADS 2
> set STREAMS 2
> -set TIME 30
> +set TIME 10
> hout OMIT echo __TIME__ / 6 | bc -l
> set OPTS -Z -w 4M -l 1M -P __STREAMS__ -i1 -O__OMIT__ --pacing-timer 10000
>
> diff --git a/test/perf/pasta_udp b/test/perf/pasta_udp
> index ae898b1..abb88b0 100644
> --- a/test/perf/pasta_udp
> +++ b/test/perf/pasta_udp
> @@ -22,7 +22,7 @@ hout FREQ [ -n "__FREQ_CPUFREQ__" ] && echo __FREQ_CPUFREQ__ || echo __FREQ_PROC
>
> set THREADS 1
> set STREAMS 4
> -set TIME 30
> +set TIME 10
> set OPTS -u -i1 -P __STREAMS__
>
> info Throughput in Gbps, latency in µs, one thread at __FREQ__ GHz, __STREAMS__ streams
--
David Gibson | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH 3/7] tap: Check return value of accept4() before calling getsockopt()
2022-09-21 20:55 [PATCH 0/7] Fixes and workarounds for tests, Coverity warnings Stefano Brivio
2022-09-21 20:55 ` [PATCH 1/7] test/perf: Always use /sbin/sysctl in tcp test Stefano Brivio
2022-09-21 20:55 ` [PATCH 2/7] test/perf: Switch performance test duration to 10 seconds instead of 30 Stefano Brivio
@ 2022-09-21 20:55 ` Stefano Brivio
2022-09-22 6:30 ` David Gibson
2022-09-21 20:55 ` [PATCH 4/7] conf, tcp, udp: Arrays for ports need 2^16 values, not 2^16-8 Stefano Brivio
` (3 subsequent siblings)
6 siblings, 1 reply; 19+ messages in thread
From: Stefano Brivio @ 2022-09-21 20:55 UTC (permalink / raw)
To: passt-dev
[-- Attachment #1: Type: text/plain, Size: 950 bytes --]
Reported by Coverity (CWE-119):
Negative value used as argument to a function expecting a
positive value (for example, size of buffer or allocation)
and harmless, because getsockopt() would return -EBADF if the
socket is -1, so we wouldn't print anything.
Check if accept4() returns a valid socket before calling getsockopt()
on it.
Signed-off-by: Stefano Brivio <sbrivio(a)redhat.com>
---
tap.c | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/tap.c b/tap.c
index 3231da7..4d7422f 100644
--- a/tap.c
+++ b/tap.c
@@ -872,11 +872,13 @@ static void tap_sock_unix_new(struct ctx *c)
int discard = accept4(c->fd_tap_listen, NULL, NULL,
SOCK_NONBLOCK);
+ if (discard == -1)
+ return;
+
if (!getsockopt(discard, SOL_SOCKET, SO_PEERCRED, &ucred, &len))
info("discarding connection from PID %i", ucred.pid);
- if (discard != -1)
- close(discard);
+ close(discard);
return;
}
--
@@ -872,11 +872,13 @@ static void tap_sock_unix_new(struct ctx *c)
int discard = accept4(c->fd_tap_listen, NULL, NULL,
SOCK_NONBLOCK);
+ if (discard == -1)
+ return;
+
if (!getsockopt(discard, SOL_SOCKET, SO_PEERCRED, &ucred, &len))
info("discarding connection from PID %i", ucred.pid);
- if (discard != -1)
- close(discard);
+ close(discard);
return;
}
--
2.35.1
^ permalink raw reply related [flat|nested] 19+ messages in thread
* Re: [PATCH 3/7] tap: Check return value of accept4() before calling getsockopt()
2022-09-21 20:55 ` [PATCH 3/7] tap: Check return value of accept4() before calling getsockopt() Stefano Brivio
@ 2022-09-22 6:30 ` David Gibson
0 siblings, 0 replies; 19+ messages in thread
From: David Gibson @ 2022-09-22 6:30 UTC (permalink / raw)
To: passt-dev
[-- Attachment #1: Type: text/plain, Size: 1324 bytes --]
On Wed, Sep 21, 2022 at 10:55:03PM +0200, Stefano Brivio wrote:
> Reported by Coverity (CWE-119):
>
> Negative value used as argument to a function expecting a
> positive value (for example, size of buffer or allocation)
>
> and harmless, because getsockopt() would return -EBADF if the
> socket is -1, so we wouldn't print anything.
>
> Check if accept4() returns a valid socket before calling getsockopt()
> on it.
>
> Signed-off-by: Stefano Brivio <sbrivio(a)redhat.com>
Reviewed-by: David Gibson <david(a)gibson.dropbear.id.au>
> ---
> tap.c | 6 ++++--
> 1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/tap.c b/tap.c
> index 3231da7..4d7422f 100644
> --- a/tap.c
> +++ b/tap.c
> @@ -872,11 +872,13 @@ static void tap_sock_unix_new(struct ctx *c)
> int discard = accept4(c->fd_tap_listen, NULL, NULL,
> SOCK_NONBLOCK);
>
> + if (discard == -1)
> + return;
> +
> if (!getsockopt(discard, SOL_SOCKET, SO_PEERCRED, &ucred, &len))
> info("discarding connection from PID %i", ucred.pid);
>
> - if (discard != -1)
> - close(discard);
> + close(discard);
>
> return;
> }
--
David Gibson | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH 4/7] conf, tcp, udp: Arrays for ports need 2^16 values, not 2^16-8
2022-09-21 20:55 [PATCH 0/7] Fixes and workarounds for tests, Coverity warnings Stefano Brivio
` (2 preceding siblings ...)
2022-09-21 20:55 ` [PATCH 3/7] tap: Check return value of accept4() before calling getsockopt() Stefano Brivio
@ 2022-09-21 20:55 ` Stefano Brivio
2022-09-22 6:43 ` David Gibson
2022-09-21 20:55 ` [PATCH 5/7] test/lib: Restore IFS while executing directives in def blocks Stefano Brivio
` (2 subsequent siblings)
6 siblings, 1 reply; 19+ messages in thread
From: Stefano Brivio @ 2022-09-21 20:55 UTC (permalink / raw)
To: passt-dev
[-- Attachment #1: Type: text/plain, Size: 1964 bytes --]
Reported by David but also by Coverity (CWE-119):
In conf_ports: Out-of-bounds access to a buffer
...not in practice, because the allocation size is rounded up
anyway, but not nice either.
Reported-by: David Gibson <david(a)gibson.dropbear.id.au>
Signed-off-by: Stefano Brivio <sbrivio(a)redhat.com>
---
conf.c | 2 +-
tcp.h | 4 ++--
udp.h | 4 ++--
3 files changed, 5 insertions(+), 5 deletions(-)
diff --git a/conf.c b/conf.c
index d80233c..7ecfa1e 100644
--- a/conf.c
+++ b/conf.c
@@ -127,8 +127,8 @@ static int conf_ports(struct ctx *c, char optname, const char *optarg,
{
int start_src, end_src, start_dst, end_dst, exclude_only = 1, i, port;
char addr_buf[sizeof(struct in6_addr)] = { 0 }, *addr = addr_buf;
+ uint8_t *map, exclude[DIV_ROUND_UP(USHRT_MAX, 8)] = { 0 };
void (*remap)(in_port_t port, in_port_t delta);
- uint8_t *map, exclude[USHRT_MAX / 8] = { 0 };
char buf[BUFSIZ], *sep, *spec, *p;
sa_family_t af = AF_UNSPEC;
diff --git a/tcp.h b/tcp.h
index 7b720c1..6431b75 100644
--- a/tcp.h
+++ b/tcp.h
@@ -69,9 +69,9 @@ struct tcp_ctx {
uint64_t hash_secret[2];
int conn_count;
int splice_conn_count;
- uint8_t port_to_tap [USHRT_MAX / 8];
+ uint8_t port_to_tap [DIV_ROUND_UP(USHRT_MAX, 8)];
int init_detect_ports;
- uint8_t port_to_init [USHRT_MAX / 8];
+ uint8_t port_to_init [DIV_ROUND_UP(USHRT_MAX, 8)];
int ns_detect_ports;
struct timespec timer_run;
#ifdef HAS_SND_WND
diff --git a/udp.h b/udp.h
index f16fe5e..8f82842 100644
--- a/udp.h
+++ b/udp.h
@@ -53,9 +53,9 @@ union udp_epoll_ref {
* @timer_run: Timestamp of most recent timer run
*/
struct udp_ctx {
- uint8_t port_to_tap [USHRT_MAX / 8];
+ uint8_t port_to_tap [DIV_ROUND_UP(USHRT_MAX, 8)];
int init_detect_ports;
- uint8_t port_to_init [USHRT_MAX / 8];
+ uint8_t port_to_init [DIV_ROUND_UP(USHRT_MAX, 8)];
int ns_detect_ports;
struct timespec timer_run;
};
--
@@ -53,9 +53,9 @@ union udp_epoll_ref {
* @timer_run: Timestamp of most recent timer run
*/
struct udp_ctx {
- uint8_t port_to_tap [USHRT_MAX / 8];
+ uint8_t port_to_tap [DIV_ROUND_UP(USHRT_MAX, 8)];
int init_detect_ports;
- uint8_t port_to_init [USHRT_MAX / 8];
+ uint8_t port_to_init [DIV_ROUND_UP(USHRT_MAX, 8)];
int ns_detect_ports;
struct timespec timer_run;
};
--
2.35.1
^ permalink raw reply related [flat|nested] 19+ messages in thread
* Re: [PATCH 4/7] conf, tcp, udp: Arrays for ports need 2^16 values, not 2^16-8
2022-09-21 20:55 ` [PATCH 4/7] conf, tcp, udp: Arrays for ports need 2^16 values, not 2^16-8 Stefano Brivio
@ 2022-09-22 6:43 ` David Gibson
2022-09-22 8:21 ` Stefano Brivio
0 siblings, 1 reply; 19+ messages in thread
From: David Gibson @ 2022-09-22 6:43 UTC (permalink / raw)
To: passt-dev
[-- Attachment #1: Type: text/plain, Size: 3482 bytes --]
On Wed, Sep 21, 2022 at 10:55:04PM +0200, Stefano Brivio wrote:
> Reported by David but also by Coverity (CWE-119):
>
> In conf_ports: Out-of-bounds access to a buffer
>
> ...not in practice, because the allocation size is rounded up
> anyway, but not nice either.
So... this helps, but it's not enough. For starters, this is still
conceptually wrong - there should be 65536 bits in the bitmap, not
65535 (USHRT_MAX). This patch just masks it by rounding up to 65536
rather than down to 65528.
Alas, this is not the only buffer overrun caused by an off-by-one in
port numbers: the delta_to_tap etc. global arrays in tcp.c and udp.c
should also have length 65536, rather than USHRT_MAX. So should
tcp_sock_init_lo and friends. There are also similar problems in
icmp.c with echo request id rather than port number.
Not a buffer overrun, but the USHRT_MAX in udp_remap_to_tap() and
udp_remap_to_init() is also out by one (consider the case of
delta==0).
I have a series which makes a number of cleanups to the port mapping
handling. It fixes this bug and adds typing stuff that should make it
harder to make similar mistakes in future.
I held off on sending, since it's currently based on a lot of my
outstanding patches. I think it wouldn't be too hard to rebase
directly on master, should I do that so you can fix this before going
on to debug the rest of my outstanding stuff?
>
> Reported-by: David Gibson <david(a)gibson.dropbear.id.au>
> Signed-off-by: Stefano Brivio <sbrivio(a)redhat.com>
> ---
> conf.c | 2 +-
> tcp.h | 4 ++--
> udp.h | 4 ++--
> 3 files changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/conf.c b/conf.c
> index d80233c..7ecfa1e 100644
> --- a/conf.c
> +++ b/conf.c
> @@ -127,8 +127,8 @@ static int conf_ports(struct ctx *c, char optname, const char *optarg,
> {
> int start_src, end_src, start_dst, end_dst, exclude_only = 1, i, port;
> char addr_buf[sizeof(struct in6_addr)] = { 0 }, *addr = addr_buf;
> + uint8_t *map, exclude[DIV_ROUND_UP(USHRT_MAX, 8)] = { 0 };
> void (*remap)(in_port_t port, in_port_t delta);
> - uint8_t *map, exclude[USHRT_MAX / 8] = { 0 };
> char buf[BUFSIZ], *sep, *spec, *p;
> sa_family_t af = AF_UNSPEC;
>
> diff --git a/tcp.h b/tcp.h
> index 7b720c1..6431b75 100644
> --- a/tcp.h
> +++ b/tcp.h
> @@ -69,9 +69,9 @@ struct tcp_ctx {
> uint64_t hash_secret[2];
> int conn_count;
> int splice_conn_count;
> - uint8_t port_to_tap [USHRT_MAX / 8];
> + uint8_t port_to_tap [DIV_ROUND_UP(USHRT_MAX, 8)];
> int init_detect_ports;
> - uint8_t port_to_init [USHRT_MAX / 8];
> + uint8_t port_to_init [DIV_ROUND_UP(USHRT_MAX, 8)];
> int ns_detect_ports;
> struct timespec timer_run;
> #ifdef HAS_SND_WND
> diff --git a/udp.h b/udp.h
> index f16fe5e..8f82842 100644
> --- a/udp.h
> +++ b/udp.h
> @@ -53,9 +53,9 @@ union udp_epoll_ref {
> * @timer_run: Timestamp of most recent timer run
> */
> struct udp_ctx {
> - uint8_t port_to_tap [USHRT_MAX / 8];
> + uint8_t port_to_tap [DIV_ROUND_UP(USHRT_MAX, 8)];
> int init_detect_ports;
> - uint8_t port_to_init [USHRT_MAX / 8];
> + uint8_t port_to_init [DIV_ROUND_UP(USHRT_MAX, 8)];
> int ns_detect_ports;
> struct timespec timer_run;
> };
--
David Gibson | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 4/7] conf, tcp, udp: Arrays for ports need 2^16 values, not 2^16-8
2022-09-22 6:43 ` David Gibson
@ 2022-09-22 8:21 ` Stefano Brivio
2022-09-22 23:39 ` Stefano Brivio
0 siblings, 1 reply; 19+ messages in thread
From: Stefano Brivio @ 2022-09-22 8:21 UTC (permalink / raw)
To: passt-dev
[-- Attachment #1: Type: text/plain, Size: 1973 bytes --]
On Thu, 22 Sep 2022 16:43:24 +1000
David Gibson <david(a)gibson.dropbear.id.au> wrote:
> On Wed, Sep 21, 2022 at 10:55:04PM +0200, Stefano Brivio wrote:
> > Reported by David but also by Coverity (CWE-119):
> >
> > In conf_ports: Out-of-bounds access to a buffer
> >
> > ...not in practice, because the allocation size is rounded up
> > anyway, but not nice either.
>
> So... this helps, but it's not enough. For starters, this is still
> conceptually wrong - there should be 65536 bits in the bitmap, not
> 65535 (USHRT_MAX). This patch just masks it by rounding up to 65536
> rather than down to 65528.
Ah, yes, I indeed wanted to have 65536 values in the bitmap (see commit
title), and this does it, but there's no reason to make it implicit.
> Alas, this is not the only buffer overrun caused by an off-by-one in
> port numbers: the delta_to_tap etc. global arrays in tcp.c and udp.c
> should also have length 65536, rather than USHRT_MAX. So should
> tcp_sock_init_lo and friends. There are also similar problems in
> icmp.c with echo request id rather than port number.
>
> Not a buffer overrun, but the USHRT_MAX in udp_remap_to_tap() and
> udp_remap_to_init() is also out by one (consider the case of
> delta==0).
>
> I have a series which makes a number of cleanups to the port mapping
> handling. It fixes this bug and adds typing stuff that should make it
> harder to make similar mistakes in future.
>
> I held off on sending, since it's currently based on a lot of my
> outstanding patches. I think it wouldn't be too hard to rebase
> directly on master, should I do that so you can fix this before going
> on to debug the rest of my outstanding stuff?
If it's not too much effort for you, yes, that would be appreciated.
I hope to be able to push out everything else later today, it's just
one glitch still occurring in test_iperf3() -- sometimes, clients and
servers terminate properly, but we don't see it for some reason.
--
Stefano
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 4/7] conf, tcp, udp: Arrays for ports need 2^16 values, not 2^16-8
2022-09-22 8:21 ` Stefano Brivio
@ 2022-09-22 23:39 ` Stefano Brivio
2022-09-23 2:09 ` David Gibson
0 siblings, 1 reply; 19+ messages in thread
From: Stefano Brivio @ 2022-09-22 23:39 UTC (permalink / raw)
To: passt-dev
[-- Attachment #1: Type: text/plain, Size: 1132 bytes --]
On Thu, 22 Sep 2022 10:21:50 +0200
Stefano Brivio <sbrivio(a)redhat.com> wrote:
> On Thu, 22 Sep 2022 16:43:24 +1000
> David Gibson <david(a)gibson.dropbear.id.au> wrote:
>
> [...]
>
> > I have a series which makes a number of cleanups to the port mapping
> > handling. It fixes this bug and adds typing stuff that should make it
> > harder to make similar mistakes in future.
> >
> > I held off on sending, since it's currently based on a lot of my
> > outstanding patches. I think it wouldn't be too hard to rebase
> > directly on master, should I do that so you can fix this before going
> > on to debug the rest of my outstanding stuff?
>
> If it's not too much effort for you, yes, that would be appreciated.
>
> I hope to be able to push out everything else later today, it's just
> one glitch still occurring in test_iperf3() -- sometimes, clients and
> servers terminate properly, but we don't see it for some reason.
Actually, it looks like I got tests finally passing, and I should be
pushing all the pending patches soon... perhaps you could wait before
you rebase, I guess it's more convenient.
--
Stefano
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 4/7] conf, tcp, udp: Arrays for ports need 2^16 values, not 2^16-8
2022-09-22 23:39 ` Stefano Brivio
@ 2022-09-23 2:09 ` David Gibson
0 siblings, 0 replies; 19+ messages in thread
From: David Gibson @ 2022-09-23 2:09 UTC (permalink / raw)
To: passt-dev
[-- Attachment #1: Type: text/plain, Size: 1442 bytes --]
On Fri, Sep 23, 2022 at 01:39:47AM +0200, Stefano Brivio wrote:
> On Thu, 22 Sep 2022 10:21:50 +0200
> Stefano Brivio <sbrivio(a)redhat.com> wrote:
>
> > On Thu, 22 Sep 2022 16:43:24 +1000
> > David Gibson <david(a)gibson.dropbear.id.au> wrote:
> >
> > [...]
> >
> > > I have a series which makes a number of cleanups to the port mapping
> > > handling. It fixes this bug and adds typing stuff that should make it
> > > harder to make similar mistakes in future.
> > >
> > > I held off on sending, since it's currently based on a lot of my
> > > outstanding patches. I think it wouldn't be too hard to rebase
> > > directly on master, should I do that so you can fix this before going
> > > on to debug the rest of my outstanding stuff?
> >
> > If it's not too much effort for you, yes, that would be appreciated.
> >
> > I hope to be able to push out everything else later today, it's just
> > one glitch still occurring in test_iperf3() -- sometimes, clients and
> > servers terminate properly, but we don't see it for some reason.
>
> Actually, it looks like I got tests finally passing, and I should be
> pushing all the pending patches soon... perhaps you could wait before
> you rebase, I guess it's more convenient.
Will do, thanks.
--
David Gibson | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH 5/7] test/lib: Restore IFS while executing directives in def blocks
2022-09-21 20:55 [PATCH 0/7] Fixes and workarounds for tests, Coverity warnings Stefano Brivio
` (3 preceding siblings ...)
2022-09-21 20:55 ` [PATCH 4/7] conf, tcp, udp: Arrays for ports need 2^16 values, not 2^16-8 Stefano Brivio
@ 2022-09-21 20:55 ` Stefano Brivio
2022-09-22 6:44 ` David Gibson
2022-09-21 20:55 ` [PATCH 6/7] test/lib: Wait on iperf3 clients to be done, then send SIGINT to servers Stefano Brivio
2022-09-21 20:55 ` [PATCH 7/7] test/perf: Disable periodic throughput reports to avoid vhost hang Stefano Brivio
6 siblings, 1 reply; 19+ messages in thread
From: Stefano Brivio @ 2022-09-21 20:55 UTC (permalink / raw)
To: passt-dev
[-- Attachment #1: Type: text/plain, Size: 593 bytes --]
If we don't, guest command dispatch will fail altogether, given that
we use cat(1) on the enter file, which contains spaces.
Signed-off-by: Stefano Brivio <sbrivio(a)redhat.com>
---
test/lib/test | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/test/lib/test b/test/lib/test
index 3ad178f..a9ffe83 100755
--- a/test/lib/test
+++ b/test/lib/test
@@ -312,7 +312,7 @@ test_one_line() {
IFS='
'
for __def_line in ${__def_body}; do
- IFS= test_one_line "${__def_line}"
+ IFS="${__ifs}" test_one_line "${__def_line}"
done
IFS="${__ifs}"
fi
--
@@ -312,7 +312,7 @@ test_one_line() {
IFS='
'
for __def_line in ${__def_body}; do
- IFS= test_one_line "${__def_line}"
+ IFS="${__ifs}" test_one_line "${__def_line}"
done
IFS="${__ifs}"
fi
--
2.35.1
^ permalink raw reply related [flat|nested] 19+ messages in thread
* Re: [PATCH 5/7] test/lib: Restore IFS while executing directives in def blocks
2022-09-21 20:55 ` [PATCH 5/7] test/lib: Restore IFS while executing directives in def blocks Stefano Brivio
@ 2022-09-22 6:44 ` David Gibson
2022-09-22 8:25 ` Stefano Brivio
0 siblings, 1 reply; 19+ messages in thread
From: David Gibson @ 2022-09-22 6:44 UTC (permalink / raw)
To: passt-dev
[-- Attachment #1: Type: text/plain, Size: 923 bytes --]
On Wed, Sep 21, 2022 at 10:55:05PM +0200, Stefano Brivio wrote:
> If we don't, guest command dispatch will fail altogether, given that
> we use cat(1) on the enter file, which contains spaces.
>
> Signed-off-by: Stefano Brivio <sbrivio(a)redhat.com>
Huh. I wonder how this was working for me.
> ---
> test/lib/test | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/test/lib/test b/test/lib/test
> index 3ad178f..a9ffe83 100755
> --- a/test/lib/test
> +++ b/test/lib/test
> @@ -312,7 +312,7 @@ test_one_line() {
> IFS='
> '
> for __def_line in ${__def_body}; do
> - IFS= test_one_line "${__def_line}"
> + IFS="${__ifs}" test_one_line "${__def_line}"
> done
> IFS="${__ifs}"
> fi
--
David Gibson | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 5/7] test/lib: Restore IFS while executing directives in def blocks
2022-09-22 6:44 ` David Gibson
@ 2022-09-22 8:25 ` Stefano Brivio
2022-09-23 6:55 ` David Gibson
0 siblings, 1 reply; 19+ messages in thread
From: Stefano Brivio @ 2022-09-22 8:25 UTC (permalink / raw)
To: passt-dev
[-- Attachment #1: Type: text/plain, Size: 555 bytes --]
On Thu, 22 Sep 2022 16:44:31 +1000
David Gibson <david(a)gibson.dropbear.id.au> wrote:
> On Wed, Sep 21, 2022 at 10:55:05PM +0200, Stefano Brivio wrote:
> > If we don't, guest command dispatch will fail altogether, given that
> > we use cat(1) on the enter file, which contains spaces.
> >
> > Signed-off-by: Stefano Brivio <sbrivio(a)redhat.com>
>
> Huh. I wonder how this was working for me.
I thought you disabled that workaround, which should be the only 'def'
used in non-distro tests (which are still using screen grabbing)...?
--
Stefano
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 5/7] test/lib: Restore IFS while executing directives in def blocks
2022-09-22 8:25 ` Stefano Brivio
@ 2022-09-23 6:55 ` David Gibson
0 siblings, 0 replies; 19+ messages in thread
From: David Gibson @ 2022-09-23 6:55 UTC (permalink / raw)
To: passt-dev
[-- Attachment #1: Type: text/plain, Size: 889 bytes --]
On Thu, Sep 22, 2022 at 10:25:14AM +0200, Stefano Brivio wrote:
> On Thu, 22 Sep 2022 16:44:31 +1000
> David Gibson <david(a)gibson.dropbear.id.au> wrote:
>
> > On Wed, Sep 21, 2022 at 10:55:05PM +0200, Stefano Brivio wrote:
> > > If we don't, guest command dispatch will fail altogether, given that
> > > we use cat(1) on the enter file, which contains spaces.
> > >
> > > Signed-off-by: Stefano Brivio <sbrivio(a)redhat.com>
> >
> > Huh. I wonder how this was working for me.
>
> I thought you disabled that workaround, which should be the only 'def'
> used in non-distro tests (which are still using screen grabbing)...?
Oh, right. I missed that this was specific to the 'def' handling.
--
David Gibson | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH 6/7] test/lib: Wait on iperf3 clients to be done, then send SIGINT to servers
2022-09-21 20:55 [PATCH 0/7] Fixes and workarounds for tests, Coverity warnings Stefano Brivio
` (4 preceding siblings ...)
2022-09-21 20:55 ` [PATCH 5/7] test/lib: Restore IFS while executing directives in def blocks Stefano Brivio
@ 2022-09-21 20:55 ` Stefano Brivio
2022-09-21 20:55 ` [PATCH 7/7] test/perf: Disable periodic throughput reports to avoid vhost hang Stefano Brivio
6 siblings, 0 replies; 19+ messages in thread
From: Stefano Brivio @ 2022-09-21 20:55 UTC (permalink / raw)
To: passt-dev
[-- Attachment #1: Type: text/plain, Size: 1702 bytes --]
An iperf3 client might fail to send the control message indicating
the end of the test, if the kernel buffer doesn't accept it, and exit
without having sent it, as the control socket is non-blocking. Should
this happen, the server will just wait forever for this message,
instead of terminating.
Restore some of the behaviour that went away with the
"test: Rewrite test_iperf3" patch: instead of waiting on servers to
terminate, wait on the clients. When they are done, wait 2 seconds,
and then send SIGINT to the servers, which make them still write
out the JSON report before terminating.
Signed-off-by: Stefano Brivio <sbrivio(a)redhat.com>
---
test/lib/test | 16 ++++++++--------
1 file changed, 8 insertions(+), 8 deletions(-)
diff --git a/test/lib/test b/test/lib/test
index a9ffe83..b69d519 100755
--- a/test/lib/test
+++ b/test/lib/test
@@ -32,13 +32,11 @@ test_iperf3() {
__time="${1}"; shift
pane_or_context_run_bg "${__sctx}" \
- '(' \
- ' for i in $(seq 0 '${__procs}'); do' \
- ' iperf3 -s1J -p'${__port}' -i'${__time} \
- ' > s${i}.json &' \
- ' done;' \
- ' wait' \
- ')'
+ 'for i in $(seq 0 '${__procs}'); do' \
+ ' (iperf3 -s1J -p'${__port}' -i'${__time} \
+ ' > s${i}.json) &' \
+ ' echo $! > s${i}.pid &' \
+ 'done' \
sleep 1 # Wait for server to be ready
@@ -51,7 +49,9 @@ test_iperf3() {
' wait' \
')'
- pane_or_context_wait "${__sctx}"
+ # If client fails to deliver control message, tell server we're done
+ pane_or_context_run "${__sctx}" \
+ 'sleep 2; kill -INT $(cat s*.pid); rm s*.pid'
__jval=".end.sum_received.bits_per_second"
for __opt in ${@}; do
--
@@ -32,13 +32,11 @@ test_iperf3() {
__time="${1}"; shift
pane_or_context_run_bg "${__sctx}" \
- '(' \
- ' for i in $(seq 0 '${__procs}'); do' \
- ' iperf3 -s1J -p'${__port}' -i'${__time} \
- ' > s${i}.json &' \
- ' done;' \
- ' wait' \
- ')'
+ 'for i in $(seq 0 '${__procs}'); do' \
+ ' (iperf3 -s1J -p'${__port}' -i'${__time} \
+ ' > s${i}.json) &' \
+ ' echo $! > s${i}.pid &' \
+ 'done' \
sleep 1 # Wait for server to be ready
@@ -51,7 +49,9 @@ test_iperf3() {
' wait' \
')'
- pane_or_context_wait "${__sctx}"
+ # If client fails to deliver control message, tell server we're done
+ pane_or_context_run "${__sctx}" \
+ 'sleep 2; kill -INT $(cat s*.pid); rm s*.pid'
__jval=".end.sum_received.bits_per_second"
for __opt in ${@}; do
--
2.35.1
^ permalink raw reply related [flat|nested] 19+ messages in thread
* [PATCH 7/7] test/perf: Disable periodic throughput reports to avoid vhost hang
2022-09-21 20:55 [PATCH 0/7] Fixes and workarounds for tests, Coverity warnings Stefano Brivio
` (5 preceding siblings ...)
2022-09-21 20:55 ` [PATCH 6/7] test/lib: Wait on iperf3 clients to be done, then send SIGINT to servers Stefano Brivio
@ 2022-09-21 20:55 ` Stefano Brivio
2022-09-22 6:46 ` David Gibson
6 siblings, 1 reply; 19+ messages in thread
From: Stefano Brivio @ 2022-09-21 20:55 UTC (permalink / raw)
To: passt-dev
[-- Attachment #1: Type: text/plain, Size: 3427 bytes --]
It appears that if we run throughput tests with one-second periodic
reports, the sending side of the vhost channel used for SSH-based
command dispatch occasionally stops working altogether. I haven't
investigated this further, all I see is that output is truncated
at some point, and doesn't resume.
If we use gzip compression (ssh -C) this happens less frequently,
but it still happens, seemingly indicating the issue is probably
related to vhost itself.
Disable periodic reports in iperf3 clients. The -i options were
actually redundant, so remove them from both test files as well as
from test_iperf3().
Signed-off-by: Stefano Brivio <sbrivio(a)redhat.com>
---
test/lib/test | 2 +-
test/perf/passt_tcp | 2 +-
test/perf/passt_udp | 2 +-
test/perf/pasta_tcp | 2 +-
test/perf/pasta_udp | 2 +-
5 files changed, 5 insertions(+), 5 deletions(-)
diff --git a/test/lib/test b/test/lib/test
index b69d519..d68ade4 100755
--- a/test/lib/test
+++ b/test/lib/test
@@ -44,7 +44,7 @@ test_iperf3() {
'(' \
' for i in $(seq 0 '${__procs}'); do' \
' iperf3 -c '${__dest}' -p '${__port} \
- ' -t'${__time}' -T s${i} '"${@}"' &' \
+ ' -t'${__time}' -i0 -T s${i} '"${@}"' &' \
' done;' \
' wait' \
')'
diff --git a/test/perf/passt_tcp b/test/perf/passt_tcp
index 5f0aa3a..5ba5450 100644
--- a/test/perf/passt_tcp
+++ b/test/perf/passt_tcp
@@ -46,7 +46,7 @@ set THREADS 1
set STREAMS 8
set TIME 10
hout OMIT echo __TIME__ / 6 | bc -l
-set OPTS -Z -P __STREAMS__ -l 1M -i1 -O__OMIT__ --pacing-timer 1000000
+set OPTS -Z -P __STREAMS__ -l 1M -O__OMIT__ --pacing-timer 1000000
info Throughput in Gbps, latency in µs, one thread at __FREQ__ GHz, __STREAMS__ streams
report passt tcp __THREADS__ __FREQ__
diff --git a/test/perf/passt_udp b/test/perf/passt_udp
index 6bd86ff..fd2ddc1 100644
--- a/test/perf/passt_udp
+++ b/test/perf/passt_udp
@@ -38,7 +38,7 @@ hout FREQ [ -n "__FREQ_CPUFREQ__" ] && echo __FREQ_CPUFREQ__ || echo __FREQ_PROC
set THREADS 4
set STREAMS 1
set TIME 10
-set OPTS -u -i1 -P __STREAMS__ --pacing-timer 1000
+set OPTS -u -P __STREAMS__ --pacing-timer 1000
info Throughput in Gbps, latency in µs, __THREADS__ threads at __FREQ__ GHz, one stream each
diff --git a/test/perf/pasta_tcp b/test/perf/pasta_tcp
index 44c5e54..1847c83 100644
--- a/test/perf/pasta_tcp
+++ b/test/perf/pasta_tcp
@@ -25,7 +25,7 @@ set THREADS 2
set STREAMS 2
set TIME 10
hout OMIT echo __TIME__ / 6 | bc -l
-set OPTS -Z -w 4M -l 1M -P __STREAMS__ -i1 -O__OMIT__ --pacing-timer 10000
+set OPTS -Z -w 4M -l 1M -P __STREAMS__ -O__OMIT__ --pacing-timer 10000
hout FREQ_PROCFS (echo "scale=1"; sed -n 's/cpu MHz.*: \([0-9]*\)\..*$/(\1+10^2\/2)\/10^3/p' /proc/cpuinfo) | bc -l | head -n1
hout FREQ_CPUFREQ (echo "scale=1"; printf '( %i + 10^5 / 2 ) / 10^6\n' $(cat /sys/devices/system/cpu/cpu0/cpufreq/scaling_max_freq) ) | bc -l
diff --git a/test/perf/pasta_udp b/test/perf/pasta_udp
index abb88b0..27ea724 100644
--- a/test/perf/pasta_udp
+++ b/test/perf/pasta_udp
@@ -23,7 +23,7 @@ hout FREQ [ -n "__FREQ_CPUFREQ__" ] && echo __FREQ_CPUFREQ__ || echo __FREQ_PROC
set THREADS 1
set STREAMS 4
set TIME 10
-set OPTS -u -i1 -P __STREAMS__
+set OPTS -u -P __STREAMS__
info Throughput in Gbps, latency in µs, one thread at __FREQ__ GHz, __STREAMS__ streams
--
@@ -23,7 +23,7 @@ hout FREQ [ -n "__FREQ_CPUFREQ__" ] && echo __FREQ_CPUFREQ__ || echo __FREQ_PROC
set THREADS 1
set STREAMS 4
set TIME 10
-set OPTS -u -i1 -P __STREAMS__
+set OPTS -u -P __STREAMS__
info Throughput in Gbps, latency in µs, one thread at __FREQ__ GHz, __STREAMS__ streams
--
2.35.1
^ permalink raw reply related [flat|nested] 19+ messages in thread
* Re: [PATCH 7/7] test/perf: Disable periodic throughput reports to avoid vhost hang
2022-09-21 20:55 ` [PATCH 7/7] test/perf: Disable periodic throughput reports to avoid vhost hang Stefano Brivio
@ 2022-09-22 6:46 ` David Gibson
0 siblings, 0 replies; 19+ messages in thread
From: David Gibson @ 2022-09-22 6:46 UTC (permalink / raw)
To: passt-dev
[-- Attachment #1: Type: text/plain, Size: 3892 bytes --]
On Wed, Sep 21, 2022 at 10:55:07PM +0200, Stefano Brivio wrote:
> It appears that if we run throughput tests with one-second periodic
> reports, the sending side of the vhost channel used for SSH-based
> command dispatch occasionally stops working altogether. I haven't
> investigated this further, all I see is that output is truncated
> at some point, and doesn't resume.
Huh. I don't think I ever observed this.
> If we use gzip compression (ssh -C) this happens less frequently,
> but it still happens, seemingly indicating the issue is probably
> related to vhost itself.
>
> Disable periodic reports in iperf3 clients. The -i options were
> actually redundant, so remove them from both test files as well as
> from test_iperf3().
>
> Signed-off-by: Stefano Brivio <sbrivio(a)redhat.com>
> ---
> test/lib/test | 2 +-
> test/perf/passt_tcp | 2 +-
> test/perf/passt_udp | 2 +-
> test/perf/pasta_tcp | 2 +-
> test/perf/pasta_udp | 2 +-
> 5 files changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/test/lib/test b/test/lib/test
> index b69d519..d68ade4 100755
> --- a/test/lib/test
> +++ b/test/lib/test
> @@ -44,7 +44,7 @@ test_iperf3() {
> '(' \
> ' for i in $(seq 0 '${__procs}'); do' \
> ' iperf3 -c '${__dest}' -p '${__port} \
> - ' -t'${__time}' -T s${i} '"${@}"' &' \
> + ' -t'${__time}' -i0 -T s${i} '"${@}"' &' \
> ' done;' \
> ' wait' \
> ')'
> diff --git a/test/perf/passt_tcp b/test/perf/passt_tcp
> index 5f0aa3a..5ba5450 100644
> --- a/test/perf/passt_tcp
> +++ b/test/perf/passt_tcp
> @@ -46,7 +46,7 @@ set THREADS 1
> set STREAMS 8
> set TIME 10
> hout OMIT echo __TIME__ / 6 | bc -l
> -set OPTS -Z -P __STREAMS__ -l 1M -i1 -O__OMIT__ --pacing-timer 1000000
> +set OPTS -Z -P __STREAMS__ -l 1M -O__OMIT__ --pacing-timer 1000000
>
> info Throughput in Gbps, latency in µs, one thread at __FREQ__ GHz, __STREAMS__ streams
> report passt tcp __THREADS__ __FREQ__
> diff --git a/test/perf/passt_udp b/test/perf/passt_udp
> index 6bd86ff..fd2ddc1 100644
> --- a/test/perf/passt_udp
> +++ b/test/perf/passt_udp
> @@ -38,7 +38,7 @@ hout FREQ [ -n "__FREQ_CPUFREQ__" ] && echo __FREQ_CPUFREQ__ || echo __FREQ_PROC
> set THREADS 4
> set STREAMS 1
> set TIME 10
> -set OPTS -u -i1 -P __STREAMS__ --pacing-timer 1000
> +set OPTS -u -P __STREAMS__ --pacing-timer 1000
>
> info Throughput in Gbps, latency in µs, __THREADS__ threads at __FREQ__ GHz, one stream each
>
> diff --git a/test/perf/pasta_tcp b/test/perf/pasta_tcp
> index 44c5e54..1847c83 100644
> --- a/test/perf/pasta_tcp
> +++ b/test/perf/pasta_tcp
> @@ -25,7 +25,7 @@ set THREADS 2
> set STREAMS 2
> set TIME 10
> hout OMIT echo __TIME__ / 6 | bc -l
> -set OPTS -Z -w 4M -l 1M -P __STREAMS__ -i1 -O__OMIT__ --pacing-timer 10000
> +set OPTS -Z -w 4M -l 1M -P __STREAMS__ -O__OMIT__ --pacing-timer 10000
>
> hout FREQ_PROCFS (echo "scale=1"; sed -n 's/cpu MHz.*: \([0-9]*\)\..*$/(\1+10^2\/2)\/10^3/p' /proc/cpuinfo) | bc -l | head -n1
> hout FREQ_CPUFREQ (echo "scale=1"; printf '( %i + 10^5 / 2 ) / 10^6\n' $(cat /sys/devices/system/cpu/cpu0/cpufreq/scaling_max_freq) ) | bc -l
> diff --git a/test/perf/pasta_udp b/test/perf/pasta_udp
> index abb88b0..27ea724 100644
> --- a/test/perf/pasta_udp
> +++ b/test/perf/pasta_udp
> @@ -23,7 +23,7 @@ hout FREQ [ -n "__FREQ_CPUFREQ__" ] && echo __FREQ_CPUFREQ__ || echo __FREQ_PROC
> set THREADS 1
> set STREAMS 4
> set TIME 10
> -set OPTS -u -i1 -P __STREAMS__
> +set OPTS -u -P __STREAMS__
>
> info Throughput in Gbps, latency in µs, one thread at __FREQ__ GHz, __STREAMS__ streams
>
--
David Gibson | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 19+ messages in thread