On Tue, Dec 13, 2022 at 11:48:47PM +0100, Stefano Brivio wrote:
...
  Sorry for the long delay here,
 
 On Mon,  5 Dec 2022 19:14:21 +1100
 David Gibson <david(a)gibson.dropbear.id.au> wrote:
 
  Usually udp_sock_handler() will receive and
forward multiple (up to 32)
 datagrams in udp_sock_handler(), then forward them all to the tap
 interface.  For unclear reasons, though, when in pasta mode we will only
 receive and forward a single datagram at a time.  Change it to receive
 multiple datagrams at once, like the other paths. 
 
 This is explained in the commit message of 6c931118643c ("tcp, udp:
 Receive batching doesn't pay off when writing single frames to tap").
 
 I think it's worth re-checking the throughput now as this path is a bit
 different, but unfortunately I didn't include this in the "perf" tests :(
 because at the time I introduced those I wasn't sure it even made sense to
 have traffic from the same host being directed to the tap device.
 
 The iperf3 runs were I observed this are actually the ones from the Podman
 demo. Ideally that case should be also checked in the perf/pasta_udp tests. 
Hm, ok.

...
  How fundamental is this for the rest of the series? I
couldn't find any
 actual dependency on this but I might be missing something. 
So the issue is that prior to this change in pasta we receive multiple
frames at once on the splice path, but one frame at a time on the tap
path.  By the end of this series we can't do that any more, because we
don't know before the recvmmsg() which one we'll be doing.

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

Sorry for the further delay,

On Wed, 14 Dec 2022 11:35:46 +0100
Stefano Brivio <sbrivio(a)redhat.com> wrote:

...
  On Wed, 14 Dec 2022 12:42:14 +1100
 David Gibson <david(a)gibson.dropbear.id.au> wrote:
 
  On Tue, Dec 13, 2022 at 11:48:47PM +0100, Stefano
Brivio wrote:  
  Sorry for the long delay here,
 
 On Mon,  5 Dec 2022 19:14:21 +1100
 David Gibson <david(a)gibson.dropbear.id.au> wrote:
     
  Usually udp_sock_handler() will receive and
forward multiple (up to 32)
 datagrams in udp_sock_handler(), then forward them all to the tap
 interface.  For unclear reasons, though, when in pasta mode we will only
 receive and forward a single datagram at a time.  Change it to receive
 multiple datagrams at once, like the other paths.     
 
 This is explained in the commit message of 6c931118643c ("tcp, udp:
 Receive batching doesn't pay off when writing single frames to tap").
 
 I think it's worth re-checking the throughput now as this path is a bit
 different, but unfortunately I didn't include this in the "perf" tests :(
 because at the time I introduced those I wasn't sure it even made sense to
 have traffic from the same host being directed to the tap device.
 
 The iperf3 runs were I observed this are actually the ones from the Podman
 demo. Ideally that case should be also checked in the perf/pasta_udp tests.     
 
 Hm, ok.
   
  How fundamental is this for the rest of the
series? I couldn't find any
 actual dependency on this but I might be missing something.     
 
 So the issue is that prior to this change in pasta we receive multiple
 frames at once on the splice path, but one frame at a time on the tap
 path.  By the end of this series we can't do that any more, because we
 don't know before the recvmmsg() which one we'll be doing.   
 
 Oh, right, I see. Then let me add this path to the perf/pasta_udp test
 and check how relevant this is now, I'll get back to you in a bit. 
I was checking the wrong path. With this:

--
diff --git a/test/perf/pasta_udp b/test/perf/pasta_udp
index 27ea724..973c2f4 100644
--- a/test/perf/pasta_udp
+++ b/test/perf/pasta_udp
@@ -31,6 +31,14 @@ report	pasta lo_udp 1 __FREQ__
 
 th	MTU 1500B 4000B 16384B 65535B
 
+tr	UDP throughput over IPv6: host to ns
+nsout	IFNAME ip -j link show | jq -rM '.[] | select(.link_type ==
"ether").ifname'
+nsout	ADDR6 ip -j -6 addr show|jq -rM '.[] | select(.ifname ==
"__IFNAME__").addr_info[] | select(.scope == "global" and .prefixlen
== 64).local'
+bw	-
+bw	-
+bw	-
+iperf3	BW host ns __ADDR6__ 100${i}2 __THREADS__ __TIME__ __OPTS__ -b 15G
+bw	__BW__ 7.0 9.0
 
 tr	UDP throughput over IPv6: ns to host
 ns	ip link set dev lo mtu 1500
