fix(vsock): restore listener after accept

[archevel]

After accepting a connection, move the accepted socket to an ephemeral port and reset the listener entry to Listen state. Track the listen port separately in Socket so subsequent accept calls always find the listener entry regardless of how many connections have been accepted.

Fixes #2433

[archevel]

As a reference I made this pull request as well with a regression test:
hermit-os/hermit-rs#996

[github-actions]

Benchmark Results

Details

Benchmark	Current: f0f88da	Previous: b3883fa	Performance Ratio
startup_benchmark Build Time	82.78 s	80.39 s	1.03 ❗
startup_benchmark File Size	0.79 MB	0.79 MB	1.00 ❗
Startup Time - 1 core	0.76 s (±0.02 s)	0.74 s (±0.01 s)	1.02
Startup Time - 2 cores	0.74 s (±0.02 s)	0.75 s (±0.02 s)	0.98
Startup Time - 4 cores	0.77 s (±0.02 s)	0.76 s (±0.02 s)	1.01
multithreaded_benchmark Build Time	78.25 s	83.71 s	0.93 ❗
multithreaded_benchmark File Size	0.90 MB	0.90 MB	1.00 ❗
Multithreaded Pi Efficiency - 2 Threads	89.54 % (±5.21 %)	87.67 % (±5.37 %)	1.02
Multithreaded Pi Efficiency - 4 Threads	43.68 % (±2.63 %)	43.34 % (±2.79 %)	1.01
Multithreaded Pi Efficiency - 8 Threads	25.72 % (±1.03 %)	25.32 % (±1.83 %)	1.02
micro_benchmarks Build Time	76.86 s	80.38 s	0.96 ❗
micro_benchmarks File Size	0.90 MB	0.90 MB	1.00 ❗
Scheduling time - 1 thread	64.04 ticks (±2.29 ticks)	68.25 ticks (±5.81 ticks)	0.94
Scheduling time - 2 threads	36.27 ticks (±4.55 ticks)	38.15 ticks (±5.34 ticks)	0.95
Micro - Time for syscall (getpid)	4.26 ticks (±0.74 ticks)	4.13 ticks (±0.69 ticks)	1.03
Memcpy speed - (built_in) block size 4096	86113.96 MByte/s (±59482.26 MByte/s)	82192.20 MByte/s (±56877.97 MByte/s)	1.05
Memcpy speed - (built_in) block size 1048576	30585.99 MByte/s (±24644.56 MByte/s)	30409.95 MByte/s (±24564.57 MByte/s)	1.01
Memcpy speed - (built_in) block size 16777216	28543.92 MByte/s (±23457.96 MByte/s)	26462.68 MByte/s (±21974.68 MByte/s)	1.08
Memset speed - (built_in) block size 4096	86981.82 MByte/s (±60012.98 MByte/s)	82271.23 MByte/s (±56937.12 MByte/s)	1.06
Memset speed - (built_in) block size 1048576	31347.54 MByte/s (±25092.76 MByte/s)	31147.37 MByte/s (±25005.00 MByte/s)	1.01
Memset speed - (built_in) block size 16777216	29312.36 MByte/s (±23916.10 MByte/s)	27241.00 MByte/s (±22475.91 MByte/s)	1.08
Memcpy speed - (rust) block size 4096	75182.17 MByte/s (±52532.99 MByte/s)	74974.37 MByte/s (±52256.45 MByte/s)	1.00
Memcpy speed - (rust) block size 1048576	30511.43 MByte/s (±24672.13 MByte/s)	30241.00 MByte/s (±24501.06 MByte/s)	1.01
Memcpy speed - (rust) block size 16777216	28417.05 MByte/s (±23347.08 MByte/s)	27430.85 MByte/s (±22631.84 MByte/s)	1.04
Memset speed - (rust) block size 4096	75828.33 MByte/s (±52962.43 MByte/s)	75374.74 MByte/s (±52547.87 MByte/s)	1.01
Memset speed - (rust) block size 1048576	31278.33 MByte/s (±25121.45 MByte/s)	30996.28 MByte/s (±24944.22 MByte/s)	1.01
Memset speed - (rust) block size 16777216	29196.00 MByte/s (±23817.29 MByte/s)	28210.43 MByte/s (±23118.03 MByte/s)	1.03
alloc_benchmarks Build Time	79.81 s	76.93 s	1.04 ❗
alloc_benchmarks File Size	0.87 MB	0.87 MB	1.00 ❗
Allocations - Allocation success	91.31 %	91.31 %	1
Allocations - Deallocation success	100.00 %	100.00 %	1
Allocations - Pre-fail Allocations	61.44 %	61.44 %	1
Allocations - Average Allocation time	5039.45 Ticks (±138.48 Ticks)	5120.57 Ticks (±121.11 Ticks)	0.98
Allocations - Average Allocation time (no fail)	5761.73 Ticks (±117.30 Ticks)	5812.28 Ticks (±133.21 Ticks)	0.99
Allocations - Average Deallocation time	1219.92 Ticks (±202.94 Ticks)	1001.83 Ticks (±184.72 Ticks)	1.22
mutex_benchmark Build Time	76.27 s	77.85 s	0.98 ❗
mutex_benchmark File Size	0.90 MB	0.90 MB	1.00 ❗
Mutex Stress Test Average Time per Iteration - 1 Threads	12.12 ns (±0.32 ns)	12.44 ns (±0.61 ns)	0.97
Mutex Stress Test Average Time per Iteration - 2 Threads	42.44 ns (±2.38 ns)	44.68 ns (±3.31 ns)	0.95

