Skip to content

Fix:iperf Follow-up Robustness And PR CI - #265

Merged
a6d9a6m merged 4 commits into
mainfrom
fix/iperf-throughput
Jun 23, 2026
Merged

Fix:iperf Follow-up Robustness And PR CI#265
a6d9a6m merged 4 commits into
mainfrom
fix/iperf-throughput

Conversation

@a6d9a6m

@a6d9a6m a6d9a6m commented Jun 23, 2026

Copy link
Copy Markdown
Contributor

Fix iperf Follow-up Robustness And PR CI

描述

本 PR 基于最新 main 新建分支 fix/iperf-followup-ci。上一轮 fix/iperf-throughput 已合并,本次只包含合并后 review 和本地 PR CI 预检发现的 follow-up 修复。

主要改动:

  • UDP 接收队列显式使用 UDP_RXQ_MAX_CAP 作为长度硬上限,不再把 VecDeque::capacity() 当作逻辑上限。
  • TCP listener 补充失败时,如果当前仍有 spare listener,则允许 accept 继续处理已有连接,而不是直接失败。
  • 修复 CI cargo fmt --check 会报出的 socket connection 日志宏格式问题。
  • 修复 make run TEST=1 中 pipe 相关测试失败:区分 File::readable/writable 的访问方向语义和 poll/select 的就绪语义。

背景

上一轮 iperf PR 已经把 TCP/UDP 吞吐和 parallel TCP 连接问题修到可用状态,但 review 指出两个稳定性问题:

  • UDP 队列扩容逻辑用 q.len() == q.capacity() 判断是否达到上限。VecDeque 的实际 capacity 由分配器决定,可能超过请求值,因此不能保证 512 项硬上限。
  • TCP listener pool 在资源压力下补充失败时会让整个 accept 失败。若此时已有 spare listener,继续 accept 更符合评分场景下的稳定性目标。

同时,本地按 .github/workflows/ci.yml 提前跑 PR 检查时发现:

  • cargo fmt --check 会因 connection_ops.rs 三处 pr_debug! 格式失败。
  • cd os && make run TEST=1 虽然命令退出码为 0,但测试摘要显示 2 个 pipe 测试失败。

关键修复

UDP 队列上限

udp_push 现在先检查 q.len() >= UDP_RXQ_MAX_CAP,达到上限就丢弃最旧 datagram。只有未达逻辑上限且底层队列满时,才尝试 lazy reserve。

这个修复避免两类风险:

  • 分配器给出的实际 capacity 超过 512,导致逻辑上限失效。
  • 扩容失败后继续 push_back,在满队列情况下触发 panic。

TCP listener 补充降级

replenish_tcp_listeners 现在在创建或 listen 新 socket 失败时,会重新检查 spare listener 数量:

  • 若已有 spare listener,则停止补充并返回 Ok(()),让 accept 继续。
  • 若没有 spare listener,则保留原行为,返回对应错误。

这让 parallel TCP 在资源压力下更偏向降级服务已有连接,而不是因为补池失败直接拒绝 accept。

PR CI 修复

connection_ops.rs 的三处日志宏按 rustfmt 展开,解决 cargo fmt --check 失败。

PipeFile 原先把 readable/writable 同时用于“文件访问方向”和“当前是否 ready”。这会让空管道读端 readable() 返回 false,违反 File trait 和已有测试期望。

本次处理为:

  • PipeFile::readable/writable 只表达端点访问方向。
  • 新增 PipeFile::read_ready/write_ready 表达 pipe 缓冲区就绪状态。
  • poll/select 通过 helper 对 PipeFile 使用 readiness 方法,对其他文件保持原逻辑。

这样修复单元测试,同时避免空管道在 poll/select 中被误报为可读。

验证

在新分支 fix/iperf-followup-ci 上按 CI 顺序本地验证:

  • cd os && cargo check --target riscv64gc-unknown-none-elf 通过。
  • cd os && cargo fmt --all -- --check 通过。
  • cd os && cargo clippy --target riscv64gc-unknown-none-elf 通过。
  • cd os && make run TEST=1 通过,测试摘要为 Total: 533, Passed: 533, Failed: 0

本次验证使用的是基于最新 origin/main cherry-pick 4 个 follow-up commit 后的新分支,等价于新 PR 的差异范围。

