feat(s2n-quic-platform): wire up tokio sockets to ring #1790
Conversation
camshaft
force-pushed
the
camshaft/tokio-socket-task
branch
from
b61c15a to
291fd06
Compare
| events: &mut tx::Events, | ||
| ) -> io::Result<()> { | ||
| let mut index = 0; | ||
| while let Some(entry) = entries.get_mut(index) { |
There was a problem hiding this comment.
My sense is that this can be a regular for loop - we exit early, but the indices here aren't ever stored back anywhere etc.
There was a problem hiding this comment.
yep. i think that was left over from something else and i forgot to refactor it back to for
| } | ||
| Poll::Pending => { | ||
| events.blocked(); | ||
| break; |
There was a problem hiding this comment.
This specific code makes me wonder if an interface like events.finished(count: usize, res: Poll<Result<T, E>>) would reduce duplication and make things clearer in terms of why we increment self.pending in on_error, for example.
| } | ||
| } | ||
| Poll::Ready(Err(err)) => { | ||
| if events.on_error(err).is_break() { |
There was a problem hiding this comment.
So if I'm reading on_error right and it's not updated later in this PR somewhere, if we keep hitting errors we're going to spin here rather than exiting? Not entirely sure what kind of error that could be...
There was a problem hiding this comment.
Yeah so the general case for TX errors is discarding the packet and moving on to the next one. The only reason we'd break is if we got a WouldBlock or Interrupted. Outside of these, the socket could return errors for various reasons like the address not being reachable or MTU being too large (for platforms that don't have a way to disable kernel-level MTU probing). In these cases, the packet transmission will always fail so we just discard it and let the connection assume it was lost.
There was a problem hiding this comment.
OK, I think the latest version (with the for loop) fixes the issue I was worried about, or at least does within this function, where we'd get an Err, but not increment the pointer into entries and so infinitely spin in this loop trying to send (and failing).
| } | ||
| Interrupted => ControlFlow::Break(()), | ||
| #[cfg(s2n_quic_platform_gso)] | ||
| _ if errno::errno().0 == libc::EIO => { |
There was a problem hiding this comment.
This feels wrong - shouldn't this be error.raw_os_error()? errno here could be anything if we ran some other code between the relevant syscall and here (e.g., for logging or w/e).
There was a problem hiding this comment.
I... somehow completely missed that API. Yes we should use that instead.
| pub cmsg: Aligned<[u8; cmsg::MAX_LEN]>, | ||
| } | ||
|
|
||
| #[repr(C, align(8))] |
There was a problem hiding this comment.
Similarly, how is alignment important? Why 8? I think this deserves a comment.
There was a problem hiding this comment.
I can comment but the cmsg needs to be aligned to the same as cmsghdr. And I wasn't sure how to make the alignment derived from that so i just made it 8 everywhere :smile. The other fields probably don't need the alignment specification so I can remove those if needed.
There was a problem hiding this comment.
That makes sense.
| unsafe fn update(&mut self, entry: &mut msghdr, payload: &UnsafeCell<u8>, payload_len: u32) { | ||
| let iovec = self.iovec.0.get_mut(); | ||
|
|
||
| iovec.iov_base = payload.get() as *mut _; |
There was a problem hiding this comment.
I would pass/keep payload as *mut u8 rather than &UnsafeCell<u8> - I think as-is this is likely UB under stacked borrows (a proposed memory model), at least when we end up using the iov_base to do useful things.
| let storage = T::alloc(entries, payload_len, DATA_OFFSET); | ||
|
|
||
| let storage = Arc::new(storage); | ||
| let ptr = NonNull::new(storage.as_ref()[0].get()).unwrap(); |
There was a problem hiding this comment.
Similarly to above: you probably want as_ptr on the full array rather than get() on the first element.
| } | ||
|
|
||
| let slice = core::slice::from_raw_parts_mut(ptr.as_ptr() as *mut UnsafeCell<u8>, layout.size()); | ||
| Box::from_raw(slice).into() |
There was a problem hiding this comment.
I suspect it may not matter in practice, but I think this may technically be UB -- the expectation is that you use the same Layout when alloc'ing and dealloc'ing, and here Box will end up dealloc'ing with a different layout (see safety on https://doc.rust-lang.org/nightly/std/alloc/trait.GlobalAlloc.html#tymethod.dealloc).
The fix I'd probably go for is to implement your own Box that keeps a NonNull and stores the layout directly within itself.
There was a problem hiding this comment.
Yeah that's a good catch
|
|
||
| /// Returns the empty messages for the producer | ||
| #[inline] | ||
| pub fn data(&mut self) -> &mut [T] { |
There was a problem hiding this comment.
I guess we already asserted that T here is initialized? Technically this is more of &mut [MaybeUninit<T>] probably in terms of how the caller should treat it -- I wonder if that adds enough verbosity that it's not worth making that the return type. It would help catch typos where we start trying to read from the array while being layout-equivalent.
There was a problem hiding this comment.
By the time you're calling data, T will be initialized, yes. It's kind of a weird one cause we have initialized the message with valid pointers and lengths, but the actual payload data is garbage.
camshaft
force-pushed
the
camshaft/tokio-socket-task
branch
from
291fd06 to
c3cacf5
Compare
camshaft
force-pushed
the
camshaft/tokio-socket-task
branch
from
c3cacf5 to
da711aa
Compare
Description of changes:
This change implements the new socket traits for tokio sockets.
Testing:
Tests are finally added in #1792.
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.