This comment was automatically generated by workflow using github-action-benchmark.

[mkroening]

Thanks for finding and fixing this bug! :)

Sorry for the late reply. I can reproduce the fix.

Regarding the implementation:
If I understand correctly, your changes now track two ports for listening sockets, one for the listener and one for the last connection, right? This does not support multiple open connections at the same time, right?

Perhaps doing something similar to TCP would be more useful: return a proper connection FD from accept instead of the current dummy NullSocket.

Regarding the test:
Could you adapt the existing vsock tests instead?

[archevel]

No worries! I appreciate you taking the time to have look. I've kept working on this locally and had to do some additional changes. Those are out of scope for this I think, as they involved needing to patch libc and mio to handle epoll for hermit. I'm working on that in order to be able to use firecracker to run a hermit based vm and communicate over vsock. Anyway, that is a larger piece of work, but I think I actually ended up aligning this more with how the TCP is handled. I'll take a peek and adjust this PR.

[archevel]

I've rebased the changes on main. I then changed to align it more with how the tcp socket works (handling multiple concurrent connections). Let me know if there are any other issues I should address/fix! The CI failed, so I'll take a peek and see if I understand why.

[archevel]

I think the CI fails because it isn't aligned with the changes in hermit-os/hermit-rs#996

[archevel]

@mkroening I realized that the code doesn't really handle multiple cids currently. E.g. If the host assigns two vsock cids, say 3 and 4. Currently listening on either cid on port 1234 will get connections for both I think. Might be a separate issue to solve.

Edit: I realized this wasn't a real issue since qemu can only assign a single cid to a guest with vhost-vsock. However, the code did not fail on binding on a non-assigned cid. I've now added so that a bind on e.g. cid 4 when cid 3 was assigned fails. Binding on VMADDR_CID_ANY works. Arguably there might be a case for allowing binding on CID 1 (loopback), but since hermit apps as far as I understand are essentially single processes there ought to be better ways to handle internal messages....

[archevel]

@mkroening I'm not entierly familiar with the setup, but this PR won't pass the CI unless hermit-os/hermit-rs#996 is merged. However, merging that will likely cause OTHER PRs to start failing before merging this PR... Is there anything I should amend to make this smoother?

[archevel]

I ended up using claude to aid me with the accept backlog and try to align it with the tcp code. Not sure if that is acceptable? In the process I discovered that mio doesn't handle select in the same way for the native socket types (e.g. TcpStream wrapped in IoSource) as it does for a raw file descriptor in a mio::unix::SourceFd. This causes an issue where the reader is never woken up even when data is arriving. I made a workaround in the app I am working on. Should I raise it as an issue against the kernel? Not sure exactly where the fix should go (might be in mio).

[mkroening]

needing to patch libc and mio to handle epoll for hermit

We do have patched versions of socket2 and tokio to make them work on Hermit (hermit-rs/Cargo.toml#L35-L37]. We do have some mio patches upstream already, though I need to rework our system call story in the winter.

Arguably there might be a case for allowing binding on CID 1 (loopback), but since hermit apps as far as I understand are essentially single processes there ought to be better ways to handle internal messages....

Yeah, I think we can postpone that for now. :)

I ended up using claude to aid me with the accept backlog and try to align it with the tcp code. Not sure if that is acceptable?

As long as you can stand behind the code as if you wrote it, it's fine. I need to understand it as well, after all. Please remove the Claude authorship from the commit (I can also do this before merging, if you prefer).

In the process I discovered that mio doesn't handle select in the same way for the native socket types (e.g. TcpStream wrapped in IoSource) as it does for a raw file descriptor in a mio::unix::SourceFd. This causes an issue where the reader is never woken up even when data is arriving. I made a workaround in the app I am working on. Should I raise it as an issue against the kernel? Not sure exactly where the fix should go (might be in mio).

Yes, please go ahead and open an issue.

I will do another review today. Sorry for the delay again! Could you rebase on main instead of merging? Otherwise, the PR shows many unrelated commits from main in this PR's merge-base diff. Meanwhile, I will take a look at the direct diff (badd4c4..7ac357f).

[archevel]

Sorry for the delay again!

Absolutely, no worries! I have this working my machine and that's good enough for me :)

Could you rebase on main instead of merging?

Done! I rebased on main and squashed the minor fixup commits into their parents.

Yes, please go ahead and open an issue.

Raised it here: #2533
It is not a blocker for me since I used the workaround.

[archevel]

I think this is worth calling out. I added this to gate the test code on if the example code in hermit-rs repo is updated. This could be removed once that's merged. Perhaps also removing the default example.