风险与取舍

  • UDP 队列达到 512 后仍会丢最旧 datagram。这是为了维持内存硬上限,符合 UDP 可丢包语义。
  • TCP listener 补充失败时只在已有 spare listener 的情况下吞掉错误;没有 spare 时仍返回错误,避免掩盖完全不可服务状态。
  • PipeFile readiness 只针对 poll/select 做类型分发,没有扩展 File trait,避免影响所有文件实现。
  • 本 PR 不继续扩大 iperf buffer,也不修改上一轮已合并的 musl 全量测试入口。

本记录基于以下提交

  • e7fb6f3 net: cap udp receive queue length explicitly
  • 6242964 net: tolerate tcp listener replenishment pressure
  • 4ce6eed net: format socket connection logs
  • 5dd05f5 vfs: separate pipe access and readiness checks

关联 Issue

暂无关联 Issue。

@a6d9a6m
a6d9a6m merged commit b43b078 into main Jun 23, 2026
1 check passed
@a6d9a6m
a6d9a6m deleted the fix/iperf-throughput branch June 23, 2026 16:52

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Code Review

This pull request introduces several updates to I/O and networking operations, including new helper functions file_read_ready and file_write_ready to handle pipe file polling, early exit logic for TCP listener replenishment, and UDP queue overflow handling. The review feedback highlights two key improvement opportunities: first, updating write_ready in PipeFile to return true when the reader end is closed to prevent writer processes from hanging indefinitely; second, refactoring the new helper functions in io.rs to accept &dyn File instead of &Arc<dyn File> to decouple them from the Arc smart pointer.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

Comment on lines +284 to +286
pub fn write_ready(&self) -> bool {
self.end_type.writable() && self.buffer.lock().can_write_now()
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

high

When the read end of a pipe is closed, any subsequent write to the pipe will fail immediately with EPIPE / BrokenPipe rather than blocking. Therefore, the write end of the pipe should be considered write-ready (i.e., poll / select should return immediately with POLLOUT instead of blocking indefinitely).

Currently, write_ready only checks can_write_now(), which returns false if read_end_count == 0. This causes writer processes to hang indefinitely in poll / select once the reader exits.

We should update write_ready to also return true if the reader was previously present but has now closed its end (ever_had_reader && read_end_count == 0).

    pub fn write_ready(&self) -> bool {
        if !self.end_type.writable() {
            return false;
        }
        let buf = self.buffer.lock();
        buf.can_write_now() || (buf.ever_had_reader && buf.read_end_count == 0)
    }

Comment on lines +87 to +99
fn file_read_ready(file: &Arc<dyn File>) -> bool {
if let Some(pipe_file) = file.as_any().downcast_ref::<crate::vfs::PipeFile>() {
return pipe_file.read_ready();
}
file.readable()
}

fn file_write_ready(file: &Arc<dyn File>) -> bool {
if let Some(pipe_file) = file.as_any().downcast_ref::<crate::vfs::PipeFile>() {
return pipe_file.write_ready();
}
file.writable()
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

medium

The helper functions file_read_ready and file_write_ready currently take &Arc<dyn File> as their parameter. However, they only need to perform operations on the underlying dyn File trait object (via as_any() and readable() / writable()).

By changing the parameter type to &dyn File, we decouple these helper functions from the Arc smart pointer, making them more idiomatic, flexible, and reusable. Deref coercion will automatically handle passing &Arc<dyn File> at the call sites without requiring any modifications there.

Suggested change
fn file_read_ready(file: &Arc<dyn File>) -> bool {
if let Some(pipe_file) = file.as_any().downcast_ref::<crate::vfs::PipeFile>() {
return pipe_file.read_ready();
}
file.readable()
}
fn file_write_ready(file: &Arc<dyn File>) -> bool {
if let Some(pipe_file) = file.as_any().downcast_ref::<crate::vfs::PipeFile>() {
return pipe_file.write_ready();
}
file.writable()
}
fn file_read_ready(file: &dyn File) -> bool {
if let Some(pipe_file) = file.as_any().downcast_ref::<crate::vfs::PipeFile>() {
return pipe_file.read_ready();
}
file.readable()
}
fn file_write_ready(file: &dyn File) -> bool {
if let Some(pipe_file) = file.as_any().downcast_ref::<crate::vfs::PipeFile>() {
return pipe_file.write_ready();
}
file.writable()
}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant