feat(s2n-quic-dc): implement queue allocator/dispatcher #2517
Conversation
| /// # Safety | ||
| /// | ||
| /// * The descriptor must be in an owned state. | ||
| /// * After calling this method, the descriptor handle should not be used |
There was a problem hiding this comment.
Presumably this method gets called twice (once for Control and once for Stream), so this seems inaccurate?
There was a problem hiding this comment.
It's more referring to the handle used to call the method - wasn't sure how to phrase that...
There was a problem hiding this comment.
I see. Maybe something like "don't use self after calling this method"? Or you could steal from Arc's wording:
This method can be used to release the final Arc and backing storage, but should not be called after the final Arc has been released.
->
This method can be used to drop the Descriptor, but shouldn't be called after the last Descriptor is released. That implies only calling it once on a given Descriptor handle obtained from into_owned.
| id: VarInt, | ||
| references: CachePadded<AtomicUsize>, | ||
| stream: CachePadded<mpsc::Receiver<T>>, | ||
| control: CachePadded<mpsc::Receiver<T>>, |
There was a problem hiding this comment.
Padding these doesn't feel right to me. A receiver is Arc<Channel<T>>, so we're really only padding the pointer, which is immutable... and that seems useless. Padding the references feels more reasonable, though it's still a bit wasteful (64 or 128 bytes -- some of that probably goes to the other fields here but not that much :).
There was a problem hiding this comment.
fair enough. would you recommend i remove all padding?
There was a problem hiding this comment.
Speaking to this more: another path would be to remove the Arc indirection and put the mutex in line. Thoughts?
There was a problem hiding this comment.
Oh, I just realized that mpsc::Receiver is actually two Arcs away (plus a VecDeque) from the underlying data (Arc, and Channel is RingDeque which is also Arc<Mutex>).
I think my sense is that we aren't going to benefit from padding here as long as we still take a mutex in order to send/receive from the channel (that's going to contend on the mutex's lock cache line at minimum), so I would just drop the padding for now I think. I suspect it might be a good idea to boil away the indirection and just have a Mutex<(Waker, VecDeque<T>)> for each of stream + control here, but I don't have a strong opinion there - what we currently have does seem a bit excessively indirect though :)
There was a problem hiding this comment.
Yeah I've got a change locally that removes these Arcs entirely; seems like a waste to have this custom allocator but have 2 pointer jumps just to get at the data
| unsafe { | ||
| let descriptor = ptr.as_ptr().add(offset).cast::<DescriptorInner<T>>(); | ||
| // initialize the descriptor - note that it is self-referential to `addr`, `data`, and `free` | ||
| // SAFETY: address, payload, and memory are all initialized |
There was a problem hiding this comment.
ah shoot. will fix 😄
| #[inline] | ||
| pub fn sender(&self) -> Sender<T> { | ||
| Sender { | ||
| channel: self.channel.clone_for_sender(), |
There was a problem hiding this comment.
This seems a bit odd -- we close the channel when the sender count drops to zero, but this would allow "reopening" the sender side. Maybe needs an assert!(...) that the sender count isn't zero?
There was a problem hiding this comment.
Yes it's a bit goofy; it's mostly the way it is to work around the current impl where we create the sender after initializing the descriptor, which would start out with 0 senders:
let descriptor = Descriptor::new(descriptor);
let sender = Sender::new(descriptor.clone_for_sender());Like you said, it's an internal implementation detail so I can revisit later.
Description of changes:
With the changes in #2507, we now use the
queue_idfield to route packets to the correct queues, which will be read by the target stream. This means we need a way to allocatequeue_ids from a pool and free them once they're no longer used for another stream to pick up.This change implements exactly that - a queue allocator.
Additionally, it includes a packet dispatcher implementation that looks up the queue by its ID and sends the packet if there is an active owner.
Call-outs:
A lot of the allocator code has been copied/modified from #2483. I considered trying to make some generic code between the two, but decided against it for now, since I want the implementations to settle before trying to extract the overlap and try to figure out an interface that works for both.
Testing:
I added a model that tests random operations against the allocator/dispatcher and does some checks to make sure things were routed correctly. I ran the model for several hours and collected and committed a corpus to capture that testing.
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.