feat(s2n-quic-platform): add message ring using sync::Cursor #1787
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
camshaft
changed the title
camshaft
force-pushed
the
camshaft/socket-ring
branch
2 times, most recently
from
4d0ba83
to
97ee823
Compare
camshaft
force-pushed
the
camshaft/socket-ring
branch
from
97ee823
to
901069f
Compare
toidiu
reviewed
Jun 8, 2023
Comment on lines
+140
to
+158
| { | ||
| let mut entry_ptr = ptr.as_ptr().add(entry_offset) as *mut UnsafeCell<Message>; | ||
| let mut payload_ptr = ptr.as_ptr().add(payload_offset) as *mut UnsafeCell<u8>; | ||
| for _ in 0..entries { | ||
| let entry = (*entry_ptr).get_mut(); | ||
| entry.payload_ptr = (*payload_ptr).get(); | ||
| entry.payload_len = payload_len as _; | ||
|
|
||
| entry_ptr = entry_ptr.add(1); | ||
| debug_assert!(end_pointer >= entry_ptr as *mut u8); | ||
| payload_ptr = payload_ptr.add(payload_len as _); | ||
| debug_assert!(end_pointer >= payload_ptr as *mut u8); | ||
| } | ||
|
|
||
| let primary = ptr.as_ptr().add(entry_offset) as *mut Message; | ||
| let secondary = primary.add(entries as _); | ||
| debug_assert!(end_pointer >= secondary.add(entries as _) as *mut u8); | ||
| core::ptr::copy_nonoverlapping(primary, secondary, entries as _); | ||
| } |
There was a problem hiding this comment.
some docs/diagrams here would be nice
Comment on lines
+290
to
+291
| #[repr(C, align(8))] | ||
| struct Aligned<T>(UnsafeCell<T>); |
There was a problem hiding this comment.
is this also an optimization trick? can you add a comment if thats true
There was a problem hiding this comment.
It's a correctness thing. I added a comment
toidiu
reviewed
Jun 8, 2023
Comment on lines
+238
to
+239
| #[inline] | ||
| unsafe fn builder<T: Message>(ptr: NonNull<u8>, size: u32) -> cursor::Builder<T> { |
There was a problem hiding this comment.
Are there power of 2 requirements for size? I know there were for XDP but maybe we also want to enforce them here. Can you add a check for that
There was a problem hiding this comment.
That's already being checked by the cursor:
|
|
||
| /// Releases ready-to-consume messages to the consumer | ||
| #[inline] | ||
| pub fn release(&mut self, len: u32) { |
There was a problem hiding this comment.
nit: size and len are confusing. can you add better names
|
|
||
| // if messages were also written to the secondary region, we need to copy them back to the | ||
| // primary region | ||
| if let Some(replication_count) = (idx + len).checked_sub(size).filter(|v| *v > 0) { |
There was a problem hiding this comment.
can you simplify or add a comment for this
toidiu
approved these changes
Jun 8, 2023
This was referenced Jun 8, 2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description of changes:
This change uses the
s2n_quic_core::sync::Cursoradded in #1774 to implement a ring buffer for passing messages between the endpoint and socket tasks. We're also adding anallocmethod to theMessagetrait that allocates the underlying ring buffer for a given message.Call-outs:
Note that we still maintain the primary/secondary regions on the ring buffer to make it possible to have a single syscall for the
mmsgmessage type. Without the primary/secondary replication, we would be left with 2 slices and would end up roughly doubling the number of syscalls performed.Testing:
I've included tests for replication behavior of each type of message to make sure that's working properly. Those also implicitly test the
allocmethods as well.By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.