From 919e7cecfb8a657b39a1f60dd96ef8566cb7e60e Mon Sep 17 00:00:00 2001
From: Cameron Bytheway <bytheway.cameron@gmail.com>
Date: Mon, 1 May 2023 21:06:30 -0600
Subject: [PATCH] more feedback

---
 tools/xdp/s2n-quic-xdp/src/task/completion_to_tx.rs |  6 ++++--
 .../src/task/completion_to_tx/assign.rs             | 13 +++++++------
 tools/xdp/s2n-quic-xdp/src/task/rx.rs               |  2 ++
 3 files changed, 13 insertions(+), 8 deletions(-)

diff --git a/tools/xdp/s2n-quic-xdp/src/task/completion_to_tx.rs b/tools/xdp/s2n-quic-xdp/src/task/completion_to_tx.rs
index aa54f1c9f3..66abdf037e 100644
--- a/tools/xdp/s2n-quic-xdp/src/task/completion_to_tx.rs
+++ b/tools/xdp/s2n-quic-xdp/src/task/completion_to_tx.rs
@@ -33,7 +33,7 @@ pub async fn completion_to_tx<P: Poller>(
         frame_size.is_power_of_two(),
         tx_queues.len().is_power_of_two(),
     ) {
-        (0, _, _) => panic!("invalid tx_queues size"),
+        (0, _, _) => panic!("invalid must be non-zero length"),
         (1, _, _) => {
             trace!("using single queue mode");
             CompletionRingToTx {
@@ -101,7 +101,9 @@ pub async fn completion_to_tx<P: Poller>(
 
 /// Polls the completion queue for progress
 pub trait Poller: Unpin {
+    /// Polls the completion queue for progress
     fn poll(&mut self, comp: &mut ring::Completion, cx: &mut Context) -> Poll<Option<u32>>;
+    /// Releases `count` number of entries
     fn release(&mut self, comp: &mut ring::Completion, count: usize);
 }
 
@@ -271,7 +273,7 @@ impl<T: Txs, A: assign::Assign, P: Poller> Future for CompletionRingToTx<T, A, P
                                     .iter()
                                     .take(count as _)
                                     .copied()
-                                    .filter(|desc| assignment.assign(*desc, idx))
+                                    .filter(|desc| assignment.is_assigned(*desc, idx))
                                     .map(|desc| {
                                         trace!("assigning address {} to queue {idx}", desc.address);
 
diff --git a/tools/xdp/s2n-quic-xdp/src/task/completion_to_tx/assign.rs b/tools/xdp/s2n-quic-xdp/src/task/completion_to_tx/assign.rs
index 3215cf16ff..528a31b749 100644
--- a/tools/xdp/s2n-quic-xdp/src/task/completion_to_tx/assign.rs
+++ b/tools/xdp/s2n-quic-xdp/src/task/completion_to_tx/assign.rs
@@ -9,12 +9,12 @@ use crate::if_xdp::UmemDescriptor;
 /// 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;
+    fn is_assigned(&self, desc: UmemDescriptor, idx: u64) -> bool;
 }
 
 impl Assign for () {
     #[inline]
-    fn assign(&self, _desc: UmemDescriptor, idx: u64) -> bool {
+    fn is_assigned(&self, _desc: UmemDescriptor, idx: u64) -> bool {
         debug_assert_eq!(
             idx, 0,
             "assignment mode should only be used for single queue workflows"
@@ -33,7 +33,7 @@ pub struct AssignGeneric<F: FrameToIndex, I: IndexToQueue> {
 
 impl<F: FrameToIndex, I: IndexToQueue> Assign for AssignGeneric<F, I> {
     #[inline]
-    fn assign(&self, desc: UmemDescriptor, idx: u64) -> bool {
+    fn is_assigned(&self, desc: UmemDescriptor, idx: u64) -> bool {
         let v = self.frame.frame_to_index(desc);
         let v = self.index.index_to_queue(v);
         v == idx
@@ -51,7 +51,8 @@ pub struct AlignedFrame {
 
 impl AlignedFrame {
     pub fn new(frame_size: u32) -> Self {
-        debug_assert!(frame_size.is_power_of_two());
+        assert!(frame_size.is_power_of_two());
+        assert!(frame_size > 2, "cannot take the square root of 2");
 
         let shift = frame_size.trailing_zeros();
 
@@ -163,7 +164,7 @@ mod tests {
                 for offset in [0, 1, 2, (frame_size - 1) as _] {
                     let mut desc = desc;
                     desc.address += offset;
-                    let was_assigned = assigner.assign(desc, queue as _);
+                    let was_assigned = assigner.is_assigned(desc, queue as _);
                     assert_eq!(
                         is_expected, was_assigned,
                         "desc: {desc:?}, expected_queue: {expected_queue}, queue: {queue}"
@@ -192,7 +193,7 @@ mod tests {
 
         let is_expected = queue == expected_queue;
 
-        let was_assigned = assigner.assign(desc, queue);
+        let was_assigned = assigner.is_assigned(desc, queue);
         assert_eq!(is_expected, was_assigned,);
     }
 
diff --git a/tools/xdp/s2n-quic-xdp/src/task/rx.rs b/tools/xdp/s2n-quic-xdp/src/task/rx.rs
index fe6d616d88..63adbfafa0 100644
--- a/tools/xdp/s2n-quic-xdp/src/task/rx.rs
+++ b/tools/xdp/s2n-quic-xdp/src/task/rx.rs
@@ -298,6 +298,8 @@ impl<P: Poller, N: Notifier> Future for Rx<P, N> {
                 "the number of actual items should not exceed what was acquired"
             );
 
+            // While we have a `debug_assert` above, this is being overly defensive just in case.
+            // In regular conditions, it's equivalent to just releasing `actual`.
             let len = len.min(actual);
 
             // release the entries back to the RX ring