Skip to content

hypervisor: drop tap TX frames on EIO instead of exiting the VM #1240

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
rogelioRuiz wants to merge 1 commit into
base: master
Choose a base branch
from
Open

hypervisor: drop tap TX frames on EIO instead of exiting the VM #1240

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
80 changes: 74 additions & 6 deletions hypervisor/net_util/src/queue_pair.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,12 +15,24 @@ use vm_memory::bitmap::Bitmap;
use vm_memory::{Bytes, GuestMemory};
use vm_virtio::{AccessPlatform, Translatable};

/// Whether an EIO-drop log line should be emitted after the cumulative dropped
/// count moved from `before` to `after`. Log the first drop, then once per
/// 1000-frame boundary crossed, so a persistently down tap cannot flood the
/// log at line rate. The `tx_dropped_frames` counter is the source of truth
/// for monitoring; this log is best-effort.
fn should_log_dropped(before: u64, after: u64) -> bool {
(before == 0 && after > 0) || before / 1000 != after / 1000

@cubesandboxbot cubesandboxbot Bot Jul 31, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit — the throttle is keyed to the cumulative counter, which never resets, so a new incident after the total has passed the first 1000-boundary can go completely unlogged: should_log_dropped(1500, 1501) is false, and the "first drop" branch only ever fires once in the VM's lifetime. So a tap that flaps once, months after an earlier burst left the total at, say, 1500, produces no log at all (an operator watching logs would miss it until the total crosses 2000). Consider also triggering on "first drop after a quiet period" (e.g., log when the previous value had been static for some time), or at least documenting that tx_dropped_frames is the source of truth and the log is best-effort.

}

#[derive(Clone)]
pub struct TxVirtio {
pub counter_bytes: Wrapping<u64>,
pub counter_frames: Wrapping<u64>,
pub limit_bytes: Wrapping<u64>,
pub limit_frames: Wrapping<u64>,
/// Frames the tap refused with EIO and we dropped instead of killing the
/// VM (see the EIO branch in `process_desc_chain`).
pub dropped_frames: Wrapping<u64>,

@cubesandboxbot cubesandboxbot Bot Jul 31, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

dropped_frames is counted but never surfaced: unlike counter_frames/limit_frames/limit_bytes, it is not flushed into NetCounters in NetQueuePair::process_tx and never reset, so it never appears in Net::counters() (hypervisor/virtio-devices/src/net.rs:800) or the vm counter CLI. During a down-tap period tx_frames/tx_bytes drop to ~0 and the only signal an operator gets is the throttled warn log — the drop count is invisible to monitoring. Consider adding a tx_dropped_frames field to NetCounters, flushing/resetting it in process_tx alongside the other counters, and exposing it in the counters() map (noting the log's "total dropped" would then need to read the cumulative atomic rather than self.dropped_frames.0).

}

impl Default for TxVirtio {
Expand All @@ -36,6 +48,7 @@ impl TxVirtio {
counter_frames: Wrapping(0),
limit_bytes: Wrapping(0),
limit_frames: Wrapping(0),
dropped_frames: Wrapping(0),
}
}

Expand Down Expand Up @@ -111,14 +124,22 @@ impl TxVirtio {
retry_write = true;
break;
}
error!("net: tx: failed writing to tap: {}", e);
return Err(NetQueuePairError::WriteTap(e));
}

@cubesandboxbot cubesandboxbot Bot Jul 31, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Coverage gap: the new #[cfg(test)] module only tests should_log_dropped. This EIO drop path (drop-and-continue with used length 0) and the flush/log block in process_tx are untested. Since tap.rs already has NET_ADMIN-gated integration-style tests, a test that takes a tap down, pushes a TX descriptor chain through the queue, and asserts the frame is dropped (counter incremented, descriptor marked used, worker does not error) would lock in this exact behavior and would have caught a regression in the drain/break logic.

self.counter_bytes += Wrapping(result as u64 - vnet_hdr_len() as u64);
self.counter_frames += Wrapping(1);
if e.raw_os_error() != Some(libc::EIO) {
error!("net: tx: failed writing to tap: {}", e);
return Err(NetQueuePairError::WriteTap(e));
}

result as u32
// EIO: the tap refused the frame (device down / carrier
// not up). Drop it like real hardware would and keep the
// queue alive; see the commit message for the rationale.

@cubesandboxbot cubesandboxbot Bot Aug 13, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

EIO from a tap write is not exclusively the "carrier not up" case — the kernel also returns EIO when the tap device is being torn down or is otherwise broken. With this branch, those cases no longer fail fast: the VM stays up and TX is silently discarded, which can look like a healthy VM with no egress. The counter makes this observable and it's a deliberate trade-off (matching physical-NIC drop behavior), but it's worth noting in the comment that this path also covers device teardown, and making sure any monitoring/alerting keys off tx_dropped_frames rather than assuming the VM is healthy because it's still running.

self.dropped_frames += Wrapping(1);
0
} else {
self.counter_bytes += Wrapping(result as u64 - vnet_hdr_len() as u64);
self.counter_frames += Wrapping(1);
result as u32
}
} else {
0
};
Expand Down Expand Up @@ -322,6 +343,7 @@ impl RxVirtio {
pub struct NetCounters {
pub tx_bytes: Arc<AtomicU64>,
pub tx_frames: Arc<AtomicU64>,
pub tx_dropped_frames: Arc<AtomicU64>,
pub rx_bytes: Arc<AtomicU64>,
pub rx_frames: Arc<AtomicU64>,
pub rx_limit_bytes: Arc<AtomicU64>,
Expand Down Expand Up @@ -432,10 +454,24 @@ impl NetQueuePair {
self.counters
.tx_limit_frames
.fetch_add(self.tx.limit_frames.0, Ordering::AcqRel);
if self.tx.dropped_frames.0 != 0 {
let before = self
.counters
.tx_dropped_frames
.fetch_add(self.tx.dropped_frames.0, Ordering::AcqRel);
let after = before + self.tx.dropped_frames.0;
if should_log_dropped(before, after) {
debug!(

@cubesandboxbot cubesandboxbot Bot Aug 13, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This log is at debug!, but the hypervisor defaults to LevelFilter::Info (vmm_config.rs:33), and to Warn with no -v. So in a default deployment neither the first drop nor the per-1000-boundary log is ever emitted — the drop path becomes completely silent in production and is only observable through the tx_dropped_frames counter.

Since the PR's stated goal is to make these otherwise-silent drops visible, consider emitting the first drop at info! (the existing EAGAIN "listening for TAP to become writable" log uses info!, so this would be consistent) and keeping only the per-1000-boundary logs at debug!. The first drop is a one-time, low-volume signal; the boundary logs are the flood risk that belongs at debug!.

"net: tx: tap refused frame(s) with EIO; dropping instead of failing the VM (total dropped: {})",
after
);
}
}
self.tx.counter_bytes = Wrapping(0);
self.tx.counter_frames = Wrapping(0);
self.tx.limit_bytes = Wrapping(0);
self.tx.limit_frames = Wrapping(0);
self.tx.dropped_frames = Wrapping(0);

queue
.needs_notification(mem)
Expand Down Expand Up @@ -496,3 +532,35 @@ impl NetQueuePair {
.map_err(NetQueuePairError::QueueNeedsNotification)
}
}

@cubesandboxbot cubesandboxbot Bot Jul 31, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These tests cover only should_log_dropped. The actual EIO behavior (writev → EIO → descriptor marked used with len 0, dropped_frames incremented, process_desc_chain returns Ok, queue survives) is exercised only by the manual repro in the PR description. net_util already has tap-backed tests in tap.rs that run with /dev/net/tun + NET_ADMIN (and Tap::enable() at tap.rs:490 shows the ioctl pattern); a test that brings a tap down so writes return EIO and asserts a frame is dropped rather than the queue erroring would guard this regression.


#[cfg(test)]
mod tests {
use super::should_log_dropped;

#[test]
fn logs_first_drop() {
assert!(should_log_dropped(0, 1));
assert!(should_log_dropped(0, 5));
}

#[test]
fn suppresses_within_the_same_thousand_bucket() {
assert!(!should_log_dropped(1, 2));
assert!(!should_log_dropped(500, 999));
assert!(!should_log_dropped(1000, 1500));
}

#[test]
fn logs_on_each_thousand_boundary_crossed() {
assert!(should_log_dropped(999, 1000));
assert!(should_log_dropped(1999, 2001));
// A single batch spanning several boundaries still logs once.
assert!(should_log_dropped(1500, 4200));
}

#[test]
fn no_log_when_nothing_was_dropped() {
assert!(!should_log_dropped(0, 0));
assert!(!should_log_dropped(42, 42));
}
}
4 changes: 4 additions & 0 deletions hypervisor/virtio-devices/src/net.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -824,6 +824,10 @@ impl VirtioDevice for Net {
"tx_frames",
Wrapping(self.counters.tx_frames.load(Ordering::Acquire)),
);
counters.insert(
"tx_dropped_frames",
Wrapping(self.counters.tx_dropped_frames.load(Ordering::Acquire)),
);
counters.insert(
"tx_limit_bytes",
Wrapping(self.counters.tx_limit_bytes.load(Ordering::Acquire)),
Expand Down
Loading