Fix:iperf Follow-up Robustness And PR CI - #265
Conversation
There was a problem hiding this comment.
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.
| pub fn write_ready(&self) -> bool { | ||
| self.end_type.writable() && self.buffer.lock().can_write_now() | ||
| } |
There was a problem hiding this comment.
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)
}| 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() | ||
| } |
There was a problem hiding this comment.
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.
| 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() | |
| } |
Fix iperf Follow-up Robustness And PR CI
描述
本 PR 基于最新
main新建分支fix/iperf-followup-ci。上一轮fix/iperf-throughput已合并,本次只包含合并后 review 和本地 PR CI 预检发现的 follow-up 修复。主要改动:
UDP_RXQ_MAX_CAP作为长度硬上限,不再把VecDeque::capacity()当作逻辑上限。accept继续处理已有连接,而不是直接失败。cargo fmt --check会报出的 socket connection 日志宏格式问题。make run TEST=1中 pipe 相关测试失败:区分File::readable/writable的访问方向语义和poll/select的就绪语义。背景
上一轮 iperf PR 已经把 TCP/UDP 吞吐和 parallel TCP 连接问题修到可用状态,但 review 指出两个稳定性问题:
q.len() == q.capacity()判断是否达到上限。VecDeque的实际 capacity 由分配器决定,可能超过请求值,因此不能保证 512 项硬上限。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。这个修复避免两类风险:
push_back,在满队列情况下触发 panic。TCP listener 补充降级
replenish_tcp_listeners现在在创建或 listen 新 socket 失败时,会重新检查 spare listener 数量:Ok(()),让accept继续。这让 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/maincherry-pick 4 个 follow-up commit 后的新分支,等价于新 PR 的差异范围。风险与取舍
poll/select做类型分发,没有扩展 File trait,避免影响所有文件实现。本记录基于以下提交
e7fb6f3 net: cap udp receive queue length explicitly6242964 net: tolerate tcp listener replenishment pressure4ce6eed net: format socket connection logs5dd05f5 vfs: separate pipe access and readiness checks关联 Issue
暂无关联 Issue。