diff --git a/test/run b/test/run
index e07513f..b53182b 100755
--- a/test/run
+++ b/test/run
@@ -67,6 +67,14 @@ run() {
 	test build/clang_tidy
 	teardown build
 
+	VALGRIND=0
+	setup passt_in_ns
+	test passt/ndp
+	test passt/dhcp
+	test perf/pasta_udp
+	test passt_in_ns/shutdown
+	teardown passt_in_ns
+
 	setup pasta
 	test pasta/ndp
 	test pasta/dhcp
--

I get 21.6 gbps after this series, and 29.7 gbps before -- it's quite
significant.

And there's nothing strange in perf's output, really, the distribution
of overhead per functions is pretty much the same, but writing multiple
messages to the tap device just takes more cycles per message compared
to a single message.

I'm a bit ashamed to propose this, but do you think about something
like:

	if (c->mode == MODE_PASTA) {
		if (recvmmsg(ref.r.s, mmh_recv, 1, 0, NULL) <= 0)
			return;

		if (udp_mmh_splice_port(v6, mmh_recv)) {
			n = recvmmsg(ref.r.s, mmh_recv + 1,
				     UDP_MAX_FRAMES - 1, 0, NULL);
		}

		if (n > 0)
			n++;
		else
			n = 1;
	} else {
		n = recvmmsg(ref.r.s, mmh_recv, UDP_MAX_FRAMES, 0, NULL);
		if (n <= 0)
			return;
	}

? Other than the inherent ugliness, it looks like a good approximation
to me.

-- 
Stefano

On Wed, 21 Dec 2022 17:00:24 +1100
David Gibson <david(a)gibson.dropbear.id.au> wrote:

...
  On Tue, Dec 20, 2022 at 11:42:46AM +0100, Stefano
Brivio wrote:
  Sorry for the further delay,
 
 On Wed, 14 Dec 2022 11:35:46 +0100
 Stefano Brivio <sbrivio(a)redhat.com> wrote:
   
  On Wed, 14 Dec 2022 12:42:14 +1100
 David Gibson <david(a)gibson.dropbear.id.au> wrote:
   
  On Tue, Dec 13, 2022 at 11:48:47PM +0100, Stefano
Brivio wrote:    
 > Sorry for the long delay here,
 > 
 > On Mon,  5 Dec 2022 19:14:21 +1100
 > David Gibson <david(a)gibson.dropbear.id.au> wrote:
 >       
 > > Usually udp_sock_handler() will receive and forward multiple (up to 32)
 > > datagrams in udp_sock_handler(), then forward them all to the tap
 > > interface.  For unclear reasons, though, when in pasta mode we will only
 > > receive and forward a single datagram at a time.  Change it to receive
 > > multiple datagrams at once, like the other paths.      
 > 
 > This is explained in the commit message of 6c931118643c ("tcp, udp:
 > Receive batching doesn't pay off when writing single frames to tap").
 > 
 > I think it's worth re-checking the throughput now as this path is a bit
 > different, but unfortunately I didn't include this in the "perf"
tests :(
 > because at the time I introduced those I wasn't sure it even made sense to
 > have traffic from the same host being directed to the tap device.
 > 
 > The iperf3 runs were I observed this are actually the ones from the Podman
 > demo. Ideally that case should be also checked in the perf/pasta_udp tests.      
 
 Hm, ok.
     
 > How fundamental is this for the rest of the series? I couldn't find any
 > actual dependency on this but I might be missing something.      
 
 So the issue is that prior to this change in pasta we receive multiple
 frames at once on the splice path, but one frame at a time on the tap
 path.  By the end of this series we can't do that any more, because we
 don't know before the recvmmsg() which one we'll be doing.     
 
 Oh, right, I see. Then let me add this path to the perf/pasta_udp test
 and check how relevant this is now, I'll get back to you in a bit.   
 
 I was checking the wrong path. With this:
 
 diff --git a/test/perf/pasta_udp b/test/perf/pasta_udp
 index 27ea724..973c2f4 100644
 --- a/test/perf/pasta_udp
 +++ b/test/perf/pasta_udp
 @@ -31,6 +31,14 @@ report	pasta lo_udp 1 __FREQ__
  
  th	MTU 1500B 4000B 16384B 65535B
  
 +tr	UDP throughput over IPv6: host to ns
 +nsout	IFNAME ip -j link show | jq -rM '.[] | select(.link_type ==
"ether").ifname'
 +nsout	ADDR6 ip -j -6 addr show|jq -rM '.[] | select(.ifname ==
"__IFNAME__").addr_info[] | select(.scope == "global" and .prefixlen
== 64).local'
 +bw	-
 +bw	-
 +bw	-
 +iperf3	BW host ns __ADDR6__ 100${i}2 __THREADS__ __TIME__ __OPTS__ -b 15G
 +bw	__BW__ 7.0 9.0
  
  tr	UDP throughput over IPv6: ns to host
  ns	ip link set dev lo mtu 1500
 diff --git a/test/run b/test/run
 index e07513f..b53182b 100755
 --- a/test/run
 +++ b/test/run
 @@ -67,6 +67,14 @@ run() {
  	test build/clang_tidy
  	teardown build
  
 +	VALGRIND=0
 +	setup passt_in_ns
 +	test passt/ndp
 +	test passt/dhcp
 +	test perf/pasta_udp
 +	test passt_in_ns/shutdown
 +	teardown passt_in_ns
 +
  	setup pasta
  	test pasta/ndp
  	test pasta/dhcp   
 
 Ah, ok.  Can we add that to the standard set of tests ASAP, please. 
Yes -- that part itself was easy, but now I'm fighting against my own
finest write-only code that generates the JavaScript snippet for the
performance report (perf_fill_lines() in test/lib/perf_report -- and
this is not a suggestion to have a look at it ;)).

I'm trying to rework it a bit together with the "new" test.

...
 
  I get 21.6
gbps after this series, and 29.7 gbps before -- it's quite
 significant.   
 
 Drat.
 
  And there's nothing strange in perf's
output, really, the distribution
 of overhead per functions is pretty much the same, but writing multiple
 messages to the tap device just takes more cycles per message compared
 to a single message.   
 
 That's so weird.  It should be basically an identical set of write()s,
 except that they happen in a batch, rather than a bit spread out.  I
 guess it has to be some kind of cache locality thing.  I wonder if the
 difference would go away or reverse if we had a way to submit multiple
 frames with a single syscall. 
I haven't tried, but to test this, I think we could actually just write
multiple frames in a single call, with subsequent headers and
everything, and the iperf3 server will simply report how many bytes it
received.

...
 
  I'm a bit
ashamed to propose this, but do you think about something
 like:   
 
  	if (c->mode == MODE_PASTA) { if
(recvmmsg(ref.r.s, mmh_recv,
 		1, 0, NULL) <= 0) return;   
 
  		if (udp_mmh_splice_port(v6, mmh_recv)) { n =
 			recvmmsg(ref.r.s, mmh_recv + 1, UDP_MAX_FRAMES
 			- 1, 0, NULL); }   
 
  		if (n > 0) n++; else n = 1; } else { n =
 			recvmmsg(ref.r.s, mmh_recv, UDP_MAX_FRAMES, 0,
 			NULL); if (n <= 0) return; }   
 
  ? Other than the inherent ugliness, it looks like
a good
 approximation to me.   
 
 Hmm.  Well, the first question is how much impact does going 1 message
 at a time have on the spliced throughput.  If it's not too bad, then
 we could just always go one at a time for pasta, regardless of
 splicing.  And we could even abstract that difference into the tap
 backend with a callback like tap_batch_size(c). 
Right... it used to be significantly worse in the "spliced" case, I
checked that when I did that commit to use 1 instead of UDP_MAX_FRAME
in the other case, but I don't have data. I'll test this again.

-- 
Stefano

On Wed, Jan 04, 2023 at 01:08:52AM +0100, Stefano Brivio wrote:
...
  On Wed, 21 Dec 2022 17:00:24 +1100
 David Gibson <david(a)gibson.dropbear.id.au> wrote:
 
  On Tue, Dec 20, 2022 at 11:42:46AM +0100, Stefano
Brivio wrote:
  Sorry for the further delay,
 
 On Wed, 14 Dec 2022 11:35:46 +0100
 Stefano Brivio <sbrivio(a)redhat.com> wrote:
   
  On Wed, 14 Dec 2022 12:42:14 +1100
 David Gibson <david(a)gibson.dropbear.id.au> wrote:
   
 > On Tue, Dec 13, 2022 at 11:48:47PM +0100, Stefano Brivio wrote:    
 > > Sorry for the long delay here,
 > > 
 > > On Mon,  5 Dec 2022 19:14:21 +1100
 > > David Gibson <david(a)gibson.dropbear.id.au> wrote:
 > >       
 > > > Usually udp_sock_handler() will receive and forward multiple (up to 32)
 > > > datagrams in udp_sock_handler(), then forward them all to the tap
 > > > interface.  For unclear reasons, though, when in pasta mode we will only
 > > > receive and forward a single datagram at a time.  Change it to receive
 > > > multiple datagrams at once, like the other paths.      
 > > 
 > > This is explained in the commit message of 6c931118643c ("tcp, udp:
 > > Receive batching doesn't pay off when writing single frames to
tap").
 > > 
 > > I think it's worth re-checking the throughput now as this path is a bit
 > > different, but unfortunately I didn't include this in the "perf"
tests :(
 > > because at the time I introduced those I wasn't sure it even made sense
to
 > > have traffic from the same host being directed to the tap device.
 > > 
 > > The iperf3 runs were I observed this are actually the ones from the Podman
 > > demo. Ideally that case should be also checked in the perf/pasta_udp tests.    
 
 > 
 > Hm, ok.
 >     
 > > How fundamental is this for the rest of the series? I couldn't find any
 > > actual dependency on this but I might be missing something.      
 > 
 > So the issue is that prior to this change in pasta we receive multiple
 > frames at once on the splice path, but one frame at a time on the tap
 > path.  By the end of this series we can't do that any more, because we
 > don't know before the recvmmsg() which one we'll be doing.    
 
 Oh, right, I see. Then let me add this path to the perf/pasta_udp test
 and check how relevant this is now, I'll get back to you in a bit.   
 
 I was checking the wrong path. With this:
 
 diff --git a/test/perf/pasta_udp b/test/perf/pasta_udp
 index 27ea724..973c2f4 100644
 --- a/test/perf/pasta_udp
 +++ b/test/perf/pasta_udp
 @@ -31,6 +31,14 @@ report	pasta lo_udp 1 __FREQ__
  
  th	MTU 1500B 4000B 16384B 65535B
  
 +tr	UDP throughput over IPv6: host to ns
 +nsout	IFNAME ip -j link show | jq -rM '.[] | select(.link_type ==
"ether").ifname'
 +nsout	ADDR6 ip -j -6 addr show|jq -rM '.[] | select(.ifname ==
"__IFNAME__").addr_info[] | select(.scope == "global" and .prefixlen
== 64).local'
 +bw	-
 +bw	-
 +bw	-
 +iperf3	BW host ns __ADDR6__ 100${i}2 __THREADS__ __TIME__ __OPTS__ -b 15G
 +bw	__BW__ 7.0 9.0
  
  tr	UDP throughput over IPv6: ns to host
  ns	ip link set dev lo mtu 1500
 diff --git a/test/run b/test/run
 index e07513f..b53182b 100755
 --- a/test/run
 +++ b/test/run
 @@ -67,6 +67,14 @@ run() {
  	test build/clang_tidy
  	teardown build
  
 +	VALGRIND=0
 +	setup passt_in_ns
 +	test passt/ndp
 +	test passt/dhcp
 +	test perf/pasta_udp
 +	test passt_in_ns/shutdown
 +	teardown passt_in_ns
 +
  	setup pasta
  	test pasta/ndp
  	test pasta/dhcp   
 
 Ah, ok.  Can we add that to the standard set of tests ASAP, please.
 
  I get 21.6 gbps after this series, and 29.7 gbps
before -- it's quite
 significant.   
 
 Drat.
 
  And there's nothing strange in perf's
output, really, the distribution
 of overhead per functions is pretty much the same, but writing multiple
 messages to the tap device just takes more cycles per message compared
 to a single message.   
 
 That's so weird.  It should be basically an identical set of write()s,
 except that they happen in a batch, rather than a bit spread out.  I
 guess it has to be some kind of cache locality thing.  I wonder if the
 difference would go away or reverse if we had a way to submit multiple
 frames with a single syscall.
 
  I'm a bit ashamed to propose this, but do
you think about something
 like:   
 
  	if (c->mode == MODE_PASTA) { if
(recvmmsg(ref.r.s, mmh_recv,
 		1, 0, NULL) <= 0) return;   
 
  		if (udp_mmh_splice_port(v6, mmh_recv)) { n =
 			recvmmsg(ref.r.s, mmh_recv + 1, UDP_MAX_FRAMES
 			- 1, 0, NULL); }   
 
  		if (n > 0) n++; else n = 1; } else { n =
 			recvmmsg(ref.r.s, mmh_recv, UDP_MAX_FRAMES, 0,
 			NULL); if (n <= 0) return; }   
 
  ? Other than the inherent ugliness, it looks like
a good
 approximation to me.   
 
 Hmm.  Well, the first question is how much impact does going 1 message
 at a time have on the spliced throughput.  If it's not too bad, then
 we could just always go one at a time for pasta, regardless of
 splicing.  And we could even abstract that difference into the tap
 backend with a callback like tap_batch_size(c). 
 
 So, finally I had the chance to try this out.
 
 First off, baseline with the patch adding the new tests I just sent,
 and the series you posted:
 
 === perf/pasta_udp
  pasta: throughput and latency (local traffic)
 Throughput in Gbps, latency in µs, one thread at 3.6 GHz, 4 streams
                                                          MTU: |  1500B |  4000B | 16384B
| 65535B |
                                                              
|--------|--------|--------|--------|
              UDP throughput over IPv6: ns to host             |    4.4 |    8.5 |   19.5
|   23.0 |
              UDP RR latency over IPv6: ns to host             |      - |      - |      -
|     27 |
                                                              
|--------|--------|--------|--------|
              UDP throughput over IPv4: ns to host             |    4.3 |    8.8 |   18.5
|   24.4 |
              UDP RR latency over IPv4: ns to host             |      - |      - |      -
|     26 |
                                                              
|--------|--------|--------|--------|
              UDP throughput over IPv6: host to ns             |      - |      - |      -
|   22.5 |
              UDP RR latency over IPv6: host to ns             |      - |      - |      -
|     30 |
                                                              
|--------|--------|--------|--------|
              UDP throughput over IPv4: host to ns             |      - |      - |      -
|   24.5 |
              UDP RR latency over IPv4: host to ns             |      - |      - |      -
|     25 |
                                                              
'--------'--------'--------'--------'
 ...passed.
 
  pasta: throughput and latency (traffic via tap)
 Throughput in Gbps, latency in µs, one thread at 3.6 GHz, 4 streams
                                                          MTU: |  1500B |  4000B | 16384B
| 65520B |
                                                              
|--------|--------|--------|--------|
              UDP throughput over IPv6: ns to host             |    4.4 |   10.4 |   16.0
|   23.4 |
              UDP RR latency over IPv6: ns to host             |      - |      - |      -
|     27 |
                                                              
|--------|--------|--------|--------|
              UDP throughput over IPv4: ns to host             |    5.2 |   10.8 |   16.0
|   24.0 |
              UDP RR latency over IPv4: ns to host             |      - |      - |      -
|     28 |
                                                              
|--------|--------|--------|--------|
              UDP throughput over IPv6: host to ns             |      - |      - |      -
|   21.5 |
              UDP RR latency over IPv6: host to ns             |      - |      - |      -
|     29 |
                                                              
|--------|--------|--------|--------|
              UDP throughput over IPv4: host to ns             |      - |      - |      -
|   26.3 |
              UDP RR latency over IPv4: host to ns             |      - |      - |      -
|     26 |
                                                              
'--------'--------'--------'--------'
 
 which seems to indicate the whole "splicing" thing is pretty much
 useless, for UDP (except for that 16 KiB MTU case, but I wonder how
 relevant that is).
 
 If I set UDP_MAX_FRAMES to 1, with a quick workaround for the resulting
 warning in udp_tap_send() (single frame to send, hence single message),
 it gets somewhat weird:
 
 === perf/pasta_udp
  pasta: throughput and latency (local traffic)
 Throughput in Gbps, latency in µs, one thread at 3.6 GHz, 4 streams
                                                          MTU: |  1500B |  4000B | 16384B
| 65535B |
                                                              
|--------|--------|--------|--------|
              UDP throughput over IPv6: ns to host             |    3.4 |    7.0 |   21.6
|   31.6 |
              UDP RR latency over IPv6: ns to host             |      - |      - |      -
|     30 |
                                                              
|--------|--------|--------|--------|
              UDP throughput over IPv4: ns to host             |    3.8 |    7.0 |   22.0
|   32.4 |
              UDP RR latency over IPv4: ns to host             |      - |      - |      -
|     26 |
                                                              
|--------|--------|--------|--------|
              UDP throughput over IPv6: host to ns             |      - |      - |      -
|   29.3 |
              UDP RR latency over IPv6: host to ns             |      - |      - |      -
|     31 |
                                                              
|--------|--------|--------|--------|
              UDP throughput over IPv4: host to ns             |      - |      - |      -
|   33.8 |
              UDP RR latency over IPv4: host to ns             |      - |      - |      -
|     25 |
                                                              
'--------'--------'--------'--------'
 ...passed.
 
  pasta: throughput and latency (traffic via tap)
 Throughput in Gbps, latency in µs, one thread at 3.6 GHz, 4 streams
                                                          MTU: |  1500B |  4000B | 16384B
| 65520B |
                                                              
|--------|--------|--------|--------|
              UDP throughput over IPv6: ns to host             |    4.7 |   10.3 |   16.0
|   24.0 |
              UDP RR latency over IPv6: ns to host             |      - |      - |      -
|     27 |
                                                              
|--------|--------|--------|--------|
              UDP throughput over IPv4: ns to host             |    5.6 |   11.4 |   16.0
|   24.0 |
              UDP RR latency over IPv4: ns to host             |      - |      - |      -
|     26 |
                                                              
|--------|--------|--------|--------|
              UDP throughput over IPv6: host to ns             |      - |      - |      -
|   21.5 |
              UDP RR latency over IPv6: host to ns             |      - |      - |      -
|     29 |
                                                              
|--------|--------|--------|--------|
              UDP throughput over IPv4: host to ns             |      - |      - |      -
|   28.7 |
              UDP RR latency over IPv4: host to ns             |      - |      - |      -
|     29 |
                                                              
'--------'--------'--------'--------'
 
 ...except for the cases with low MTUs, throughput is significantly
 higher if we read and send one message at a time on the "spliced" path.
 
 Next, I would like to:
 
 - bisect between 32 and 1 for UDP_MAX_FRAMES: maybe 32 affects data
   locality too much, but some lower value would still be beneficial by
   lowering syscall overhead 
Ok.

...
  - try with sendmsg() instead of sendmmsg(), at this
point. Looking at
   the kernel, that doesn't seem to make a real difference. 
Which sendmmsg() specifically are you looking at changing?

...
  About this series: should we just go ahead and apply
it with
 UDP_MAX_FRAMES set to 1 for the moment being? It's anyway better than
 the existing situation. 
I think that's a good idea - or rather, not setting UDP_MAX_FRAMES to
1, but clamping the batch size to 1 for pasta - I'm pretty sure we
still want the batching for passt.  We lose a little bit on
small-packet spliced, but we gain on both tap and large-packet
spliced.  This will unblock the dual stack udp stuff and we can
further tune it later.

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

On Mon,  5 Dec 2022 19:14:22 +1100
David Gibson <david(a)gibson.dropbear.id.au> wrote:

...
  udp_splice_namebuf is now used only for spliced
sending, and so it only
 ever populated with the localhost address, either IPv4 or IPv6.  So,
 replace the awkward initialization in udp_sock_handler_splice() with
 statically initialized versions for IPv4 and IPv6.  We then just need to
 update the port number in udp_sock_handler_splice().
 
 Signed-off-by: David Gibson <david(a)gibson.dropbear.id.au>
 ---
  udp.c  | 40 ++++++++++++++++++----------------------
  util.h |  7 +++++++
  2 files changed, 25 insertions(+), 22 deletions(-)
 
 diff --git a/udp.c b/udp.c
 index 24fa984..7c601cc 100644
 --- a/udp.c
 +++ b/udp.c
 @@ -232,11 +232,18 @@ static struct mmsghdr	udp4_l2_mh_tap		[UDP_MAX_FRAMES];
  static struct mmsghdr	udp6_l2_mh_tap		[UDP_MAX_FRAMES];
  
  /* recvmmsg()/sendmmsg() data for "spliced" connections */
 -static struct sockaddr_storage udp_splice_namebuf;
 -
  static struct iovec	udp4_iov_splice		[UDP_MAX_FRAMES];
  static struct iovec	udp6_iov_splice		[UDP_MAX_FRAMES];
  
 +static struct sockaddr_in udp_localname4 = {
 +	.sin_family = AF_INET,
 +	.sin_addr = IN4ADDR_LOOPBACK_INIT,
 +};
 +static struct sockaddr_in6 udp_localname6 = {
 +	.sin6_family = AF_INET6,
 +	.sin6_addr = IN6ADDR_LOOPBACK_INIT,
 +}; 
Nit, not a strong preference and not worth re-spinning just for this: I
think udp4_localname and udp6_localname would be more consistent with
everything else here, hence easier to type without double checking.

-- 
Stefano

On Tue, Dec 13, 2022 at 11:48:58PM +0100, Stefano Brivio wrote:
...
  On Mon,  5 Dec 2022 19:14:23 +1100
 David Gibson <david(a)gibson.dropbear.id.au> wrote:
 
  These two functions now have a very similar
structure, and their first
 part (calling recvmmsg()) is functionally identical.  So, merge the two
 functions into one.
 
 Signed-off-by: David Gibson <david(a)gibson.dropbear.id.au>
 ---
  udp.c | 86 +++++++++++++++++++----------------------------------------
  1 file changed, 28 insertions(+), 58 deletions(-)
 
 diff --git a/udp.c b/udp.c
 index 7c601cc..6ccfe8c 100644
 --- a/udp.c
 +++ b/udp.c
 @@ -590,52 +590,6 @@ static void udp_splice_sendfrom(const struct ctx *c, unsigned start,
unsigned n,
  	sendmmsg(s, mmh_send + start, n, MSG_NOSIGNAL);
  }
  
 -/**
 - * udp_sock_handler_splice() - Handler for socket mapped to "spliced"
connection
 - * @c:		Execution context
 - * @ref:	epoll reference
 - * @events:	epoll events bitmap
 - * @now:	Current timestamp
 - */
 -static void udp_sock_handler_splice(const struct ctx *c, union epoll_ref ref,
 -				    uint32_t events, const struct timespec *now)
 -{
 -	in_port_t dst = ref.r.p.udp.udp.port;
 -	int v6 = ref.r.p.udp.udp.v6, n, i, m;
 -	struct mmsghdr *mmh_recv;
 -
 -	if (!(events & EPOLLIN))
 -		return;
 -
 -	if (v6)
 -		mmh_recv = udp6_l2_mh_sock;
 -	else
 -		mmh_recv = udp4_l2_mh_sock;
 -
 -	n = recvmmsg(ref.r.s, mmh_recv, UDP_MAX_FRAMES, 0, NULL);
 -
 -	if (n <= 0)
 -		return;
 -
 -	if (v6)
 -		udp_localname6.sin6_port = htons(dst);
 -	else
 -		udp_localname4.sin_port = htons(dst);
 -
 -	for (i = 0; i < n; i += m) {
 -		in_port_t src = sa_port(v6, mmh_recv[i].msg_hdr.msg_name);
 -
 -		for (m = 1; i + m < n; m++) {
 -			void *mname = mmh_recv[i + m].msg_hdr.msg_name;
 -			if (sa_port(v6, mname) != src)
 -				break;
 -		}
 -
 -		udp_splice_sendfrom(c, i, m, src, dst, v6, ref.r.p.udp.udp.ns,
 -				    ref.r.p.udp.udp.orig, now);
 -	}
 -}
 -
  /**
   * udp_update_hdr4() - Update headers for one IPv4 datagram
   * @c:		Execution context
 @@ -945,27 +899,43 @@ void udp_sock_handler(const struct ctx *c, union epoll_ref ref,
uint32_t events,
  {
  	in_port_t dstport = ref.r.p.udp.udp.port;
  	bool v6 = ref.r.p.udp.udp.v6;
 -	struct mmsghdr *sock_mmh;
 +	struct mmsghdr *mmh_recv;
 +	unsigned int i, m;
  	ssize_t n;
  
 -	if (events == EPOLLERR)
 +	if (!(events & EPOLLIN)) 
 
 Pre-existing, unrelated issue, but this reminds me: we don't handle
 socket errors here, and while udp_timer_one() will drop any sockets we
 created, eventually, it would probably be better to act right away. 
Ok... I'm not sure what, if anything, you would like me to do about
it, however.

...
  Not that I have in mind a valid example of an error on
UDP sockets,
 except perhaps if the interface goes down (but we'll handle that
 separately). 
-- 
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

On Tue, Dec 13, 2022 at 11:49:18PM +0100, Stefano Brivio wrote:
...
  On Mon,  5 Dec 2022 19:14:24 +1100
 David Gibson <david(a)gibson.dropbear.id.au> wrote:
 
  Currently we have special sockets for receiving
datagrams from locahost
 which can use the optimized "splice" path rather than going across the tap
 interface.
 
 We want to loosen this so that sockets can receive sockets that will be
 forwarded by both the spliced and non-spliced paths.  To do this, we alter
 the meaning of the @splice bit in the reference to mean that packets
 receieved on this socket *can* be spliced, not that they *will* be spliced.
 They'll only actually be spliced if they come from 127.0.0.1 or ::1.
 
 We can't (for now) remove the splice bit entirely, unlike with TCP.  Our
 gateway mapping means that if the ns initiates communication to the gw
 address, we'll translate that to target 127.0.0.1 on the host side.  Reply
 packets will therefore have source address 127.0.0.1 when received on the
 host, but these need to go via the tap path where that will be translated
 back to the gateway address.  We need the @splice bit to distinguish that
 case from packets going from localhost to a port mapped explicitly with
 -u which should be spliced.
 
 Signed-off-by: David Gibson <david(a)gibson.dropbear.id.au>
 ---
  udp.c | 54 +++++++++++++++++++++++++++++++++++-------------------
  udp.h |  2 +-
  2 files changed, 36 insertions(+), 20 deletions(-)
 
 diff --git a/udp.c b/udp.c
 index 6ccfe8c..011a157 100644
 --- a/udp.c
 +++ b/udp.c
 @@ -513,16 +513,27 @@ static int udp_splice_new_ns(void *arg)
  }
  
  /**
 - * sa_port() - Determine port from a sockaddr_in or sockaddr_in6
 + * udp_mmh_splice_port() - Is source address of message suitable for splicing?
   * @v6:		Is @sa a sockaddr_in6 (otherwise sockaddr_in)?
 - * @sa:		Pointer to either sockaddr_in or sockaddr_in6
 + * @mmh:	mmsghdr of incoming message
 + *
 + * Return: if @sa refers to localhost (127.0.0.1 or ::1) the port from
 + *         @sa, otherwise 0.
 + *
 + * NOTE: this relies on the fact that it's not valid to use UDP port 0 
 
 The port is reserved by IANA indeed, but... it can actually be used. On
 Linux, you can bind() it and you can connect() to it. As far as I can
 tell from the new version of udp_sock_handler() we would actually
 misdirect packets in that case. 
Hm, ok.  Given the IANA reservation, I think it would be acceptable to
simply drop such packets - but if we were to make that choice we
should do so explicitly, rather than misdirecting them.

...
  How bad would it be to use an int here? 
Pretty straightforward.  Just means we have to use the somewhat
abtruse "if (port <= USHRT_MAX)" or "if (port >= 0)" or
something
instead of just "if (port)".  Should I go ahead and make that change?

...
  By the way, I think the comment should also mention
that the port is
 returned in host order. 
Ok, easily done.  Generally I try to keep the endianness associated
with the type, rather than attempting to document it for each variable
(or even worse, each point in time for each variable).

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

On Wed, Dec 14, 2022 at 11:35:54AM +0100, Stefano Brivio wrote:
...
  On Wed, 14 Dec 2022 12:47:25 +1100
 David Gibson <david(a)gibson.dropbear.id.au> wrote:
 
  On Tue, Dec 13, 2022 at 11:49:18PM +0100, Stefano
Brivio wrote:
  On Mon,  5 Dec 2022 19:14:24 +1100
 David Gibson <david(a)gibson.dropbear.id.au> wrote:
   
  Currently we have special sockets for receiving
datagrams from locahost
 which can use the optimized "splice" path rather than going across the tap
 interface.
 
 We want to loosen this so that sockets can receive sockets that will be
 forwarded by both the spliced and non-spliced paths.  To do this, we alter
 the meaning of the @splice bit in the reference to mean that packets
 receieved on this socket *can* be spliced, not that they *will* be spliced.
 They'll only actually be spliced if they come from 127.0.0.1 or ::1.
 
 We can't (for now) remove the splice bit entirely, unlike with TCP.  Our
 gateway mapping means that if the ns initiates communication to the gw
 address, we'll translate that to target 127.0.0.1 on the host side.  Reply
 packets will therefore have source address 127.0.0.1 when received on the
 host, but these need to go via the tap path where that will be translated
 back to the gateway address.  We need the @splice bit to distinguish that
 case from packets going from localhost to a port mapped explicitly with
 -u which should be spliced.
 
 Signed-off-by: David Gibson <david(a)gibson.dropbear.id.au>
 ---
  udp.c | 54 +++++++++++++++++++++++++++++++++++-------------------
  udp.h |  2 +-
  2 files changed, 36 insertions(+), 20 deletions(-)
 
 diff --git a/udp.c b/udp.c
 index 6ccfe8c..011a157 100644
 --- a/udp.c
 +++ b/udp.c
 @@ -513,16 +513,27 @@ static int udp_splice_new_ns(void *arg)
  }
  
  /**
 - * sa_port() - Determine port from a sockaddr_in or sockaddr_in6
 + * udp_mmh_splice_port() - Is source address of message suitable for splicing?
   * @v6:		Is @sa a sockaddr_in6 (otherwise sockaddr_in)?
 - * @sa:		Pointer to either sockaddr_in or sockaddr_in6
 + * @mmh:	mmsghdr of incoming message
 + *
 + * Return: if @sa refers to localhost (127.0.0.1 or ::1) the port from
 + *         @sa, otherwise 0.
 + *
 + * NOTE: this relies on the fact that it's not valid to use UDP port 0   
 
 The port is reserved by IANA indeed, but... it can actually be used. On
 Linux, you can bind() it and you can connect() to it. As far as I can
 tell from the new version of udp_sock_handler() we would actually
 misdirect packets in that case.   
 
 Hm, ok.  Given the IANA reservation, I think it would be acceptable to
 simply drop such packets - but if we were to make that choice we
 should do so explicitly, rather than misdirecting them. 
 
 Acceptable, sure, but... I don't know, it somehow doesn't look
 desirable to me. The kernel doesn't enforce this, so I guess we
 shouldn't either.
 
 
  How bad
would it be to use an int here?   
 
 Pretty straightforward.  Just means we have to use the somewhat
 abtruse "if (port <= USHRT_MAX)" or "if (port >= 0)" or
something
 instead of just "if (port)".  Should I go ahead and make that change? 
 
 Eh, I don't like it either, but... I guess it's better than the
 alternative, so yes, thanks. Or pass port as a pointer, set on return.
 I'm fine with both. 
I think the int is less ugly than the pointer.  Ok, I'll make that change.

...
 
 
  By the way, I think the comment should also mention
that the port is
 returned in host order.   
 
 Ok, easily done.  Generally I try to keep the endianness associated
 with the type, rather than attempting to document it for each variable
 (or even worse, each point in time for each variable). 
 
 Yes, I see, and it's a more valid approach than mine, but still mine
 comes almost for free. 
Right.  Actually rereading the function in question, it specifically
says "port from @sa" and in the sockaddr, of course, it's network
endian, so it could particularly do with clarification in this case.

...
  By the way, I got distracted by this and I forgot
about two things:
 
  +static in_port_t udp_mmh_splice_port(bool v6,
const struct mmsghdr *mmh)
  {
 -	const struct sockaddr_in6 *sa6 = sa;
 -	const struct sockaddr_in *sa4 = sa;
 +	const struct sockaddr_in6 *sa6 = mmh->msg_hdr.msg_name;
 +	const struct sockaddr_in *sa4 = mmh->msg_hdr.msg_name;; 
 
 Stray semicolon here. 
Fixed.

...
 
  +
 +	if (v6 && IN6_IS_ADDR_LOOPBACK(&sa6->sin6_addr))
 +		return ntohs(sa6->sin6_port);
  
 -	return v6 ? ntohs(sa6->sin6_port) : ntohs(sa4->sin_port);
 +	if (ntohl(sa4->sin_addr.s_addr) == INADDR_LOOPBACK)
 +		return ntohs(sa4->sin_port); 
 
 If it's IPv6, but not a loopback address, we'll check if
 sa4->sin_addr.s_addr == INADDR_LOOPBACK -- which might actually be true for
 an IPv6, non-loopback address. 
Oops, yes, that's definitely wrong, and wrong enough to respin.

...
  Also, I think we can happily "splice" for
any loopback address, not
 just 127.0.0.1. What about something like: 
Well.. yes and no.  Yes, we can deliver packets in this way, but we'll
lost track of the original from address, so reply packets will be
misdirected.

Hrm.. ISTR you mentioned some cases that worked despite that, so I'll
make the change, and hope to fix it up better when I get to the NAT /
splice semantics rework.

...
  	if (v6 &&
IN6_IS_ADDR_LOOPBACK(&sa6->sin6_addr))
 		return ntohs(sa6->sin6_port);
 
 	if (!v4 && IN4_IS_ADDR_LOOPBACK(&sa4->sin_addr))
 		return ntohs(sa4->sin_port);
 
 	return -1;
 
 ?
  
-- 
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