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.

Depends on hermit-os/hermit-rs#996.

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: 2ba4de9	Previous: 2e23902	Performance Ratio
startup_benchmark Build Time	97.56 s	80.34 s	1.21 ❗
startup_benchmark File Size	0.79 MB	0.80 MB	1.00 ❗
Startup Time - 1 core	0.86 s (±0.02 s)	0.75 s (±0.02 s)	1.15 ❗
Startup Time - 2 cores	0.88 s (±0.02 s)	0.74 s (±0.02 s)	1.20 ❗
Startup Time - 4 cores	0.89 s (±0.01 s)	0.74 s (±0.02 s)	1.20 ❗
multithreaded_benchmark Build Time	95.91 s	82.11 s	1.17 ❗
multithreaded_benchmark File Size	0.90 MB	0.86 MB	1.05 ❗
Multithreaded Pi Efficiency - 2 Threads	91.28 % (±5.05 %)	85.89 % (±6.61 %)	1.06
Multithreaded Pi Efficiency - 4 Threads	44.98 % (±2.30 %)	43.43 % (±2.56 %)	1.04
Multithreaded Pi Efficiency - 8 Threads	25.71 % (±1.14 %)	25.76 % (±1.53 %)	1.00
micro_benchmarks Build Time	101.94 s	80.40 s	1.27 ❗
micro_benchmarks File Size	0.90 MB	0.86 MB	1.05 ❗
Scheduling time - 1 thread	79.96 ticks (±1.90 ticks)	62.65 ticks (±4.06 ticks)	1.28 ❗
Scheduling time - 2 threads	42.98 ticks (±2.77 ticks)	34.08 ticks (±4.10 ticks)	1.26 ❗
Micro - Time for syscall (getpid)	4.31 ticks (±0.56 ticks)	3.45 ticks (±0.58 ticks)	1.25 ❗
Memcpy speed - (built_in) block size 4096	69102.81 MByte/s (±47657.38 MByte/s)	82448.38 MByte/s (±56997.13 MByte/s)	0.84
Memcpy speed - (built_in) block size 1048576	29699.41 MByte/s (±24756.62 MByte/s)	30585.98 MByte/s (±24707.84 MByte/s)	0.97
Memcpy speed - (built_in) block size 16777216	11677.52 MByte/s (±8235.62 MByte/s)	26340.06 MByte/s (±21720.96 MByte/s)	0.44
Memset speed - (built_in) block size 4096	69171.14 MByte/s (±47704.62 MByte/s)	82292.76 MByte/s (±56891.50 MByte/s)	0.84
Memset speed - (built_in) block size 1048576	30458.57 MByte/s (±25164.65 MByte/s)	31323.85 MByte/s (±25145.86 MByte/s)	0.97
Memset speed - (built_in) block size 16777216	11770.58 MByte/s (±8296.41 MByte/s)	27104.68 MByte/s (±22209.94 MByte/s)	0.43
Memcpy speed - (rust) block size 4096	58149.00 MByte/s (±40720.44 MByte/s)	74097.96 MByte/s (±51811.44 MByte/s)	0.78
Memcpy speed - (rust) block size 1048576	29702.14 MByte/s (±24713.31 MByte/s)	30361.60 MByte/s (±24602.37 MByte/s)	0.98
Memcpy speed - (rust) block size 16777216	11623.16 MByte/s (±8195.21 MByte/s)	27625.34 MByte/s (±22806.88 MByte/s)	0.42
Memset speed - (rust) block size 4096	58627.75 MByte/s (±41017.84 MByte/s)	74373.47 MByte/s (±51976.48 MByte/s)	0.79
Memset speed - (rust) block size 1048576	30453.62 MByte/s (±25121.81 MByte/s)	31110.89 MByte/s (±25033.24 MByte/s)	0.98
Memset speed - (rust) block size 16777216	11732.05 MByte/s (±8269.16 MByte/s)	28386.93 MByte/s (±23265.03 MByte/s)	0.41
alloc_benchmarks Build Time	99.45 s	74.76 s	1.33 ❗
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	5062.48 Ticks (±167.70 Ticks)	5860.58 Ticks (±98.43 Ticks)	0.86 ❗
Allocations - Average Allocation time (no fail)	5784.40 Ticks (±173.56 Ticks)	6554.81 Ticks (±92.86 Ticks)	0.88 ❗
Allocations - Average Deallocation time	1255.34 Ticks (±415.31 Ticks)	1805.01 Ticks (±250.35 Ticks)	0.70 ❗
mutex_benchmark Build Time	99.12 s	79.82 s	1.24 ❗
mutex_benchmark File Size	0.90 MB	0.86 MB	1.05 ❗
Mutex Stress Test Average Time per Iteration - 1 Threads	15.88 ns (±1.92 ns)	12.10 ns (±0.41 ns)	1.31 ❗
Mutex Stress Test Average Time per Iteration - 2 Threads	59.28 ns (±4.74 ns)	40.26 ns (±1.68 ns)	1.47 ❗

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.

[mkroening]

Thanks, this is looking great! Thanks again for the patience. I only have one more nit regarding the code, then I will merge it.

Regarding the CI: I would merge hermit-os/hermit-rs#996 before merging this, so we don't need any conditionals in CI.

[archevel]

Thanks, this is looking great! Thanks again for the patience. I only have one more nit regarding the code, then I will merge it.

Regarding the CI: I would merge hermit-os/hermit-rs#996 before merging this, so we don't need any conditionals in CI.

Awesome! I rebased the changes and changed to a struct instead of the tuple for the ConnKey. As mentioned this PR will fail CI until hermit-os/hermit-rs#996 get's merged.

[mkroening]

I'll take a last look tomorrow morning. :)

Meanwhile, would you mind telling me which coding assistant you used, so we can properly attribute it in the commit messages? I have done some rebasing locally and can add the attribution.

[archevel]

I have used a mix of models from Anthropic. The model I most often use is Opus 4.7 (now 5, but started on 4.7 I believe). I did use Fable for aligning it with the TCP socket code.