feat(s2n-quic-xdp): add async tasks #1730

camshaft · 2023-04-26T21:55:53Z

Description of changes:

This PR adds a set of async tasks responsible for managing ring buffer and queue state

Fundamentally, each task takes a set of input sources and routes them to one or more output
queues. Each task is generic over the execution environment, meaning it can be using in
something driven by polling for events, like tokio, or spawned on its own thread in a busy
poll loop.

Call-outs:

These new tests caught an issue in the ring cursor implementation that deadlocked if the consumer consumed the entire ring from being completely full. Very fun to debug that one...

Testing:

The ordering of operations in each of the tasks is critical for correctness. It's very easy to
get into a deadlock if things aren't exactly right. As such, each task has a fuzz test that
tries to show the tasks working properly, even in extreme cases.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

camshaft · 2023-04-27T02:40:22Z

tools/xdp/s2n-quic-xdp/src/ring/cursor.rs

+        let mut new_value = self.consumer().load(Ordering::Acquire);
+
+        // Our cached copy has the size added so we also need to add the size here when comparing
+        //
+        // See `Self::init_producer` for more details
+        new_value += self.size;


This was a fun bug to hunt down... The optimization that I put in to compare the new_value against the cached value backfired and caused a deadlock when the consumer incremented the cursor by size (meaning they consumed the entire ring).

I added a kani proof to make sure the fix worked as well.

Without

SUMMARY: ** 1 of 1720 failed (9 unreachable) Failed Checks: attempt to add with overflow File: "/home/cameron/Projects/aws/s2n-quic/tools/xdp/s2n-quic-xdp/src/ring/cursor.rs", line 154, in ring::cursor::Cursor::<u32>::acquire_producer VERIFICATION:- FAILED Verification Time: 65.64434s

With

SUMMARY: ** 0 of 1719 failed (9 unreachable) VERIFICATION:- SUCCESSFUL Verification Time: 72.64367s

camshaft · 2023-04-27T02:42:26Z

tools/xdp/s2n-quic-xdp/src/task/completion_to_tx.rs

+        // let the poller know how many items we consumed
+        poller.release(comp, sent);
+
+        // if all of the queues are closed then shut down the task
+        if closed == txs.len() {
+            trace!("all tx queues closed; shutting down");
+            return Poll::Ready(());
+        }


Initially I had the ordering of these two reversed, which caused the fuzz tests to randomly fail at the end and drop a few descriptors.

camshaft · 2023-04-27T02:44:05Z

tools/xdp/s2n-quic-xdp/src/task/completion_to_tx/assign.rs

+        debug_assert!(frame_size.is_power_of_two());
+
+        let shift = frame_size.trailing_zeros();
+
+        debug_assert_eq!(
+            frame_size,
+            2u32.pow(shift),
+            "computing the square root of a power of two is counting the trailing zeros"
+        );
+
+        Self { shift }


The idea here is if we know that the frame_size is a power of two, it's much cheaper to shift bits than perform regular integer division.

camshaft · 2023-04-27T02:44:45Z

tools/xdp/s2n-quic-xdp/src/task/completion_to_tx/assign.rs

+        let queues = queues as u64;
+        debug_assert!(queues.is_power_of_two());
+        let mask = queues - 1;
+        Self { mask }


Same here. We can mask off the final result and it's equivalent to doing a mod operation.

camshaft · 2023-04-27T02:46:23Z

tools/xdp/s2n-quic-xdp/src/task/testing.rs

+}
+
+/// The number of items to send through the test queues
+pub const TEST_ITEMS: usize = 10_000;


Before pushing I set this to 10_000_000_000 and let it run for about 30 minutes so I think we can be pretty confident in it holding the invariants defined in the test harness.

maddeleine · 2023-04-27T19:27:24Z

tools/xdp/s2n-quic-xdp/src/task/testing.rs

+///
+/// This value is purposefully low to more frequently trigger corner cases of
+/// queues wrapping and/or getting full.
+pub const QUEUE_SIZE: usize = 16;


Should we also be testing with larger queue sizes than this? I don't expect this is close to what we would actually want the queue size to be.

Yes this is much smaller than what the production queue sizes will be. However, I think after a certain size the implementation will behave the same. As long as you have a few primes and even numbers in the range I think we should be fine. That being said, it's not hard to add more at larger sizes if we'd like.

As mentioned in the comments it's set low enough where we're running into the edge cases more frequently so the testing time is better utilized.

added large queue sizes in badcca6

WesleyRosenblum

Still reviewing, I've looked at everything except for src/task/*

tools/xdp/s2n-quic-xdp/Cargo.toml

WesleyRosenblum · 2023-04-28T00:50:36Z

tools/xdp/s2n-quic-xdp/src/ring.rs

