Conversation
|
@sukunrt (I can't tag you for review for some reason, but a review would be appreciated!) |
There was a problem hiding this comment.
Pull Request Overview
This PR implements FQ-CoDel (Fair Queuing with Controlled Delay) per RFC 8290, introducing a fairness mechanism to packet scheduling when multiple flows traverse a node. The implementation follows the Linux kernel's FQ-CoDel approach, organizing flows into buckets (each backed by a CoDel queue) and visiting them in round-robin fashion.
Key changes:
- Refactored network simulation architecture with separate router and link components using FQ-CoDel queuing
- Moved latency configuration from link settings to the Simnet type level
- Added FlowBucketCount configuration to LinkSettings for customizing FQ-CoDel behavior
- Replaced synchronous error handling with panic/logging patterns for simplified control flow
Reviewed Changes
Copilot reviewed 19 out of 19 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| simnet.go | Refactored to use VariableLatencyRouter and moved LatencyFunc to Simnet struct |
| simlink.go | Complete rewrite implementing linkDriver with FQ-CoDel queuing |
| router.go | Added VariableLatencyRouter with heap-based packet scheduling and simplified error handling |
| fq_codel.go | New implementation of FQ-CoDel algorithm per RFC 8290 |
| codel.go | Simplified CoDel queue using ring buffer instead of heap |
| ringbuffer.go | New generic ring buffer data structure |
| ratelink.go | New rate limiting abstraction using golang.org/x/time/rate |
| packetheap.go | New min-heap for packet delivery scheduling |
| simconn.go | Updated to use PacketReceiver interface and removed Router.SendPacket |
| *_test.go | Updated tests for new API and added comprehensive FQ-CoDel tests |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| "time" | ||
| ) | ||
|
|
||
| func StaticLatency(duration time.Duration) func(*Packet) time.Duration { |
There was a problem hiding this comment.
NIT:
| func StaticLatency(duration time.Duration) func(*Packet) time.Duration { | |
| func SingleLatency(duration time.Duration) func(*Packet) time.Duration { |
There was a problem hiding this comment.
Why single? I think static makes more sense in the static vs dynamic dichotomy.
| tail *listNode[T] | ||
| } | ||
|
|
||
| func (l *linkedList[T]) append(item *listNode[T]) { |
There was a problem hiding this comment.
NIT: This is probably nicer:
| func (l *linkedList[T]) append(item *listNode[T]) { | |
| func (l *linkedList[T]) append(item T) { |
There was a problem hiding this comment.
I think that would cause a realloc.
There was a problem hiding this comment.
To explain a bit more, the common operation of removing the head and appending at the end would cause a realloc.
| return Packet{}, false | ||
| } | ||
|
|
||
| type linkedList[T any] struct { |
There was a problem hiding this comment.
Can a ring buffer work here? We want remove from front, move to back, seems like we can use a ringbuffer and works with slices.
There was a problem hiding this comment.
I considered this, but I think it gets a bit complicated when you only remove from the front. The linked list approach seems simple enough, and a remove front, append operation is just a couple of pointer changes.
| bytesRcvd atomic.Int64 | ||
|
|
||
| router Router | ||
| upPacketReceiver PacketReceiver |
There was a problem hiding this comment.
Is it better to use concrete type *Simlink here?
Same comment for the upPacketReceiver in Simlink, should that be a router?
There was a problem hiding this comment.
I think it's nice to use the generic interface type. That makes testing this easier as well. I'm not sure if there's much perf gain in using the concrete type.
I think it is also simpler to think about things as just being packet receivers rather than specific kinds of packet receivers.
Everything is a packet receiver. Simconn requires setting the up packet receiver explicitly
The burst doesn't make sense anymore. We have a codel queue to absorb bursts. The rate limiter should act as a simple rate-limited pipe.
|
merging as I think all comments have been addressed. Happy to change anything in a follow up PR |
Implements https://datatracker.ietf.org/doc/html/rfc8290. This implementation follows the RFC which in turn follows the linux implementation.
This is useful when there are many flows to/from a node (just a like a peer to peer setting), and attempts to introduce a notion of fairness to the packet scheduling.
At a very high level, flows are put into buckets, each bucket is backed by a codel queue. The buckets are visited in round-robin fashion.
The user facing API is mostly unchanged except you can now specify the flow bucket count in the LinkSettings (set to 1 for behavior similar to v0.0.3). The
LatencyFuncis not a field in theSimnettype instead of link settings as delays are done in the router.