Pr/fredi/worker liveness check - #1665
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds watchdog-based kernel worker supervision and publishes per-worker driver status through shared storage. Wires status into runtime, router sources, and a new ChangesDriver status and lifecycle
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Comment |
Fredi-raspall
added
ci:+release
There was a problem hiding this comment.
Pull request overview
This PR adds infrastructure for monitoring dataplane driver worker/rx-task liveness and activity, and exposes that state via a new CLI command for operational diagnostics.
Changes:
- Introduces a per-rx-task lock-free watchdog and a supervisor loop that periodically checks/rearms it and aggregates counters.
- Publishes driver worker/rx-task status through a shared slot and exposes it via
show driver statusin the CLI. - Centralizes thread/task unexpected-exit fatal reporting via a reusable
lifecycle::ExitGuard.
Reviewed changes
Copilot reviewed 16 out of 17 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| routing/src/router/rio.rs | Switches RIO thread exit guarding to the shared ExitGuard helper and adds an exit log. |
| routing/src/router/mod.rs | Adds a driver_status CLI data source and adjusts router stop logging level. |
| routing/src/cli/handler.rs | Wires ShowDriverStatus to the corresponding CLI provider. |
| lifecycle/src/utils.rs | Adds reusable ExitGuard for fatal-on-unexpected-exit behavior. |
| lifecycle/src/lib.rs | Exposes the new lifecycle utils module and re-exports ExitGuard. |
| dataplane/src/runtime.rs | Creates driver status read/write access and threads it through router + driver startup. |
| dataplane/src/packet_processor/mod.rs | Adds driver-status provider to routing CLI sources. |
| dataplane/src/drivers/watchdog.rs | Implements a lock-free watchdog and activity tracking for rx tasks. |
| dataplane/src/drivers/status.rs | Defines driver/worker/rx-task status types, publication mechanism, and CLI formatting. |
| dataplane/src/drivers/mod.rs | Exposes the new status and watchdog driver modules. |
| dataplane/src/drivers/kernel/worker.rs | Adds rx-task watchdog pat/recording and returns worker monitors for supervision. |
| dataplane/src/drivers/kernel/mod.rs | Adds worker supervisor loop that checks watchdogs and publishes status periodically. |
| dataplane/Cargo.toml | Adds common dependency needed for CLI provider integration. |
| common/src/cliprovider.rs | Switches CLI provider support from ArcSwap* to Slot*. |
| cli/src/cliproto.rs | Adds ShowDriverStatus CLI action. |
| cli/bin/cmdtree_dp.rs | Adds show driver status command wiring to the CLI tree. |
| Cargo.lock | Updates lockfile for new/adjusted dependencies. |
| match guard.try_io(|fd| { | ||
| packet_recv( | ||
| id, | ||
| intf.if_name.as_str(), | ||
| fd.as_raw_fd(), |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
dataplane/src/drivers/kernel/worker.rs (1)
501-506: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winA
boolreturn conflates intentional pipeline drops with real TX failures.Packets that the pipeline emits with a
DoneReasonand nooifreturnfalsehere, so the caller (line 242) counts them astx_dropsalongside genuine serialize/write failures. Consider a small enum (Sent/Dropped/Failed) sototal_tx_dropsreflects only transmit errors.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@dataplane/src/drivers/kernel/worker.rs` around lines 501 - 506, Replace the bool result of tx_packet with a three-state outcome distinguishing Sent, Dropped, and Failed. Return Dropped for pipeline-completed packets with no oif, Failed only for serialization or write errors, and update the caller’s tx_drops accounting to count only Failed outcomes while preserving successful sends.
🧹 Nitpick comments (4)
dataplane/src/drivers/watchdog.rs (2)
116-124: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider deriving
DebugonActivity.
ActivityRecordderivesDebugbut the enclosing enum does not, soActivitycannot be used in{:?}diagnostics.♻️ Proposed change
-#[derive(Clone)] +#[derive(Debug, Clone)] pub enum Activity {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@dataplane/src/drivers/watchdog.rs` around lines 116 - 124, Update the Activity enum derive attributes to include Debug, relying on ActivityRecord’s existing Debug implementation so all variants support {:?} diagnostics.
51-84: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueCollapse the four identical accumulate blocks into a helper.
Each field repeats the same
fetch_update+saturating_addpattern. A small free function would cut this to four calls.♻️ Proposed refactor
+fn accumulate(counter: &AtomicU64, delta: u64) { + if delta > 0 { + let _ = counter.fetch_update(Ordering::Relaxed, Ordering::Relaxed, |v| { + Some(v.saturating_add(delta)) + }); + } +} + impl Watchdog {pub fn record(&self, rx: u64, tx: u64, ppline_drops: u64, tx_drops: u64) { - if rx > 0 { - let _ = self - .0 - .rx - .fetch_update(Ordering::Relaxed, Ordering::Relaxed, |v| { - Some(v.saturating_add(rx)) - }); - } - if tx > 0 { - ... - } + accumulate(&self.0.rx, rx); + accumulate(&self.0.tx, tx); + accumulate(&self.0.ppline_drops, ppline_drops); + accumulate(&self.0.tx_drops, tx_drops); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@dataplane/src/drivers/watchdog.rs` around lines 51 - 84, Refactor the repeated accumulation logic in Watchdog::record into a small free helper that performs fetch_update with Relaxed ordering and saturating_add. Replace the four field-specific blocks with calls to that helper, preserving the existing positive-value checks and counter behavior.dataplane/src/drivers/kernel/worker.rs (1)
358-366: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winReplace the name-based watchdog lookup with a positional zip.
ifmonitorsis built by mapping over this sameinterfacesslice inWorker::start, so it is already index-parallel. Zipping removes the O(n²) scan and theunreachable!()panic path, which would otherwise kill the worker thread if a name ever failed to match.♻️ Proposed refactor
- for kif in interfaces { - // find the watchdog for this interface - let watchdog = ifmonitors - .iter() - .find(|ifm| *ifm.ifname == *kif.name) - .map_or_else(|| unreachable!(), |ifm| ifm.watchdog.clone()); - + debug_assert_eq!(interfaces.len(), ifmonitors.len()); + for (kif, ifm) in interfaces.iter().zip(ifmonitors) { + debug_assert_eq!(*ifm.ifname, *kif.name); + let watchdog = ifm.watchdog.clone(); + let (writer, reader) = create_worker_interface(id, total_workers, &kif.name, kif.ifindex, watchdog)?;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@dataplane/src/drivers/kernel/worker.rs` around lines 358 - 366, Update the interface iteration in Worker::start to zip interfaces with ifmonitors and obtain each watchdog from the paired monitor directly. Remove the name-based find/map_or_else lookup and its unreachable!() fallback while preserving the existing create_worker_interface arguments and error propagation.dataplane/src/drivers/kernel/mod.rs (1)
50-51: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueNarrow the
cast_precision_lossallow to the offending statement.Applying it to the whole
implblock suppresses the lint for all future code in it. Only theppscomputation at line 246 needs it.♻️ Proposed change
-#[allow(clippy::cast_precision_loss)] impl DriverKernel {Then at the
ppsassignment:#[allow(clippy::cast_precision_loss)] { rx_task_status.pps = record.rx as f64 / f64::from(Self::TASK_POLL_PERIOD); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@dataplane/src/drivers/kernel/mod.rs` around lines 50 - 51, Remove the impl-level #[allow(clippy::cast_precision_loss)] from DriverKernel and scope the allowance only around the pps assignment in the relevant method, preserving the existing calculation while limiting suppression to that statement.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@dataplane/src/drivers/kernel/mod.rs`:
- Around line 212-255: Clear each terminated worker’s RX-task activity and
packet rate when handling the finished-worker branch in the monitor loop, and
also ensure the already-joined branch cannot retain stale values. Update the
corresponding entries in wk_status.rx_tasks to the inactive state with zero pps
while preserving accumulated counters and termination status.
In `@dataplane/src/drivers/kernel/worker.rs`:
- Around line 224-229: Update the ppline_drops calculation in the worker’s
pipeline processing flow to avoid subtracting output packet count from rx_pkts
when the pipeline expands packets. Count drops only when out_pkts.len() is less
than rx_pkts, otherwise use zero, and preserve the existing total_ppline_drops
accounting.
---
Outside diff comments:
In `@dataplane/src/drivers/kernel/worker.rs`:
- Around line 501-506: Replace the bool result of tx_packet with a three-state
outcome distinguishing Sent, Dropped, and Failed. Return Dropped for
pipeline-completed packets with no oif, Failed only for serialization or write
errors, and update the caller’s tx_drops accounting to count only Failed
outcomes while preserving successful sends.
---
Nitpick comments:
In `@dataplane/src/drivers/kernel/mod.rs`:
- Around line 50-51: Remove the impl-level #[allow(clippy::cast_precision_loss)]
from DriverKernel and scope the allowance only around the pps assignment in the
relevant method, preserving the existing calculation while limiting suppression
to that statement.
In `@dataplane/src/drivers/kernel/worker.rs`:
- Around line 358-366: Update the interface iteration in Worker::start to zip
interfaces with ifmonitors and obtain each watchdog from the paired monitor
directly. Remove the name-based find/map_or_else lookup and its unreachable!()
fallback while preserving the existing create_worker_interface arguments and
error propagation.
In `@dataplane/src/drivers/watchdog.rs`:
- Around line 116-124: Update the Activity enum derive attributes to include
Debug, relying on ActivityRecord’s existing Debug implementation so all variants
support {:?} diagnostics.
- Around line 51-84: Refactor the repeated accumulation logic in
Watchdog::record into a small free helper that performs fetch_update with
Relaxed ordering and saturating_add. Replace the four field-specific blocks with
calls to that helper, preserving the existing positive-value checks and counter
behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 96ba7e23-0963-47bf-9ce7-81435ba64639
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (16)
cli/bin/cmdtree_dp.rscli/src/cliproto.rscommon/src/cliprovider.rsdataplane/Cargo.tomldataplane/src/drivers/kernel/mod.rsdataplane/src/drivers/kernel/worker.rsdataplane/src/drivers/mod.rsdataplane/src/drivers/status.rsdataplane/src/drivers/watchdog.rsdataplane/src/packet_processor/mod.rsdataplane/src/runtime.rslifecycle/src/lib.rslifecycle/src/utils.rsrouting/src/cli/handler.rsrouting/src/router/mod.rsrouting/src/router/rio.rs
3210be7 to
ab6c898
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
dataplane/src/drivers/kernel/mod.rs (1)
139-141: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueStale comment.
This comment still describes the pre-refactor "join-and-log" supervisor; the code below now also does watchdog liveness checks, per-RX-task status updates, and periodic status publishing. Worth a quick wording update for future readers.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@dataplane/src/drivers/kernel/mod.rs` around lines 139 - 141, Update the supervisor comment near the worker-monitor loop to describe its current responsibilities, including watchdog liveness checks, per-RX-task status updates, and periodic status publishing, while retaining the note that worker fatal reporting is handled by each worker thread’s ExitGuard.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@dataplane/src/drivers/kernel/mod.rs`:
- Around line 184-195: Update the cancellation branch in the worker supervisor
loop to mark each joined worker as WorkerState::Terminated and reset its
rx_tasks activity/pps fields, matching the handle.is_finished() path. Publish
the updated workers_status with status_writer.publish(...) before breaking so
graceful-shutdown status queries cannot retain stale Running/Active state.
---
Nitpick comments:
In `@dataplane/src/drivers/kernel/mod.rs`:
- Around line 139-141: Update the supervisor comment near the worker-monitor
loop to describe its current responsibilities, including watchdog liveness
checks, per-RX-task status updates, and periodic status publishing, while
retaining the note that worker fatal reporting is handled by each worker
thread’s ExitGuard.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 2448d19c-974d-40ab-a5b0-9bd6232f7d64
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (14)
cli/bin/cmdtree_dp.rscli/src/cliproto.rscommon/src/cliprovider.rsdataplane/Cargo.tomldataplane/src/drivers/kernel/mod.rsdataplane/src/drivers/kernel/worker.rsdataplane/src/drivers/mod.rsdataplane/src/drivers/status.rsdataplane/src/drivers/watchdog.rsdataplane/src/packet_processor/mod.rsdataplane/src/runtime.rslifecycle/src/utils.rsrouting/src/cli/handler.rsrouting/src/router/mod.rs
💤 Files with no reviewable changes (1)
- lifecycle/src/utils.rs
🚧 Files skipped from review as they are similar to previous changes (9)
- dataplane/src/drivers/mod.rs
- cli/src/cliproto.rs
- routing/src/cli/handler.rs
- routing/src/router/mod.rs
- dataplane/src/runtime.rs
- dataplane/src/drivers/status.rs
- dataplane/src/drivers/watchdog.rs
- common/src/cliprovider.rs
- dataplane/src/drivers/kernel/worker.rs
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 16 out of 17 changed files in this pull request and generated 3 comments.
Comments suppressed due to low confidence (1)
dataplane/src/drivers/kernel/mod.rs:215
confidence: 10
tags: [logic]
`ScopedJoinHandle::is_finished()` is not available on the loom/shuttle backend’s `concurrency::thread::ScopedJoinHandle` (it only provides `join`). This breaks backend portability and will fail to compile under `--features loom`.
if let Some(handle) = monitor.handle.take() {
if handle.is_finished() {
let result = join_and_log(monitor.id, handle);
wk_status.state = WorkerState::Terminated(result);
</details>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 16 out of 17 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (2)
dataplane/src/drivers/kernel/worker.rs:270
confidence: 9
tags: [logic]
`rx_pkts + injected - output_by_pipeline` can underflow when the pipeline outputs more packets than it received (e.g., injection/duplication), wrapping to a huge `u64` and corrupting stats.
let ppline_drops = rx_pkts + injected - output_by_pipeline;
**dataplane/src/drivers/kernel/worker.rs:390**
* ```yaml
confidence: 8
tags: [style]
This error message loses the interface context, which makes diagnosing mismatches between interfaces and ifmonitors harder.
let watchdog = ifmonitors
.iter()
.find(|ifm| ifm.ifname.as_ref() == kif.name.as_str())
.map(|ifm| ifm.watchdog.clone())
.ok_or(io::Error::other("Failed to find interface watchdog"))?;
fa6ddff to
297734b
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 16 out of 17 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (2)
dataplane/src/drivers/kernel/worker.rs:270
confidence: 9
tags: [logic]
`ppline_drops` is computed with plain `u64` subtraction (`rx_pkts + injected - output_by_pipeline`), which can underflow if the pipeline outputs more packets than it was given (the `NetworkFunction::process` contract does not guarantee a 1:1 output). This will wrap to a huge value in release builds and corrupt drop stats.
Use saturating arithmetic so the stat is never negative.
let ppline_drops = rx_pkts + injected - output_by_pipeline;
**dataplane/src/drivers/kernel/worker.rs:256**
* ```yaml
confidence: 8
tags: [style]
The if false { /* injected */ } block is unreachable and makes it look like injected packets are already tracked when they are not. This is easy to forget later and will keep injected permanently at 0.
Either implement the injected-packet detection or replace this with a TODO comment until the metadata exists.
if false {
/* packet was injected by us */
injected += 1;
}
| impl<T> CliDataProvider for SlotOption<T> | ||
| where | ||
| T: CliDataProvider, | ||
| { |
qmonnet
left a comment
qmonnet
left a comment
There was a problem hiding this comment.
Looks good as far as I can tell. I've got some minor nits and questions, but nothing blocking. Copilot's comment about MSG_DONTWAIT may need to be addressed, though?
Thanks!
| } | ||
| impl Drop for ExitGuard { |
There was a problem hiding this comment.
Nit: Add a newline between the two impl
| let cancel = subsystem.cancel_token(); | ||
| let interfaces = interfaces.to_vec(); | ||
|
|
||
| let thread_builder = thread::Builder::new().name(format!("dp-worker-{id}")); |
There was a problem hiding this comment.
Nit: Commit title has a typo, feat(kernel-driver): do not pass thread builer (builder)
yeah, although it is not the result of a change in this PR. |
Add a generic ExitGuard to the lifecycle crate to avoid ad-hoc definitions. Signed-off-by: Fredi Raspall <fredi@githedgehog.com>
Do not pass a thread builder but create one in Worker::start() Signed-off-by: Fredi Raspall <fredi@githedgehog.com>
Let Worker::start() consume the Worker (non-mutably), as the worker is not use once start() is called and this avoids several clones(). Also, let start() return the id of the worker, in all cases (except panic). This is needed for subsequent changes since nowadays we rely on the ordering in the iterator of join handles for the worker id, which will not respect ids if elements are removed. Signed-off-by: Fredi Raspall <fredi@githedgehog.com>
Reorganize Worker::start() moving the per interface task logic to a separate method spawn_worker_interface_reader() and document the code. This allows removing #[allow(clippy::too_many_lines)] and has no functional change. Signed-off-by: Fredi Raspall <fredi@githedgehog.com>
297734b to
1f40bba
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 16 out of 17 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (5)
dataplane/src/drivers/kernel/mod.rs:78
confidence: 10
tags: [logic]
`WorkerMonitor` is declared with a lifetime parameter (`WorkerMonitor<'scope>`), but this local type annotation omits it, which will not compile. Either include the lifetime or let inference handle the type.
) -> Result<Vec<WorkerMonitor<'scope>>, std::io::Error> {
let mut monitors: Vec<WorkerMonitor> = Vec::with_capacity(num_workers);
**dataplane/src/drivers/kernel/mod.rs:281**
* ```yaml
confidence: 9
tags: [logic]
concurrency::thread::ScopedJoinHandle under the loom backend (and likely shuttle) does not provide is_finished(), so this supervisor loop won’t compile under those backends. Consider cfg-gating the non-blocking termination check and falling back to only joining on cancellation for loom/shuttle.
if let Some(handle) = monitor.handle.take() {
if handle.is_finished() {
// join the worker
let result = Self::join_worker(monitor.id, handle);
dataplane/src/drivers/kernel/worker.rs:271
confidence: 9
tags: [logic]
`rx_pkts + injected - output_by_pipeline` can underflow when the pipeline outputs more packets than it consumes (e.g., injection/duplication), which will wrap in release builds and corrupt stats. Use a saturating/checked computation to keep stats bounded.
let ppline_drops = rx_pkts + injected - output_by_pipeline;
**common/src/cliprovider.rs:73**
* ```yaml
confidence: 8
tags: [docs]
This impl is now for SlotOption<T>, but the inline comment below still references arc_swap::ArcSwapOption. Updating that comment would avoid confusion about why type inference is needed here (backend-dependent Arc types).
impl<T> CliDataProvider for SlotOption<T>
where
T: CliDataProvider,
{
dataplane/src/drivers/status.rs:6
confidence: 8
tags: [docs]
The module docs say these types ideally wouldn’t depend on the driver type, but this module imports `DriverKernel` for formatting constants. Either remove that dependency or update the docs to match the current design.
//! Driver status, published by the supervisor
//! Ideally, these types would not depend on the type of driver.
//!
</details>
The watchdog is meant to detect if a worker's rx task is stuck and also measure its activity, such as the packets it received, sent or dropped on tx or its pipeline. Signed-off-by: Fredi Raspall <fredi@githedgehog.com>
Defines type DriverStatus that summarizes the state of the kernel driver. The model is as follows. The DriverStatus has list of WorkerStatus, which indicates the state of a worker thread. A worker can be running or terminated. Each WorkerStatus in turn contains a list of RxTaskStatus one for each rx task spawn by the worker (there is a task per interface). A RxTaskStatus indicates the status of the task (idle, active or stuck) and keeps activity stats for the task, such as the number of packets it processed, sent or dropped. This commit also defines types DriverStatusReader/Writer to be able to access the status of a driver from other threads and implements Display for the above types to be able to show their contents over the CLI. Lastly, several constants (defaults) used to monitor the state of workers and their tasks are introduced. Signed-off-by: Fredi Raspall <fredi@githedgehog.com>
Reworks worker start, the supervisor and worker rx tasks as follows. 1) When started, each worker gets a WorkerMonitor for the supervisor to monitor its state and that of its rx tasks. Each task is monitored with a WorkerIfaceMonitor. 2) Each worker rx task has a WorkerInterfaceReader that is augmented with a watchdog that the rx task has to pat in time. The watchdog object also includes activity stats set by the task to know how active it is. 3) the prior worker supervisor would just wait (join) for workers to finish and cancel them if needed. It is reworked now to loop and periodically check worker's status and that of their tasks, arming the watchdogs and collecting rx task stats. Watchdog checking (and rearm), and activity collection are decoupled. 4) When started, the kernel driver is given a DriverStatusWriter for it to publish the status of the workers and their tasks outside of the supervisor thread. The CLI is given a DriverStatusReader to be able to access the status and show it. Signed-off-by: Fredi Raspall <fredi@githedgehog.com>
Signed-off-by: Fredi Raspall <fredi@githedgehog.com>
Signed-off-by: Fredi Raspall <fredi@githedgehog.com>
Signed-off-by: Fredi Raspall <fredi@githedgehog.com>
Adds a DoneReason in a case that should never happen in practice Signed-off-by: Fredi Raspall <fredi@githedgehog.com>
All packets being processed by a pipeline should get a verdict (DoneReason) at the end of it, which is checked by the last stage that collects stats. Prefer an error log over a panic if a packet does not get a verdict. Signed-off-by: Fredi Raspall <fredi@githedgehog.com>
Signed-off-by: Fredi Raspall <fredi@githedgehog.com>
1f40bba to
408e8a3
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 18 out of 19 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (4)
dataplane/src/drivers/kernel/mod.rs:77
confidence: 9
tags: [logic]
`WorkerMonitor` has a lifetime parameter (`WorkerMonitor<'scope>`), so `Vec<WorkerMonitor>` is not a valid concrete type here and can fail to compile depending on the compiler/version. Let inference pick up the correct lifetime from the function’s return type (or spell it explicitly).
let mut monitors: Vec<WorkerMonitor> = Vec::with_capacity(num_workers);
**dataplane/src/drivers/kernel/mod.rs:280**
* ```yaml
confidence: 9
tags: [logic]
handle.is_finished() is used here, but the loom implementation of concurrency::thread::ScopedJoinHandle does not define is_finished (see concurrency/src/thread/loom_scope.rs, where only join(self) exists). This makes the driver supervisor non-portable to loom/shuttle backends.
If you need to keep the non-blocking check, concurrency::thread::ScopedJoinHandle should grow an is_finished(&self) -> bool API across backends.
if let Some(handle) = monitor.handle.take() {
if handle.is_finished() {
// join the worker
dataplane/src/drivers/kernel/worker.rs:260
confidence: 9
tags: [logic]
This block contains dead code (`if false`) so `injected` is always 0, and `rx_pkts + injected - num_out_pkts` can underflow (wrap to a huge `u64`) if the pipeline ever emits more packets than it ingests (which the surrounding comment explicitly calls out as possible with injection). Use saturating arithmetic and remove the placeholder branch.
let mut injected: u64 = 0;
// send each of the packets
for out_pkt in out_pkts {
if false {
**dataplane/src/drivers/kernel/mod.rs:126**
* ```yaml
confidence: 10
tags: [docs, logic]
The docstring says this returns true when the subsystem was cancelled, but the implementation returns true when it is not cancelled. This makes the supervisor loop logic harder to reason about.
/// Check if the `Subsystem` got cancelled (and we must shutdown). If so,
/// join all of the workers. This will wait for all workers to finish.
/// Returns `true` if the subsystem was cancelled and `false` otherwise.
fn must_run(
|
Seems like iperf3 is just fine: |
qmonnet
left a comment
qmonnet
left a comment
There was a problem hiding this comment.
Fredi-raspall requested a review from qmonnet 15 hours ago
I already approved the previous version, this still looks good 🙂
Fixes #1653
Sample cli output
For each task, it shows the status (stuck|idle|Active) and