Enhance socket read to handle multiple packets

[KershawChang]

Previously, the we only read one packet from the socket at a time. This patch allows to read more than one packet from the socket.

[codecov-commenter]

Codecov Report

Attention: 106 lines in your changes are missing coverage. Please review.

Comparison is base (d1639a4) 87.61% compared to head (14ab5df) 86.57%.
Report is 4 commits behind head on main.

Files	Patch %	Lines
neqo-client/src/main.rs	0.00%	89 Missing ⚠️
neqo-transport/src/connection/mod.rs	10.00%	9 Missing ⚠️
neqo-http3/src/connection_client.rs	0.00%	8 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1530      +/-   ##
==========================================
- Coverage   87.61%   86.57%   -1.04%     
==========================================
  Files         117      117              
  Lines       37848    38120     +272     
==========================================
- Hits        33160    33002     -158     
- Misses       4688     5118     +430     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[larseggert]

This still passes the packets one-by-one to neqo, so effectively it's doing exactly what we did before. I think what we want (also for GRO/GSO) it to be able to pass a Vec<Datagram> into neqo.

[KershawChang]

I've tried to expose a new interface: pub fn process_multiple_input(&mut self, dgrams: Vec<Datagram>, now: Instant) to allow pass Vec<Datagram> to neqo, but I am not sure if this is enough.

[larseggert]

Actually, I think we want an array here and not a Vec, because IIRC there is some way to run the AEAD over multiple consecutive packets in a memory buffer rather than one-by-one. We'd need to pick a sane maximum array length, probably based on whatever the GSO/GRO limit is at the moment.

We'll also need to make sure that neqo consumes/processes all the inbound frames in an array of datagrams before deciding when and what to transmit.

CC @martinthomson

[martinthomson]

I'm not sure what @larseggert specifically meant when he said "array" here, but Rust convention would have this interface as:

Suggested change

	pub fn process_multiple_input(&mut self, dgrams: Vec<Datagram>, now: Instant) {
	pub fn process_multiple_input(&mut self, dgrams: &[Datagram], now: Instant) {

I would also inline the loop in multiple_input. It's a trivial function that isn't used elsewhere.

Finally, please consider what you will do when dgrams.is_empty(). This function could do nothing in that case.

[KershawChang]

If I change it to &[Datagram], how can I avoid cloning each datagram?

[martinthomson]

Good point. It's a pretty big change, but I think that you could change all the input/process functions to take a datagram reference instead. Maybe impl AsRef<Datagram> would make the whole thing less of a challenge.

For now, this is fine.

[KershawChang]

I'd like to avoid big change for now. Please let me know if you think it's better to change all input/process functions to take a reference. If so, I'll create another issue.
Thanks.

[larseggert]

If we want to support GSO/GRO and avoid memory copies, we need to pass a consecutive region of memory in and out of the packet I/O functions: https://blog.cloudflare.com/accelerating-udp-packet-transmission-for-quic

[martinthomson]

Yeah, that is possible here. The GSO calls happen outside this code and the memory can be cut up and passed in to the stack. That is easier if we move to accepting slices of bytes, but that is more disruptive and we can probably manage with Vec::split_off at first.

[larseggert]

I'm not sure I follow. On GSO (i.e., TX), the region passed down into the kernel needs to be consecutive. We can obviously copy memory to make it so, but it would be more efficient if that was not needed (or done as a side effect of where we need to copy memory anyway, i.e., when doing TLS encrypt).

On GRO (i.e., RX) we get a consecutive memory region from the kernel, which we could exploit to get some TLS decrypt efficiency but don't need to.

[martinthomson]

This change was about reading. If we get a contiguous range of memory, Rust might view that as a single x: &[u8]. It is trivial then for us to split that up into multiple slices x[..1280], x[1280..2560], ..., each of which are wrapped in a fresh Datagram instance. We probably couldn't use Vec for that, but we could if it was passed as owned memory, like Vec<u8>.

I agree that sending is going to be harder. My original design had the memory provided by the caller, but we decided to keep things this way very early on. That is still something we can probably manage with a few tweaks to our API, probably just by having the encryption in place, but we could also do in place packet building and save an allocation with only a tiny bit more effort. We can keep the single-shot nature of the API in that case though, with the caller passing in mutable slices as needed.