fix(tun): fixed performance degradation #894
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR addresses performance degradation in the TUN network stack by replacing spin locks with a lock-free ring buffer implementation. The changes remove synchronization bottlenecks that were causing performance issues in TCP stream handling.
Key changes:
- Replaced spin lock-based
Protected<RingBuffer>with a lock-freeLockFreeRingBufferimplementation - Simplified async read/write operations by removing lock contention
- Added debugging capabilities and example improvements for better testing
Reviewed Changes
Copilot reviewed 8 out of 9 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| clash-netstack/src/ring_buffer.rs | New lock-free ring buffer implementation using atomic operations |
| clash-netstack/src/spin_lock.rs | Removed spin lock implementation entirely |
| clash-netstack/src/tcp_stream.rs | Updated to use lock-free buffers and simplified async I/O operations |
| clash-netstack/src/tcp_listener.rs | Modified to use new ring buffer type and removed lock usage |
| clash-netstack/src/stack.rs | Added Debug implementation for IfaceEvent enum |
| clash-netstack/src/lib.rs | Updated module imports to reflect structural changes |
| clash-netstack/examples/with_tun_rs.rs | Enhanced example with better error handling and Linux support |
| clash-netstack/examples/README.md | Added performance benchmark results |
|
|
||
| // Calculate available space | ||
| let available = if read_pos <= write_pos { | ||
| self.capacity - write_pos + read_pos - 1 |
There was a problem hiding this comment.
The available space calculation is incorrect when read_pos <= write_pos. It should be self.capacity - (write_pos - read_pos) - 1 to properly account for the one slot that must remain empty to distinguish full from empty.
| self.capacity - write_pos + read_pos - 1 | |
| self.capacity - (write_pos - read_pos) - 1 |
|
|
||
| std::task::Poll::Ready(Ok(())) | ||
| }) | ||
| if read_buf.is_empty() { |
There was a problem hiding this comment.
There's a potential race condition here. The buffer could become non-empty between the is_empty() check and the return Poll::Pending. Consider using atomic operations or checking again after registering the waker.
| let n = buf_lock.enqueue_slice(buf); | ||
| std::task::Poll::Ready(Ok(n)) | ||
| }) | ||
| if send_buf.is_full() { |
There was a problem hiding this comment.
Similar race condition as in poll_read. The buffer state could change between the is_full() check and returning Poll::Pending. The double-check pattern that was removed provided protection against this.
|
|
||
| self.stack_notifier | ||
| .send(IfaceEvent::TcpSocketReady) | ||
| .expect("Failed to notify TCP socket ready"); |
There was a problem hiding this comment.
Using expect() with a panic on notification failure may be too harsh. Consider logging the error and gracefully handling the failure case instead of panicking.
| .expect("Failed to notify TCP socket ready"); | |
| if let Err(e) = self.stack_notifier.send(IfaceEvent::TcpSocketReady) { | |
| error!("Failed to notify TCP socket ready: {e}"); | |
| } |
|
|
||
| self.stack_notifier | ||
| .send(IfaceEvent::TcpSocketReady) | ||
| .expect("Failed to notify TCP socket ready"); |
There was a problem hiding this comment.
Same issue as above - using expect() for notification failures may cause unnecessary panics. Consider error logging instead.
| .expect("Failed to notify TCP socket ready"); | |
| if let Err(e) = self.stack_notifier.send(IfaceEvent::TcpSocketReady) { | |
| error!("Failed to notify TCP socket ready: {e}"); | |
| } |
|
|
||
| pub fn is_empty(&self) -> bool { | ||
| self.read_pos.load(Ordering::Acquire) | ||
| == self.write_pos.load(Ordering::Acquire) |
There was a problem hiding this comment.
[nitpick] Using Acquire ordering for both loads in is_empty() may be unnecessarily strong. Consider using Relaxed ordering since this is just a snapshot check and exact consistency isn't critical.
| == self.write_pos.load(Ordering::Acquire) | |
| self.read_pos.load(Ordering::Relaxed) | |
| == self.write_pos.load(Ordering::Relaxed) |
|
|
||
| pub fn is_full(&self) -> bool { | ||
| let read_pos = self.read_pos.load(Ordering::Acquire); | ||
| let write_pos = self.write_pos.load(Ordering::Acquire); |
There was a problem hiding this comment.
[nitpick] Similar to is_empty(), is_full() could use Relaxed ordering for better performance since it's a snapshot check.
| let write_pos = self.write_pos.load(Ordering::Acquire); | |
| let read_pos = self.read_pos.load(Ordering::Relaxed); | |
| let write_pos = self.write_pos.load(Ordering::Relaxed); |
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
🤔 This is a ...
🔗 Related issue link
💡 Background and solution
📝 Changelog
☑️ Self-Check before Merge