+
+    #[test]
+    fn rx_tx_test() {
+        let _ = rx_tx(16);


Was 16 chosen here for the same reason as QUEUE_SIZE_SMALL?

This one is arbitrary. Just needs to be a power of two and relatively small so we don't consume a bunch of memory in the unit test

WesleyRosenblum · 2023-04-28T01:09:33Z

tools/xdp/s2n-quic-xdp/src/socket.rs

+
+    pub fn attach_umem(&self, umem: &crate::umem::Umem) -> Result<()> {
+        umem.attach(self)?;
+        // TODO store the umem


you need an issue for this?

Nah. I'm going to be addressing it with the next PRs

tools/xdp/s2n-quic-xdp/src/task.rs

WesleyRosenblum · 2023-05-01T22:36:20Z

tools/xdp/s2n-quic-xdp/src/task/completion_to_tx.rs

+        frame_size.is_power_of_two(),
+        tx_queues.len().is_power_of_two(),
+    ) {
+        (0, _, _) => panic!("invalid tx_queues size"),


Suggested change

(0, _, _) => panic!("invalid tx_queues size"),

(0, _, _) => unreachable!("tx_queues must be non-zero length"),

tools/xdp/s2n-quic-xdp/src/task/completion_to_tx.rs

WesleyRosenblum · 2023-05-01T23:20:39Z

tools/xdp/s2n-quic-xdp/src/task/completion_to_tx/assign.rs

+/// which TX queues get which descriptors. This trait takes in a descriptor and decides if it
+/// pertains to a worker index or not.
+pub trait Assign: Unpin {
+    fn assign(&self, desc: UmemDescriptor, idx: u64) -> bool;


would is_assigned be more appropriate here since its not actually assigning anything?

Yeah that's fine

tools/xdp/s2n-quic-xdp/src/task/completion_to_tx/assign.rs

tools/xdp/s2n-quic-xdp/src/task/rx.rs

WesleyRosenblum · 2023-05-01T23:55:54Z

tools/xdp/s2n-quic-xdp/src/task/rx.rs

+                "the number of actual items should not exceed what was acquired"
+            );
+
+            let len = len.min(actual);


does the comment from completion_to_tx.rs apply here as well?

This one is a bit different. We made a debug assertion above it so this should be the same as just doing let len = actual, assuming that assertion holds. Just a little defensive here.

tools/xdp/s2n-quic-xdp/src/task/completion_to_tx.rs

Co-authored-by: Wesley Rosenblum <[email protected]>

camshaft force-pushed the camshaft/xsk-tasks branch from f5176b5 to 35ce613 Compare April 27, 2023 02:38

camshaft commented Apr 27, 2023

View reviewed changes

camshaft marked this pull request as ready for review April 27, 2023 02:50

WesleyRosenblum self-requested a review April 27, 2023 18:16

maddeleine reviewed Apr 27, 2023

View reviewed changes

WesleyRosenblum reviewed Apr 28, 2023

View reviewed changes

WesleyRosenblum reviewed May 2, 2023

View reviewed changes

camshaft added 4 commits May 1, 2023 21:00

feat(s2n-quic-xdp): add async tasks

2abfdf0

fix cursor wrapping

75a1e35

add tests for large queue sizes

0ad5d21

more feedback

919e7ce

camshaft force-pushed the camshaft/xsk-tasks branch from a4a97f0 to 919e7ce Compare May 2, 2023 03:06

WesleyRosenblum previously approved these changes May 2, 2023

View reviewed changes

tools/xdp/s2n-quic-xdp/src/task/completion_to_tx.rs Outdated Show resolved Hide resolved

Update tools/xdp/s2n-quic-xdp/src/task/completion_to_tx.rs

8721eeb

Co-authored-by: Wesley Rosenblum <[email protected]>

camshaft dismissed WesleyRosenblum’s stale review via 8721eeb May 2, 2023 16:23

camshaft enabled auto-merge (squash) May 2, 2023 16:23

camshaft requested a review from WesleyRosenblum May 2, 2023 16:27

WesleyRosenblum approved these changes May 2, 2023

View reviewed changes

camshaft merged commit e4314d2 into main May 2, 2023

camshaft deleted the camshaft/xsk-tasks branch May 2, 2023 16:54

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(s2n-quic-xdp): add async tasks #1730

feat(s2n-quic-xdp): add async tasks #1730

camshaft commented Apr 26, 2023 •

edited

Loading

camshaft Apr 27, 2023 •

edited

Loading

camshaft Apr 27, 2023

camshaft Apr 27, 2023

camshaft Apr 27, 2023

camshaft Apr 27, 2023 •

edited

Loading

maddeleine Apr 27, 2023

camshaft Apr 27, 2023

camshaft Apr 27, 2023

WesleyRosenblum left a comment

WesleyRosenblum Apr 28, 2023

camshaft Apr 28, 2023

WesleyRosenblum Apr 28, 2023

camshaft Apr 28, 2023

WesleyRosenblum May 1, 2023

WesleyRosenblum May 1, 2023

camshaft May 2, 2023

WesleyRosenblum May 1, 2023

camshaft May 2, 2023

	(0, _, _) => panic!("invalid tx_queues size"),
	(0, _, _) => unreachable!("tx_queues must be non-zero length"),

feat(s2n-quic-xdp): add async tasks #1730

feat(s2n-quic-xdp): add async tasks #1730

Conversation

camshaft commented Apr 26, 2023 • edited Loading

Description of changes:

Call-outs:

Testing:

camshaft Apr 27, 2023 • edited Loading

Choose a reason for hiding this comment

Without

With

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

camshaft Apr 27, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

WesleyRosenblum left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

camshaft commented Apr 26, 2023 •

edited

Loading

camshaft Apr 27, 2023 •

edited

Loading

camshaft Apr 27, 2023 •

edited

Loading