* [PATCH 0/2] Don't drop outbound zero-length UDP packets over tap
@ 2022-09-09 4:27 David Gibson
2022-09-09 4:27 ` [PATCH 1/2] udp: Don't drop zero-length outbound UDP packets David Gibson
2022-09-09 4:27 ` [PATCH 2/2] test: Simpler termination handling for UDP tests David Gibson
0 siblings, 2 replies; 9+ messages in thread
From: David Gibson @ 2022-09-09 4:27 UTC (permalink / raw)
To: passt-dev
[-- Attachment #1: Type: text/plain, Size: 759 bytes --]
passt/pasta was incorrectly dropping UDP packets with a zero-length
payload when travelling out via the tap interface. This is incorrect,
since for a datagram protocol, zero-length packets are still
meaningful.
Based on my earlier series for test command dispatch, user namespace
cleanup and test temporary file handling.
Bugzilla: https://bugs.passt.top/show_bug.cgi?id=19
David Gibson (2):
udp: Don't drop zero-length outbound UDP packets
test: Simpler termination handling for UDP tests
test/passt/udp | 23 +++++++-------
test/passt_in_ns/udp | 73 ++++++++++++++++++++++----------------------
test/pasta/udp | 31 +++++++++----------
udp.c | 10 +++---
4 files changed, 67 insertions(+), 70 deletions(-)
--
2.37.3
^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH 1/2] udp: Don't drop zero-length outbound UDP packets
2022-09-09 4:27 [PATCH 0/2] Don't drop outbound zero-length UDP packets over tap David Gibson
@ 2022-09-09 4:27 ` David Gibson
2022-09-09 9:26 ` Stefano Brivio
2022-09-09 4:27 ` [PATCH 2/2] test: Simpler termination handling for UDP tests David Gibson
1 sibling, 1 reply; 9+ messages in thread
From: David Gibson @ 2022-09-09 4:27 UTC (permalink / raw)
To: passt-dev
[-- Attachment #1: Type: text/plain, Size: 1168 bytes --]
udp_tap_handler() currently skips outbound packets if they have a payload
length of zero. This is not correct, since in a datagram protocol zero
length packets still have meaning.
Adjust this to correctly forward the zero-length packets by using a msghdr
with msg_iovlen == 0.
Bugzilla: https://bugs.passt.top/show_bug.cgi?id=19
Signed-off-by: David Gibson <david(a)gibson.dropbear.id.au>
---
udp.c | 10 +++++-----
1 file changed, 5 insertions(+), 5 deletions(-)
diff --git a/udp.c b/udp.c
index c4ebecc..caa852a 100644
--- a/udp.c
+++ b/udp.c
@@ -1075,19 +1075,19 @@ int udp_tap_handler(struct ctx *c, int af, const void *addr,
uh_send = packet_get(p, i, 0, sizeof(*uh), &len);
if (!uh_send)
return p->count;
+
+ mm[i].msg_hdr.msg_name = sa;
+ mm[i].msg_hdr.msg_namelen = sl;
+ count++;
+
if (!len)
continue;
m[i].iov_base = (char *)(uh_send + 1);
m[i].iov_len = len;
- mm[i].msg_hdr.msg_name = sa;
- mm[i].msg_hdr.msg_namelen = sl;
-
mm[i].msg_hdr.msg_iov = m + i;
mm[i].msg_hdr.msg_iovlen = 1;
-
- count++;
}
count = sendmmsg(s, mm, count, MSG_NOSIGNAL);
--
@@ -1075,19 +1075,19 @@ int udp_tap_handler(struct ctx *c, int af, const void *addr,
uh_send = packet_get(p, i, 0, sizeof(*uh), &len);
if (!uh_send)
return p->count;
+
+ mm[i].msg_hdr.msg_name = sa;
+ mm[i].msg_hdr.msg_namelen = sl;
+ count++;
+
if (!len)
continue;
m[i].iov_base = (char *)(uh_send + 1);
m[i].iov_len = len;
- mm[i].msg_hdr.msg_name = sa;
- mm[i].msg_hdr.msg_namelen = sl;
-
mm[i].msg_hdr.msg_iov = m + i;
mm[i].msg_hdr.msg_iovlen = 1;
-
- count++;
}
count = sendmmsg(s, mm, count, MSG_NOSIGNAL);
--
2.37.3
^ permalink raw reply related [flat|nested] 9+ messages in thread
* [PATCH 2/2] test: Simpler termination handling for UDP tests
2022-09-09 4:27 [PATCH 0/2] Don't drop outbound zero-length UDP packets over tap David Gibson
2022-09-09 4:27 ` [PATCH 1/2] udp: Don't drop zero-length outbound UDP packets David Gibson
@ 2022-09-09 4:27 ` David Gibson
1 sibling, 0 replies; 9+ messages in thread
From: David Gibson @ 2022-09-09 4:27 UTC (permalink / raw)
To: passt-dev
[-- Attachment #1: Type: text/plain, Size: 16113 bytes --]
Because UDP is connectionless we don't have an in-built end-of-stream
signal for our connectivity tests. We work around this by explicitly
adding an end marker to our sample data and killing the listening end once
it is seen.
However, socat has some built-in options - null-eof and shut-null - which
can be used to signal the end of stream with a zero-length UDP packet.
Use these to simplify how the UDP tests are implemented.
Signed-off-by: David Gibson <david(a)gibson.dropbear.id.au>
---
test/passt/udp | 23 +++++++-------
test/passt_in_ns/udp | 73 ++++++++++++++++++++++----------------------
test/pasta/udp | 31 +++++++++----------
3 files changed, 62 insertions(+), 65 deletions(-)
diff --git a/test/passt/udp b/test/passt/udp
index 0253a86..7d444b8 100644
--- a/test/passt/udp
+++ b/test/passt/udp
@@ -11,42 +11,41 @@
# Copyright (c) 2021 Red Hat GmbH
# Author: Stefano Brivio <sbrivio(a)redhat.com>
-gtools socat tee grep cat ip jq md5sum cut
-htools printf dd socat tee grep cat ip jq md5sum cut
+gtools socat ip jq md5sum cut
+htools dd socat jq md5sum cut
test UDP/IPv4: host to guest
set TEMP __STATEDIR__/data
-set SC_PID __STATEDIR__/socat.pid
-guestb (socat -u UDP4-LISTEN:10001 STDOUT & echo $! > sc.pid) | tee test.bin | (grep -qm1 "END_OF_TEST" && kill $(cat sc.pid))
+guestb socat -u UDP4-LISTEN:10001,null-eof OPEN:test.bin,create,trunc
sleep 1
-host dd if=/dev/urandom bs=1k count=5 > __TEMP__ && printf "\nEND_OF_TEST\n" >> __TEMP__
-host socat -u OPEN:__TEMP__ UDP4:127.0.0.1:10001
+host dd if=/dev/urandom bs=1k count=5 > __TEMP__
+host socat -u OPEN:__TEMP__ UDP4:127.0.0.1:10001,shut-null
guestw
hout HOST_MD5 md5sum __TEMP__ | cut -d' ' -f1
gout GUEST_MD5 md5sum test.bin | cut -d' ' -f1
check [ "__GUEST_MD5__" = "__HOST_MD5__" ]
test UDP/IPv4: guest to host
-hostb (socat -u UDP4-LISTEN:10003,bind=127.0.0.1 STDOUT & echo $! > __SC_PID__) | tee __TEMP__ | (grep -qm1 "END_OF_TEST" && kill $(cat __SC_PID__))
+hostb socat -u UDP4-LISTEN:10003,bind=127.0.0.1,null-eof OPEN:__TEMP__,create,trunc
gout GW ip -j -4 route show|jq -rM '.[] | select(.dst == "default").gateway'
-guest socat -u OPEN:test.bin UDP4:__GW__:10003
+guest socat -u OPEN:test.bin UDP4:__GW__:10003,shut-null
hostw
hout HOST_MD5 md5sum __TEMP__ | cut -d' ' -f1
check [ "__GUEST_MD5__" = "__HOST_MD5__" ]
test UDP/IPv6: host to guest
-guestb (socat -u UDP6-LISTEN:10001 STDOUT & echo $! > sc.pid) | tee test.bin | (grep -qm1 "END_OF_TEST" && kill $(cat sc.pid))
+guestb socat -u UDP6-LISTEN:10001,null-eof OPEN:test.bin,create,trunc
sleep 1
-host socat -u OPEN:__TEMP__ UDP6:[::1]:10001
+host socat -u OPEN:__TEMP__ UDP6:[::1]:10001,shut-null
guestw
gout GUEST_MD5 md5sum test.bin | cut -d' ' -f1
check [ "__GUEST_MD5__" = "__HOST_MD5__" ]
test UDP/IPv6: guest to host
-hostb (socat -u UDP6-LISTEN:10003,bind=[::1] STDOUT & echo $! > __SC_PID__) | tee __TEMP__ | (grep -qm1 "END_OF_TEST" && kill $(cat __SC_PID__))
+hostb socat -u UDP6-LISTEN:10003,bind=[::1],null-eof OPEN:__TEMP__,create,trunc
gout GW6 ip -j -6 route show|jq -rM '.[] | select(.dst == "default").gateway'
gout IFNAME ip -j link show | jq -rM '.[] | select(.link_type == "ether").ifname'
-guest socat -u OPEN:test.bin UDP6:[__GW6__%__IFNAME__]:10003
+guest socat -u OPEN:test.bin UDP6:[__GW6__%__IFNAME__]:10003,shut-null
hostw
hout HOST_MD5 md5sum __TEMP__ | cut -d' ' -f1
check [ "__GUEST_MD5__" = "__HOST_MD5__" ]
diff --git a/test/passt_in_ns/udp b/test/passt_in_ns/udp
index 3b1e521..5f01cbf 100644
--- a/test/passt_in_ns/udp
+++ b/test/passt_in_ns/udp
@@ -11,17 +11,16 @@
# Copyright (c) 2021 Red Hat GmbH
# Author: Stefano Brivio <sbrivio(a)redhat.com>
-gtools socat tee grep cat ip jq md5sum cut
-nstools socat tee grep cat ip jq md5sum cut
-htools printf dd socat tee grep cat ip jq md5sum cut
+gtools socat ip jq md5sum cut
+nstools socat ip jq md5sum cut
+htools dd socat ip jq md5sum cut
test UDP/IPv4: host to guest
set TEMP __STATEDIR__/data
-set SC_PID __STATEDIR__/socat.pid
-guestb (socat -u UDP4-LISTEN:10001 STDOUT & echo $! > sc.pid) | tee test.bin | (grep -qm1 "END_OF_TEST" && kill $(cat sc.pid))
+guestb socat -u UDP4-LISTEN:10001,null-eof OPEN:test.bin,create,trunc
sleep 1
-host dd if=/dev/urandom bs=1k count=5 > __TEMP__ && printf "\nEND_OF_TEST\n" >> __TEMP__
-host socat -u OPEN:__TEMP__ UDP4:127.0.0.1:10001
+host dd if=/dev/urandom bs=1k count=5 > __TEMP__
+host socat -u OPEN:__TEMP__ UDP4:127.0.0.1:10001,shut-null
guestw
hout MD5 md5sum __TEMP__ | cut -d' ' -f1
gout GUEST_MD5 md5sum test.bin | cut -d' ' -f1
@@ -29,114 +28,114 @@ check [ "__GUEST_MD5__" = "__MD5__" ]
test UDP/IPv4: host to ns
set TEMP_NS __STATEDIR__/data_ns
-nsb (socat -u UDP4-LISTEN:10002 STDOUT & echo $! > __SC_PID__) | tee __TEMP_NS__ | (grep -qm1 "END_OF_TEST" && kill $(cat __SC_PID__))
-host socat -u OPEN:__TEMP__ UDP4:127.0.0.1:10002
+nsb socat -u UDP4-LISTEN:10002,null-eof OPEN:__TEMP_NS__,create,trunc
+host socat -u OPEN:__TEMP__ UDP4:127.0.0.1:10002,shut-null
nsw
nsout NS_MD5 md5sum __TEMP_NS__ | cut -d' ' -f1
check [ "__NS_MD5__" = "__MD5__" ]
test UDP/IPv4: guest to host
-hostb (socat -u UDP4-LISTEN:10003 STDOUT & echo $! > __SC_PID__) | tee __TEMP__ | (grep -qm1 "END_OF_TEST" && kill $(cat __SC_PID__))
+hostb socat -u UDP4-LISTEN:10003,null-eof OPEN:__TEMP__,create,trunc
gout GW ip -j -4 route show|jq -rM '.[] | select(.dst == "default").gateway'
-guest socat -u OPEN:test.bin UDP4:__GW__:10003
+guest socat -u OPEN:test.bin UDP4:__GW__:10003,shut-null
hostw
hout HOST_MD5 md5sum __TEMP__ | cut -d' ' -f1
check [ "__HOST_MD5__" = "__MD5__" ]
test UDP/IPv4: guest to ns
-nsb (socat -u UDP4-LISTEN:10002 STDOUT & echo $! > __SC_PID__) | tee __TEMP_NS__ | (grep -qm1 "END_OF_TEST" && kill $(cat __SC_PID__))
-guest socat -u OPEN:test.bin UDP4:__GW__:10002
+nsb socat -u UDP4-LISTEN:10002,null-eof OPEN:__TEMP_NS__,create,trunc
+guest socat -u OPEN:test.bin UDP4:__GW__:10002,shut-null
nsw
hout HOST_MD5 md5sum __TEMP__ | cut -d' ' -f1
check [ "__HOST_MD5__" = "__MD5__" ]
test UDP/IPv4: ns to host (recvmmsg/sendmmsg)
sleep 1
-hostb (socat -u UDP4-LISTEN:10003 STDOUT & echo $! > __SC_PID__) | tee __TEMP__ | (grep -qm1 "END_OF_TEST" && kill $(cat __SC_PID__))
-ns socat -u OPEN:__TEMP_NS__ UDP4:127.0.0.1:10003
+hostb socat -u UDP4-LISTEN:10003,null-eof OPEN:__TEMP__,create,trunc
+ns socat -u OPEN:__TEMP_NS__ UDP4:127.0.0.1:10003,shut-null
hostw
hout HOST_MD5 md5sum __TEMP__ | cut -d' ' -f1
check [ "__HOST_MD5__" = "__MD5__" ]
test UDP/IPv4: ns to host (via tap)
-hostb (socat -u UDP4-LISTEN:10003 STDOUT & echo $! > __SC_PID__) | tee __TEMP__ | (grep -qm1 "END_OF_TEST" && kill $(cat __SC_PID__))
-ns socat -u OPEN:__TEMP_NS__ UDP4:__GW__:10003
+hostb socat -u UDP4-LISTEN:10003,null-eof OPEN:__TEMP__,create,trunc
+ns socat -u OPEN:__TEMP_NS__ UDP4:__GW__:10003,shut-null
hostw
hout HOST_MD5 md5sum __TEMP__ | cut -d' ' -f1
check [ "__HOST_MD5__" = "__MD5__" ]
test UDP/IPv4: ns to guest (using loopback address)
-guestb (socat -u UDP4-LISTEN:10001 STDOUT & echo $! > sc.pid) | tee test.bin | (grep -qm1 "END_OF_TEST" && kill $(cat sc.pid))
-ns socat -u OPEN:__TEMP_NS__ UDP4:127.0.0.1:10001
+guestb socat -u UDP4-LISTEN:10001,null-eof OPEN:test.bin,create,trunc
+ns socat -u OPEN:__TEMP_NS__ UDP4:127.0.0.1:10001,shut-null
guestw
gout GUEST_MD5 md5sum test.bin | cut -d' ' -f1
check [ "__GUEST_MD5__" = "__MD5__" ]
test UDP/IPv4: ns to guest (using namespace address)
-guestb (socat -u UDP4-LISTEN:10001 STDOUT & echo $! > sc.pid) | tee test.bin | (grep -qm1 "END_OF_TEST" && kill $(cat sc.pid))
+guestb socat -u UDP4-LISTEN:10001,null-eof OPEN:test.bin,create,trunc
nsout IFNAME ip -j link show | jq -rM '.[] | select(.link_type == "ether").ifname'
nsout ADDR ip -j -4 addr show|jq -rM '.[] | select(.ifname == "__IFNAME__").addr_info[0].local'
-ns socat -u OPEN:__TEMP_NS__ UDP4:__ADDR__:10001
+ns socat -u OPEN:__TEMP_NS__ UDP4:__ADDR__:10001,shut-null
guestw
gout GUEST_MD5 md5sum test.bin | cut -d' ' -f1
check [ "__GUEST_MD5__" = "__MD5__" ]
test UDP/IPv6: host to guest
-guestb (socat -u UDP6-LISTEN:10001 STDOUT & echo $! > sc.pid) | tee test.bin | (grep -qm1 "END_OF_TEST" && kill $(cat sc.pid))
-host socat -u OPEN:__TEMP__ UDP6:[::1]:10001
+guestb socat -u UDP6-LISTEN:10001,null-eof OPEN:test.bin,create,trunc
+host socat -u OPEN:__TEMP__ UDP6:[::1]:10001,shut-null
guestw
gout GUEST_MD5 md5sum test.bin | cut -d' ' -f1
check [ "__GUEST_MD5__" = "__MD5__" ]
test UDP/IPv6: host to ns
-nsb (socat -u UDP6-LISTEN:10002 STDOUT & echo $! > __SC_PID__) | tee __TEMP_NS__ | (grep -qm1 "END_OF_TEST" && kill $(cat __SC_PID__))
-host socat -u OPEN:__TEMP__ UDP6:[::1]:10002
+nsb socat -u UDP6-LISTEN:10002,null-eof OPEN:__TEMP_NS__,create,trunc
+host socat -u OPEN:__TEMP__ UDP6:[::1]:10002,shut-null
nsw
nsout NS_MD5 md5sum __TEMP_NS__ | cut -d' ' -f1
check [ "__NS_MD5__" = "__MD5__" ]
test UDP/IPv6: guest to host
-hostb (socat -u UDP6-LISTEN:10003 STDOUT & echo $! > __SC_PID__) | tee __TEMP__ | (grep -qm1 "END_OF_TEST" && kill $(cat __SC_PID__))
+hostb socat -u UDP6-LISTEN:10003,null-eof OPEN:__TEMP__,create,trunc
gout GW6 ip -j -6 route show|jq -rM '.[] | select(.dst == "default").gateway'
gout IFNAME ip -j link show | jq -rM '.[] | select(.link_type == "ether").ifname'
-guest socat -u OPEN:test.bin UDP6:[__GW6__%__IFNAME__]:10003
+guest socat -u OPEN:test.bin UDP6:[__GW6__%__IFNAME__]:10003,shut-null
hostw
hout HOST_MD5 md5sum __TEMP__ | cut -d' ' -f1
check [ "__HOST_MD5__" = "__MD5__" ]
test UDP/IPv6: guest to ns
-nsb (socat -u UDP6-LISTEN:10002 STDOUT & echo $! > __SC_PID__) | tee __TEMP_NS__ | (grep -qm1 "END_OF_TEST" && kill $(cat __SC_PID__))
-guest socat -u OPEN:test.bin UDP6:[__GW6__%__IFNAME__]:10002
+nsb socat -u UDP6-LISTEN:10002,null-eof OPEN:__TEMP_NS__,create,trunc
+guest socat -u OPEN:test.bin UDP6:[__GW6__%__IFNAME__]:10002,shut-null
nsw
hout HOST_MD5 md5sum __TEMP__ | cut -d' ' -f1
check [ "__HOST_MD5__" = "__MD5__" ]
test UDP/IPv6: ns to host (recvmmsg/sendmmsg)
-hostb (socat -u UDP6-LISTEN:10003 STDOUT & echo $! > __SC_PID__) | tee __TEMP__ | (grep -qm1 "END_OF_TEST" && kill $(cat __SC_PID__))
-ns socat -u OPEN:__TEMP_NS__ UDP6:[::1]:10003
+hostb socat -u UDP6-LISTEN:10003,null-eof OPEN:__TEMP__,create,trunc
+ns socat -u OPEN:__TEMP_NS__ UDP6:[::1]:10003,shut-null
hostw
hout HOST_MD5 md5sum __TEMP__ | cut -d' ' -f1
check [ "__HOST_MD5__" = "__MD5__" ]
test UDP/IPv6: ns to host (via tap)
-hostb (socat -u UDP6-LISTEN:10003 STDOUT & echo $! > __SC_PID__) | tee __TEMP__ | (grep -qm1 "END_OF_TEST" && kill $(cat __SC_PID__))
+hostb socat -u UDP6-LISTEN:10003,null-eof OPEN:__TEMP__,create,trunc
nsout IFNAME ip -j link show | jq -rM '.[] | select(.link_type == "ether").ifname'
-ns socat -u OPEN:__TEMP_NS__ UDP6:[__GW6__%__IFNAME__]:10003
+ns socat -u OPEN:__TEMP_NS__ UDP6:[__GW6__%__IFNAME__]:10003,shut-null
hostw
hout HOST_MD5 md5sum __TEMP__ | cut -d' ' -f1
check [ "__HOST_MD5__" = "__MD5__" ]
test UDP/IPv6: ns to guest (using loopback address)
-guestb (socat -u UDP6-LISTEN:10001 STDOUT & echo $! > sc.pid) | tee test.bin | (grep -qm1 "END_OF_TEST" && kill $(cat sc.pid))
-ns socat -u OPEN:__TEMP_NS__ UDP6:[::1]:10001
+guestb socat -u UDP6-LISTEN:10001,null-eof OPEN:test.bin,create,trunc
+ns socat -u OPEN:__TEMP_NS__ UDP6:[::1]:10001,shut-null
guestw
gout GUEST_MD5 md5sum test.bin | cut -d' ' -f1
check [ "__GUEST_MD5__" = "__MD5__" ]
test UDP/IPv6: ns to guest (using namespace address)
-guestb (socat -u UDP6-LISTEN:10001 STDOUT & echo $! > sc.pid) | tee test.bin | (grep -qm1 "END_OF_TEST" && kill $(cat sc.pid))
+guestb socat -u UDP6-LISTEN:10001,null-eof OPEN:test.bin,create,trunc
nsout ADDR6 ip -j -6 addr show|jq -rM '.[] | select(.ifname == "__IFNAME__").addr_info[0].local'
-ns socat -u OPEN:__TEMP_NS__ UDP6:[__ADDR6__]:10001
+ns socat -u OPEN:__TEMP_NS__ UDP6:[__ADDR6__]:10001,shut-null
guestw
gout GUEST_MD5 md5sum test.bin | cut -d' ' -f1
check [ "__GUEST_MD5__" = "__MD5__" ]
diff --git a/test/pasta/udp b/test/pasta/udp
index 74148e3..fc6bf0a 100644
--- a/test/pasta/udp
+++ b/test/pasta/udp
@@ -11,59 +11,58 @@
# Copyright (c) 2021 Red Hat GmbH
# Author: Stefano Brivio <sbrivio(a)redhat.com>
-nstools socat tee grep cat ip jq md5sum cut
-htools printf dd socat tee grep cat ip jq md5sum cut
+nstools socat ip jq md5sum cut
+htools dd socat ip jq md5sum cut
test UDP/IPv4: host to ns
set TEMP __STATEDIR__/data
set TEMP_NS __STATEDIR__/data_ns
-set SC_PID __STATEDIR__/socat.pid
-nsb (socat -u UDP4-LISTEN:10002,bind=127.0.0.1 STDOUT & echo $! > __SC_PID__) | tee __TEMP_NS__ | (grep -qm1 "END_OF_TEST" && kill $(cat __SC_PID__))
-host dd if=/dev/urandom bs=1k count=5 > __TEMP__ && printf "\nEND_OF_TEST\n" >> __TEMP__
+nsb socat -u UDP4-LISTEN:10002,bind=127.0.0.1,null-eof OPEN:__TEMP_NS__,create,trunc
+host dd if=/dev/urandom bs=1k count=5 > __TEMP__
-host socat OPEN:__TEMP__ UDP4:127.0.0.1:10002
+host socat OPEN:__TEMP__ UDP4:127.0.0.1:10002,shut-null
nsw
hout MD5 md5sum __TEMP__ | cut -d' ' -f1
nsout NS_MD5 md5sum __TEMP_NS__ | cut -d' ' -f1
check [ "__NS_MD5__" = "__MD5__" ]
test UDP/IPv4: ns to host (recvmmsg/sendmmsg)
-hostb (socat -u UDP4-LISTEN:10003,bind=127.0.0.1 STDOUT & echo $! > __SC_PID__) | tee __TEMP__ | (grep -qm1 "END_OF_TEST" && kill $(cat __SC_PID__))
+hostb socat -u UDP4-LISTEN:10003,bind=127.0.0.1,null-eof OPEN:__TEMP__,create,trunc
sleep 1
-ns socat OPEN:__TEMP_NS__ UDP4:127.0.0.1:10003
+ns socat OPEN:__TEMP_NS__ UDP4:127.0.0.1:10003,shut-null
hostw
hout HOST_MD5 md5sum __TEMP__ | cut -d' ' -f1
check [ "__HOST_MD5__" = "__MD5__" ]
test UDP/IPv4: ns to host (via tap)
-hostb (socat -u UDP4-LISTEN:10003 STDOUT & echo $! > __SC_PID__) | tee __TEMP__ | (grep -qm1 "END_OF_TEST" && kill $(cat __SC_PID__))
+hostb socat -u UDP4-LISTEN:10003,null-eof OPEN:__TEMP__,create,trunc
nsout GW ip -j -4 route show|jq -rM '.[] | select(.dst == "default").gateway'
-ns socat -u OPEN:__TEMP_NS__ UDP4:__GW__:10003
+ns socat -u OPEN:__TEMP_NS__ UDP4:__GW__:10003,shut-null
hostw
hout HOST_MD5 md5sum __TEMP__ | cut -d' ' -f1
check [ "__HOST_MD5__" = "__MD5__" ]
test UDP/IPv6: host to ns
-nsb (socat -u UDP6-LISTEN:10002,bind=[::1] STDOUT & echo $! > __SC_PID__) | tee __TEMP_NS__ | (grep -qm1 "END_OF_TEST" && kill $(cat __SC_PID__))
-host socat -u OPEN:__TEMP__ UDP6:[::1]:10002
+nsb socat -u UDP6-LISTEN:10002,bind=[::1],null-eof OPEN:__TEMP_NS__,create,trunc
+host socat -u OPEN:__TEMP__ UDP6:[::1]:10002,shut-null
nsw
hout MD5 md5sum __TEMP__ | cut -d' ' -f1
nsout NS_MD5 md5sum __TEMP_NS__ | cut -d' ' -f1
check [ "__NS_MD5__" = "__MD5__" ]
test UDP/IPv6: ns to host (recvmmsg/sendmmsg)
-hostb (socat -u UDP6-LISTEN:10003,bind=[::1] STDOUT & echo $! > __SC_PID__) | tee __TEMP__ | (grep -qm1 "END_OF_TEST" && kill $(cat __SC_PID__))
+hostb socat -u UDP6-LISTEN:10003,bind=[::1],null-eof OPEN:__TEMP__,create,trunc
sleep 1
-ns socat -u OPEN:__TEMP_NS__ UDP6:[::1]:10003
+ns socat -u OPEN:__TEMP_NS__ UDP6:[::1]:10003,shut-null
hostw
hout HOST_MD5 md5sum __TEMP__ | cut -d' ' -f1
check [ "__HOST_MD5__" = "__MD5__" ]
test UDP/IPv6: ns to host (via tap)
-hostb (socat -u UDP6-LISTEN:10003 STDOUT & echo $! > __SC_PID__) | tee __TEMP__ | (grep -qm1 "END_OF_TEST" && kill $(cat __SC_PID__))
+hostb socat -u UDP6-LISTEN:10003,null-eof OPEN:__TEMP__,create,trunc
nsout GW6 ip -j -6 route show|jq -rM '.[] | select(.dst == "default").gateway'
nsout IFNAME ip -j link show | jq -rM '.[] | select(.link_type == "ether").ifname'
-ns socat -u OPEN:__TEMP_NS__ UDP6:[__GW6__%__IFNAME__]:10003
+ns socat -u OPEN:__TEMP_NS__ UDP6:[__GW6__%__IFNAME__]:10003,shut-null
hostw
hout HOST_MD5 md5sum __TEMP__ | cut -d' ' -f1
check [ "__HOST_MD5__" = "__MD5__" ]
--
@@ -11,59 +11,58 @@
# Copyright (c) 2021 Red Hat GmbH
# Author: Stefano Brivio <sbrivio(a)redhat.com>
-nstools socat tee grep cat ip jq md5sum cut
-htools printf dd socat tee grep cat ip jq md5sum cut
+nstools socat ip jq md5sum cut
+htools dd socat ip jq md5sum cut
test UDP/IPv4: host to ns
set TEMP __STATEDIR__/data
set TEMP_NS __STATEDIR__/data_ns
-set SC_PID __STATEDIR__/socat.pid
-nsb (socat -u UDP4-LISTEN:10002,bind=127.0.0.1 STDOUT & echo $! > __SC_PID__) | tee __TEMP_NS__ | (grep -qm1 "END_OF_TEST" && kill $(cat __SC_PID__))
-host dd if=/dev/urandom bs=1k count=5 > __TEMP__ && printf "\nEND_OF_TEST\n" >> __TEMP__
+nsb socat -u UDP4-LISTEN:10002,bind=127.0.0.1,null-eof OPEN:__TEMP_NS__,create,trunc
+host dd if=/dev/urandom bs=1k count=5 > __TEMP__
-host socat OPEN:__TEMP__ UDP4:127.0.0.1:10002
+host socat OPEN:__TEMP__ UDP4:127.0.0.1:10002,shut-null
nsw
hout MD5 md5sum __TEMP__ | cut -d' ' -f1
nsout NS_MD5 md5sum __TEMP_NS__ | cut -d' ' -f1
check [ "__NS_MD5__" = "__MD5__" ]
test UDP/IPv4: ns to host (recvmmsg/sendmmsg)
-hostb (socat -u UDP4-LISTEN:10003,bind=127.0.0.1 STDOUT & echo $! > __SC_PID__) | tee __TEMP__ | (grep -qm1 "END_OF_TEST" && kill $(cat __SC_PID__))
+hostb socat -u UDP4-LISTEN:10003,bind=127.0.0.1,null-eof OPEN:__TEMP__,create,trunc
sleep 1
-ns socat OPEN:__TEMP_NS__ UDP4:127.0.0.1:10003
+ns socat OPEN:__TEMP_NS__ UDP4:127.0.0.1:10003,shut-null
hostw
hout HOST_MD5 md5sum __TEMP__ | cut -d' ' -f1
check [ "__HOST_MD5__" = "__MD5__" ]
test UDP/IPv4: ns to host (via tap)
-hostb (socat -u UDP4-LISTEN:10003 STDOUT & echo $! > __SC_PID__) | tee __TEMP__ | (grep -qm1 "END_OF_TEST" && kill $(cat __SC_PID__))
+hostb socat -u UDP4-LISTEN:10003,null-eof OPEN:__TEMP__,create,trunc
nsout GW ip -j -4 route show|jq -rM '.[] | select(.dst == "default").gateway'
-ns socat -u OPEN:__TEMP_NS__ UDP4:__GW__:10003
+ns socat -u OPEN:__TEMP_NS__ UDP4:__GW__:10003,shut-null
hostw
hout HOST_MD5 md5sum __TEMP__ | cut -d' ' -f1
check [ "__HOST_MD5__" = "__MD5__" ]
test UDP/IPv6: host to ns
-nsb (socat -u UDP6-LISTEN:10002,bind=[::1] STDOUT & echo $! > __SC_PID__) | tee __TEMP_NS__ | (grep -qm1 "END_OF_TEST" && kill $(cat __SC_PID__))
-host socat -u OPEN:__TEMP__ UDP6:[::1]:10002
+nsb socat -u UDP6-LISTEN:10002,bind=[::1],null-eof OPEN:__TEMP_NS__,create,trunc
+host socat -u OPEN:__TEMP__ UDP6:[::1]:10002,shut-null
nsw
hout MD5 md5sum __TEMP__ | cut -d' ' -f1
nsout NS_MD5 md5sum __TEMP_NS__ | cut -d' ' -f1
check [ "__NS_MD5__" = "__MD5__" ]
test UDP/IPv6: ns to host (recvmmsg/sendmmsg)
-hostb (socat -u UDP6-LISTEN:10003,bind=[::1] STDOUT & echo $! > __SC_PID__) | tee __TEMP__ | (grep -qm1 "END_OF_TEST" && kill $(cat __SC_PID__))
+hostb socat -u UDP6-LISTEN:10003,bind=[::1],null-eof OPEN:__TEMP__,create,trunc
sleep 1
-ns socat -u OPEN:__TEMP_NS__ UDP6:[::1]:10003
+ns socat -u OPEN:__TEMP_NS__ UDP6:[::1]:10003,shut-null
hostw
hout HOST_MD5 md5sum __TEMP__ | cut -d' ' -f1
check [ "__HOST_MD5__" = "__MD5__" ]
test UDP/IPv6: ns to host (via tap)
-hostb (socat -u UDP6-LISTEN:10003 STDOUT & echo $! > __SC_PID__) | tee __TEMP__ | (grep -qm1 "END_OF_TEST" && kill $(cat __SC_PID__))
+hostb socat -u UDP6-LISTEN:10003,null-eof OPEN:__TEMP__,create,trunc
nsout GW6 ip -j -6 route show|jq -rM '.[] | select(.dst == "default").gateway'
nsout IFNAME ip -j link show | jq -rM '.[] | select(.link_type == "ether").ifname'
-ns socat -u OPEN:__TEMP_NS__ UDP6:[__GW6__%__IFNAME__]:10003
+ns socat -u OPEN:__TEMP_NS__ UDP6:[__GW6__%__IFNAME__]:10003,shut-null
hostw
hout HOST_MD5 md5sum __TEMP__ | cut -d' ' -f1
check [ "__HOST_MD5__" = "__MD5__" ]
--
2.37.3
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH 1/2] udp: Don't drop zero-length outbound UDP packets
2022-09-09 4:27 ` [PATCH 1/2] udp: Don't drop zero-length outbound UDP packets David Gibson
@ 2022-09-09 9:26 ` Stefano Brivio
2022-09-09 10:39 ` David Gibson
0 siblings, 1 reply; 9+ messages in thread
From: Stefano Brivio @ 2022-09-09 9:26 UTC (permalink / raw)
To: passt-dev
[-- Attachment #1: Type: text/plain, Size: 1989 bytes --]
On Fri, 9 Sep 2022 14:27:13 +1000
David Gibson <david(a)gibson.dropbear.id.au> wrote:
> udp_tap_handler() currently skips outbound packets if they have a payload
> length of zero. This is not correct, since in a datagram protocol zero
> length packets still have meaning.
Right, nice catch. As far as I can tell it's an issue I added with
commit bb708111833e ("treewide: Packet abstraction with mandatory
boundary checks").
> Adjust this to correctly forward the zero-length packets by using a msghdr
> with msg_iovlen == 0.
>
> Bugzilla: https://bugs.passt.top/show_bug.cgi?id=19
>
> Signed-off-by: David Gibson <david(a)gibson.dropbear.id.au>
> ---
> udp.c | 10 +++++-----
> 1 file changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/udp.c b/udp.c
> index c4ebecc..caa852a 100644
> --- a/udp.c
> +++ b/udp.c
> @@ -1075,19 +1075,19 @@ int udp_tap_handler(struct ctx *c, int af, const void *addr,
> uh_send = packet_get(p, i, 0, sizeof(*uh), &len);
> if (!uh_send)
> return p->count;
> +
> + mm[i].msg_hdr.msg_name = sa;
> + mm[i].msg_hdr.msg_namelen = sl;
> + count++;
> +
> if (!len)
> continue;
>
> m[i].iov_base = (char *)(uh_send + 1);
> m[i].iov_len = len;
I haven't tested this yet, but:
- shouldn't iov_len be set to 0 (moving also this line before)? Note
that I'm not initialising m
- shouldn't iov_base point to NULL to avoid noise from valgrind?
Also:
>
> - mm[i].msg_hdr.msg_name = sa;
> - mm[i].msg_hdr.msg_namelen = sl;
> -
> mm[i].msg_hdr.msg_iov = m + i;
> mm[i].msg_hdr.msg_iovlen = 1;
...I guess we should still go through those even if the size is zero,
because we're appending a message. If we don't, I would expect some
subsequent messages in the batch to be dropped (as many as zero sized
packets we have).
That is, I suppose we could just drop the continue statement on if
(!len) above -- but, again, I haven't tested it.
--
Stefano
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 1/2] udp: Don't drop zero-length outbound UDP packets
2022-09-09 9:26 ` Stefano Brivio
@ 2022-09-09 10:39 ` David Gibson
2022-09-09 16:06 ` Stefano Brivio
0 siblings, 1 reply; 9+ messages in thread
From: David Gibson @ 2022-09-09 10:39 UTC (permalink / raw)
To: passt-dev
[-- Attachment #1: Type: text/plain, Size: 2767 bytes --]
On Fri, Sep 09, 2022 at 11:26:58AM +0200, Stefano Brivio wrote:
> On Fri, 9 Sep 2022 14:27:13 +1000
> David Gibson <david(a)gibson.dropbear.id.au> wrote:
>
> > udp_tap_handler() currently skips outbound packets if they have a payload
> > length of zero. This is not correct, since in a datagram protocol zero
> > length packets still have meaning.
>
> Right, nice catch. As far as I can tell it's an issue I added with
> commit bb708111833e ("treewide: Packet abstraction with mandatory
> boundary checks").
>
> > Adjust this to correctly forward the zero-length packets by using a msghdr
> > with msg_iovlen == 0.
> >
> > Bugzilla: https://bugs.passt.top/show_bug.cgi?id=19
> >
> > Signed-off-by: David Gibson <david(a)gibson.dropbear.id.au>
> > ---
> > udp.c | 10 +++++-----
> > 1 file changed, 5 insertions(+), 5 deletions(-)
> >
> > diff --git a/udp.c b/udp.c
> > index c4ebecc..caa852a 100644
> > --- a/udp.c
> > +++ b/udp.c
> > @@ -1075,19 +1075,19 @@ int udp_tap_handler(struct ctx *c, int af, const void *addr,
> > uh_send = packet_get(p, i, 0, sizeof(*uh), &len);
> > if (!uh_send)
> > return p->count;
> > +
> > + mm[i].msg_hdr.msg_name = sa;
> > + mm[i].msg_hdr.msg_namelen = sl;
> > + count++;
> > +
> > if (!len)
> > continue;
> >
> > m[i].iov_base = (char *)(uh_send + 1);
> > m[i].iov_len = len;
>
> I haven't tested this yet, but:
>
> - shouldn't iov_len be set to 0 (moving also this line before)? Note
> that I'm not initialising m
>
> - shouldn't iov_base point to NULL to avoid noise from valgrind?
No, because with this change m[i] is entirely unreferenced by mm[].
> Also:
>
> >
> > - mm[i].msg_hdr.msg_name = sa;
> > - mm[i].msg_hdr.msg_namelen = sl;
> > -
> > mm[i].msg_hdr.msg_iov = m + i;
> > mm[i].msg_hdr.msg_iovlen = 1;
>
> ...I guess we should still go through those even if the size is zero,
> because we're appending a message. If we don't, I would expect some
> subsequent messages in the batch to be dropped (as many as zero sized
> packets we have).
Here I'm relying on the fact that mm[] (unlike m[]) *is* initialized,
so if we don't alter it here, msg_iov is NULL and msg_iovlen is 0.
I was looking at removing that initialization, but I haven't gotten
that working yet.
> That is, I suppose we could just drop the continue statement on if
> (!len) above -- but, again, I haven't tested it.
My first version actually did that, so it also works, but I think
setting msg_iovlen to 0 is a bit neater.
--
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] 9+ messages in thread
* Re: [PATCH 1/2] udp: Don't drop zero-length outbound UDP packets
2022-09-09 10:39 ` David Gibson
@ 2022-09-09 16:06 ` Stefano Brivio
2022-09-13 6:39 ` David Gibson
0 siblings, 1 reply; 9+ messages in thread
From: Stefano Brivio @ 2022-09-09 16:06 UTC (permalink / raw)
To: passt-dev
[-- Attachment #1: Type: text/plain, Size: 3967 bytes --]
On Fri, 9 Sep 2022 20:39:44 +1000
David Gibson <david(a)gibson.dropbear.id.au> wrote:
> On Fri, Sep 09, 2022 at 11:26:58AM +0200, Stefano Brivio wrote:
> > On Fri, 9 Sep 2022 14:27:13 +1000
> > David Gibson <david(a)gibson.dropbear.id.au> wrote:
> >
> > > udp_tap_handler() currently skips outbound packets if they have a payload
> > > length of zero. This is not correct, since in a datagram protocol zero
> > > length packets still have meaning.
> >
> > Right, nice catch. As far as I can tell it's an issue I added with
> > commit bb708111833e ("treewide: Packet abstraction with mandatory
> > boundary checks").
> >
> > > Adjust this to correctly forward the zero-length packets by using a msghdr
> > > with msg_iovlen == 0.
> > >
> > > Bugzilla: https://bugs.passt.top/show_bug.cgi?id=19
> > >
> > > Signed-off-by: David Gibson <david(a)gibson.dropbear.id.au>
> > > ---
> > > udp.c | 10 +++++-----
> > > 1 file changed, 5 insertions(+), 5 deletions(-)
> > >
> > > diff --git a/udp.c b/udp.c
> > > index c4ebecc..caa852a 100644
> > > --- a/udp.c
> > > +++ b/udp.c
> > > @@ -1075,19 +1075,19 @@ int udp_tap_handler(struct ctx *c, int af, const void *addr,
> > > uh_send = packet_get(p, i, 0, sizeof(*uh), &len);
> > > if (!uh_send)
> > > return p->count;
> > > +
> > > + mm[i].msg_hdr.msg_name = sa;
> > > + mm[i].msg_hdr.msg_namelen = sl;
> > > + count++;
> > > +
> > > if (!len)
> > > continue;
> > >
> > > m[i].iov_base = (char *)(uh_send + 1);
> > > m[i].iov_len = len;
> >
> > I haven't tested this yet, but:
> >
> > - shouldn't iov_len be set to 0 (moving also this line before)? Note
> > that I'm not initialising m
> >
> > - shouldn't iov_base point to NULL to avoid noise from valgrind?
>
> No, because with this change m[i] is entirely unreferenced by mm[].
>
> > Also:
> >
> > >
> > > - mm[i].msg_hdr.msg_name = sa;
> > > - mm[i].msg_hdr.msg_namelen = sl;
> > > -
> > > mm[i].msg_hdr.msg_iov = m + i;
> > > mm[i].msg_hdr.msg_iovlen = 1;
> >
> > ...I guess we should still go through those even if the size is zero,
> > because we're appending a message. If we don't, I would expect some
> > subsequent messages in the batch to be dropped (as many as zero sized
> > packets we have).
>
> Here I'm relying on the fact that mm[] (unlike m[]) *is* initialized,
> so if we don't alter it here, msg_iov is NULL and msg_iovlen is 0.
> I was looking at removing that initialization, but I haven't gotten
> that working yet.
Oops, I see now.
So, I suppose that if you want to drop that initialisation, you might
need to zero msg_hdr.controllen as well.
And msg_hdr.control too: other than keeping valgrind happy, not leaking
random stuff to the kernel might make this marginally more secure.
That should be better than the huge memset() at the beginning, because
we're already writing to msg_iovlen anyway.
If you already tried that, though, I don't have any other quick idea.
By the way, I had a mechanism in place, just for TCP though, to avoid
reassigning those pointers and also length descriptors.
I got rid of it in commit 38fbfdbcb95d ("tcp: Get rid of iov with
cached MSS, drop sendmmsg(), add deferred flush") because it didn't
really help with throughput. I don't see any significant "userspace"
overhead on guest-to-host TCP paths with perf(1).
...maybe for UDP that's different, I haven't focused that much on UDP
performance.
> > That is, I suppose we could just drop the continue statement on if
> > (!len) above -- but, again, I haven't tested it.
>
> My first version actually did that, so it also works, but I think
> setting msg_iovlen to 0 is a bit neater.
Right. Maybe it was just me being thick, or perhaps that could use a
comment:
/* Zero-length packet: don't use any buffer, msg_iovlen is 0 */
if (!len)
continue;
--
Stefano
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 1/2] udp: Don't drop zero-length outbound UDP packets
2022-09-09 16:06 ` Stefano Brivio
@ 2022-09-13 6:39 ` David Gibson
2022-09-13 9:08 ` Stefano Brivio
0 siblings, 1 reply; 9+ messages in thread
From: David Gibson @ 2022-09-13 6:39 UTC (permalink / raw)
To: passt-dev
[-- Attachment #1: Type: text/plain, Size: 4684 bytes --]
On Fri, Sep 09, 2022 at 06:06:59PM +0200, Stefano Brivio wrote:
> On Fri, 9 Sep 2022 20:39:44 +1000
> David Gibson <david(a)gibson.dropbear.id.au> wrote:
>
> > On Fri, Sep 09, 2022 at 11:26:58AM +0200, Stefano Brivio wrote:
> > > On Fri, 9 Sep 2022 14:27:13 +1000
> > > David Gibson <david(a)gibson.dropbear.id.au> wrote:
> > >
> > > > udp_tap_handler() currently skips outbound packets if they have a payload
> > > > length of zero. This is not correct, since in a datagram protocol zero
> > > > length packets still have meaning.
> > >
> > > Right, nice catch. As far as I can tell it's an issue I added with
> > > commit bb708111833e ("treewide: Packet abstraction with mandatory
> > > boundary checks").
> > >
> > > > Adjust this to correctly forward the zero-length packets by using a msghdr
> > > > with msg_iovlen == 0.
> > > >
> > > > Bugzilla: https://bugs.passt.top/show_bug.cgi?id=19
> > > >
> > > > Signed-off-by: David Gibson <david(a)gibson.dropbear.id.au>
> > > > ---
> > > > udp.c | 10 +++++-----
> > > > 1 file changed, 5 insertions(+), 5 deletions(-)
> > > >
> > > > diff --git a/udp.c b/udp.c
> > > > index c4ebecc..caa852a 100644
> > > > --- a/udp.c
> > > > +++ b/udp.c
> > > > @@ -1075,19 +1075,19 @@ int udp_tap_handler(struct ctx *c, int af, const void *addr,
> > > > uh_send = packet_get(p, i, 0, sizeof(*uh), &len);
> > > > if (!uh_send)
> > > > return p->count;
> > > > +
> > > > + mm[i].msg_hdr.msg_name = sa;
> > > > + mm[i].msg_hdr.msg_namelen = sl;
> > > > + count++;
> > > > +
> > > > if (!len)
> > > > continue;
> > > >
> > > > m[i].iov_base = (char *)(uh_send + 1);
> > > > m[i].iov_len = len;
> > >
> > > I haven't tested this yet, but:
> > >
> > > - shouldn't iov_len be set to 0 (moving also this line before)? Note
> > > that I'm not initialising m
> > >
> > > - shouldn't iov_base point to NULL to avoid noise from valgrind?
> >
> > No, because with this change m[i] is entirely unreferenced by mm[].
> >
> > > Also:
> > >
> > > >
> > > > - mm[i].msg_hdr.msg_name = sa;
> > > > - mm[i].msg_hdr.msg_namelen = sl;
> > > > -
> > > > mm[i].msg_hdr.msg_iov = m + i;
> > > > mm[i].msg_hdr.msg_iovlen = 1;
> > >
> > > ...I guess we should still go through those even if the size is zero,
> > > because we're appending a message. If we don't, I would expect some
> > > subsequent messages in the batch to be dropped (as many as zero sized
> > > packets we have).
> >
> > Here I'm relying on the fact that mm[] (unlike m[]) *is* initialized,
> > so if we don't alter it here, msg_iov is NULL and msg_iovlen is 0.
>
> > I was looking at removing that initialization, but I haven't gotten
> > that working yet.
>
> Oops, I see now.
>
> So, I suppose that if you want to drop that initialisation, you might
> need to zero msg_hdr.controllen as well.
Duh. I completely failed to consider the other fields. I actually
suspect msg_hdr.flags is the most vital one (without flags I don't
know if it will examine control or controllen). But in any case I'm
initializing them all now and it's working.
> And msg_hdr.control too: other than keeping valgrind happy, not leaking
> random stuff to the kernel might make this marginally more secure.
>
> That should be better than the huge memset() at the beginning, because
> we're already writing to msg_iovlen anyway.
>
> If you already tried that, though, I don't have any other quick idea.
>
> By the way, I had a mechanism in place, just for TCP though, to avoid
> reassigning those pointers and also length descriptors.
>
> I got rid of it in commit 38fbfdbcb95d ("tcp: Get rid of iov with
> cached MSS, drop sendmmsg(), add deferred flush") because it didn't
> really help with throughput. I don't see any significant "userspace"
> overhead on guest-to-host TCP paths with perf(1).
>
> ...maybe for UDP that's different, I haven't focused that much on UDP
> performance.
>
> > > That is, I suppose we could just drop the continue statement on if
> > > (!len) above -- but, again, I haven't tested it.
> >
> > My first version actually did that, so it also works, but I think
> > setting msg_iovlen to 0 is a bit neater.
>
> Right. Maybe it was just me being thick, or perhaps that could use a
> comment:
>
> /* Zero-length packet: don't use any buffer, msg_iovlen is 0 */
> if (!len)
> continue;
>
--
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] 9+ messages in thread
* Re: [PATCH 1/2] udp: Don't drop zero-length outbound UDP packets
2022-09-13 6:39 ` David Gibson
@ 2022-09-13 9:08 ` Stefano Brivio
2022-09-13 9:30 ` David Gibson
0 siblings, 1 reply; 9+ messages in thread
From: Stefano Brivio @ 2022-09-13 9:08 UTC (permalink / raw)
To: passt-dev
[-- Attachment #1: Type: text/plain, Size: 3786 bytes --]
On Tue, 13 Sep 2022 16:39:26 +1000
David Gibson <david(a)gibson.dropbear.id.au> wrote:
> On Fri, Sep 09, 2022 at 06:06:59PM +0200, Stefano Brivio wrote:
> > On Fri, 9 Sep 2022 20:39:44 +1000
> > David Gibson <david(a)gibson.dropbear.id.au> wrote:
> >
> > > On Fri, Sep 09, 2022 at 11:26:58AM +0200, Stefano Brivio wrote:
> > > > On Fri, 9 Sep 2022 14:27:13 +1000
> > > > David Gibson <david(a)gibson.dropbear.id.au> wrote:
> > > >
> > > > > udp_tap_handler() currently skips outbound packets if they have a payload
> > > > > length of zero. This is not correct, since in a datagram protocol zero
> > > > > length packets still have meaning.
> > > >
> > > > Right, nice catch. As far as I can tell it's an issue I added with
> > > > commit bb708111833e ("treewide: Packet abstraction with mandatory
> > > > boundary checks").
> > > >
> > > > > Adjust this to correctly forward the zero-length packets by using a msghdr
> > > > > with msg_iovlen == 0.
> > > > >
> > > > > Bugzilla: https://bugs.passt.top/show_bug.cgi?id=19
> > > > >
> > > > > Signed-off-by: David Gibson <david(a)gibson.dropbear.id.au>
> > > > > ---
> > > > > udp.c | 10 +++++-----
> > > > > 1 file changed, 5 insertions(+), 5 deletions(-)
> > > > >
> > > > > diff --git a/udp.c b/udp.c
> > > > > index c4ebecc..caa852a 100644
> > > > > --- a/udp.c
> > > > > +++ b/udp.c
> > > > > @@ -1075,19 +1075,19 @@ int udp_tap_handler(struct ctx *c, int af, const void *addr,
> > > > > uh_send = packet_get(p, i, 0, sizeof(*uh), &len);
> > > > > if (!uh_send)
> > > > > return p->count;
> > > > > +
> > > > > + mm[i].msg_hdr.msg_name = sa;
> > > > > + mm[i].msg_hdr.msg_namelen = sl;
> > > > > + count++;
> > > > > +
> > > > > if (!len)
> > > > > continue;
> > > > >
> > > > > m[i].iov_base = (char *)(uh_send + 1);
> > > > > m[i].iov_len = len;
> > > >
> > > > I haven't tested this yet, but:
> > > >
> > > > - shouldn't iov_len be set to 0 (moving also this line before)? Note
> > > > that I'm not initialising m
> > > >
> > > > - shouldn't iov_base point to NULL to avoid noise from valgrind?
> > >
> > > No, because with this change m[i] is entirely unreferenced by mm[].
> > >
> > > > Also:
> > > >
> > > > >
> > > > > - mm[i].msg_hdr.msg_name = sa;
> > > > > - mm[i].msg_hdr.msg_namelen = sl;
> > > > > -
> > > > > mm[i].msg_hdr.msg_iov = m + i;
> > > > > mm[i].msg_hdr.msg_iovlen = 1;
> > > >
> > > > ...I guess we should still go through those even if the size is zero,
> > > > because we're appending a message. If we don't, I would expect some
> > > > subsequent messages in the batch to be dropped (as many as zero sized
> > > > packets we have).
> > >
> > > Here I'm relying on the fact that mm[] (unlike m[]) *is* initialized,
> > > so if we don't alter it here, msg_iov is NULL and msg_iovlen is 0.
> >
> > > I was looking at removing that initialization, but I haven't gotten
> > > that working yet.
> >
> > Oops, I see now.
> >
> > So, I suppose that if you want to drop that initialisation, you might
> > need to zero msg_hdr.controllen as well.
>
> Duh. I completely failed to consider the other fields. I actually
> suspect msg_hdr.flags is the most vital one (without flags I don't
> know if it will examine control or controllen).
Hmm, if we're talking about msg_flags, it should be ignored on
sendmsg(), and only used for received messages flags (MSG_EOR,
MSG_TRUNC, MSG_CTRUNC, MSG_OOB, MSG_ERRQUEUE) on recvmsg().
But,
> But in any case I'm initializing them all now and it's working.
yes, I guess it's a good idea to avoid sending the kernel random bytes
there, in any case.
--
Stefano
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 1/2] udp: Don't drop zero-length outbound UDP packets
2022-09-13 9:08 ` Stefano Brivio
@ 2022-09-13 9:30 ` David Gibson
0 siblings, 0 replies; 9+ messages in thread
From: David Gibson @ 2022-09-13 9:30 UTC (permalink / raw)
To: passt-dev
[-- Attachment #1: Type: text/plain, Size: 4331 bytes --]
On Tue, Sep 13, 2022 at 10:08:46AM +0100, Stefano Brivio wrote:
> On Tue, 13 Sep 2022 16:39:26 +1000
> David Gibson <david(a)gibson.dropbear.id.au> wrote:
>
> > On Fri, Sep 09, 2022 at 06:06:59PM +0200, Stefano Brivio wrote:
> > > On Fri, 9 Sep 2022 20:39:44 +1000
> > > David Gibson <david(a)gibson.dropbear.id.au> wrote:
> > >
> > > > On Fri, Sep 09, 2022 at 11:26:58AM +0200, Stefano Brivio wrote:
> > > > > On Fri, 9 Sep 2022 14:27:13 +1000
> > > > > David Gibson <david(a)gibson.dropbear.id.au> wrote:
> > > > >
> > > > > > udp_tap_handler() currently skips outbound packets if they have a payload
> > > > > > length of zero. This is not correct, since in a datagram protocol zero
> > > > > > length packets still have meaning.
> > > > >
> > > > > Right, nice catch. As far as I can tell it's an issue I added with
> > > > > commit bb708111833e ("treewide: Packet abstraction with mandatory
> > > > > boundary checks").
> > > > >
> > > > > > Adjust this to correctly forward the zero-length packets by using a msghdr
> > > > > > with msg_iovlen == 0.
> > > > > >
> > > > > > Bugzilla: https://bugs.passt.top/show_bug.cgi?id=19
> > > > > >
> > > > > > Signed-off-by: David Gibson <david(a)gibson.dropbear.id.au>
> > > > > > ---
> > > > > > udp.c | 10 +++++-----
> > > > > > 1 file changed, 5 insertions(+), 5 deletions(-)
> > > > > >
> > > > > > diff --git a/udp.c b/udp.c
> > > > > > index c4ebecc..caa852a 100644
> > > > > > --- a/udp.c
> > > > > > +++ b/udp.c
> > > > > > @@ -1075,19 +1075,19 @@ int udp_tap_handler(struct ctx *c, int af, const void *addr,
> > > > > > uh_send = packet_get(p, i, 0, sizeof(*uh), &len);
> > > > > > if (!uh_send)
> > > > > > return p->count;
> > > > > > +
> > > > > > + mm[i].msg_hdr.msg_name = sa;
> > > > > > + mm[i].msg_hdr.msg_namelen = sl;
> > > > > > + count++;
> > > > > > +
> > > > > > if (!len)
> > > > > > continue;
> > > > > >
> > > > > > m[i].iov_base = (char *)(uh_send + 1);
> > > > > > m[i].iov_len = len;
> > > > >
> > > > > I haven't tested this yet, but:
> > > > >
> > > > > - shouldn't iov_len be set to 0 (moving also this line before)? Note
> > > > > that I'm not initialising m
> > > > >
> > > > > - shouldn't iov_base point to NULL to avoid noise from valgrind?
> > > >
> > > > No, because with this change m[i] is entirely unreferenced by mm[].
> > > >
> > > > > Also:
> > > > >
> > > > > >
> > > > > > - mm[i].msg_hdr.msg_name = sa;
> > > > > > - mm[i].msg_hdr.msg_namelen = sl;
> > > > > > -
> > > > > > mm[i].msg_hdr.msg_iov = m + i;
> > > > > > mm[i].msg_hdr.msg_iovlen = 1;
> > > > >
> > > > > ...I guess we should still go through those even if the size is zero,
> > > > > because we're appending a message. If we don't, I would expect some
> > > > > subsequent messages in the batch to be dropped (as many as zero sized
> > > > > packets we have).
> > > >
> > > > Here I'm relying on the fact that mm[] (unlike m[]) *is* initialized,
> > > > so if we don't alter it here, msg_iov is NULL and msg_iovlen is 0.
> > >
> > > > I was looking at removing that initialization, but I haven't gotten
> > > > that working yet.
> > >
> > > Oops, I see now.
> > >
> > > So, I suppose that if you want to drop that initialisation, you might
> > > need to zero msg_hdr.controllen as well.
> >
> > Duh. I completely failed to consider the other fields. I actually
> > suspect msg_hdr.flags is the most vital one (without flags I don't
> > know if it will examine control or controllen).
>
> Hmm, if we're talking about msg_flags, it should be ignored on
> sendmsg(), and only used for received messages flags (MSG_EOR,
> MSG_TRUNC, MSG_CTRUNC, MSG_OOB, MSG_ERRQUEUE) on recvmsg().
Oh, right, I was mixing up msg_flags and the separate flags argument
to sendmsg() and sendmmsg().
> But,
>
> > But in any case I'm initializing them all now and it's working.
>
> yes, I guess it's a good idea to avoid sending the kernel random bytes
> there, in any case.
Right.
--
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] 9+ messages in thread
end of thread, other threads:[~2022-09-13 9:30 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-09-09 4:27 [PATCH 0/2] Don't drop outbound zero-length UDP packets over tap David Gibson
2022-09-09 4:27 ` [PATCH 1/2] udp: Don't drop zero-length outbound UDP packets David Gibson
2022-09-09 9:26 ` Stefano Brivio
2022-09-09 10:39 ` David Gibson
2022-09-09 16:06 ` Stefano Brivio
2022-09-13 6:39 ` David Gibson
2022-09-13 9:08 ` Stefano Brivio
2022-09-13 9:30 ` David Gibson
2022-09-09 4:27 ` [PATCH 2/2] test: Simpler termination handling for UDP tests David Gibson
Code repositories for project(s) associated with this public inbox
https://passt.top/passt
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for IMAP folder(s).