add RED, ARED and PIE - #197
Conversation
un-lock-able
left a comment
There was a problem hiding this comment.
Please also remember to fix warnings from cargo clippy (in github actions) and run cargo fmt after modifications.
There was a problem hiding this comment.
Pull request overview
Adds three Active Queue Management (AQM) queue implementations (RED, Adaptive RED, and PIE) to rattan-core’s bandwidth cell queue module, enabling more realistic congestion signaling behavior than the existing tail/head drop queues.
Changes:
- Introduces new queue implementations:
RedQueue,AdaptiveRedQueue, andPieQueue, each with its own config struct and unit tests. - Exposes the new queues from
rattan-core/src/cells/bandwidth/queue/mod.rsso they can be used by the rest of the crate. - Adds algorithm-specific state tracking (avg queue length / drop probability / burst allowance / rate estimation) and hard-limit enforcement (packet/byte limits).
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| rattan-core/src/cells/bandwidth/queue/mod.rs | Registers and re-exports the new AQM queue modules. |
| rattan-core/src/cells/bandwidth/queue/red.rs | Adds RED queue implementation + tests. |
| rattan-core/src/cells/bandwidth/queue/ared.rs | Adds Adaptive RED (ARED) implementation + tests. |
| rattan-core/src/cells/bandwidth/queue/pie.rs | Adds PIE implementation + tests. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Lethe10137
left a comment
There was a problem hiding this comment.
Please refer to #155 about the "logical timestamp". Conceptually these time-based queue should try to avoid calling Instant::now(), but refer to the timestamp of the packets, which is when the packet "should" be here.
|
I have finished modifying the code for RED, Adaptive RED, and PIE, and have strictly aligned the logic with both the RFC specifications and the Linux kernel implementation. There are currently one remaining point that need confirmation:
|
|
cc @un-lock-able @Lethe10137 @EwecaBcD Please reply to the remaining questions to confirm the design. |
The And this was originally introduced by @EwecaBcD . |
|
@CepheusC Please rebase onto the latest main branch so that the changes will only be in |
…c on invalid config
… RED/PIE in factory enums
Centaurus99
left a comment
There was a problem hiding this comment.
Overall this is a clean, well-tested PR: it compiles cleanly and all RED/PIE unit tests pass. Requesting changes for three defects around the new seed handling and the adaptive-RED update timing.
configure()ignores a changedseed(the RNG is never re-seeded) in both RED and PIE.seedcannot round-trip through serde:seed: 0is dropped on serialize and reads back as 42.- Adaptive RED resets its update reference to the arrival time instead of a fixed 500 ms grid, so the adjustment interval drifts and idle gaps are not caught up (Linux
sch_red.cuses a fixed periodic timer).
Details inline.
Centaurus99
left a comment
There was a problem hiding this comment.
LGTM. @BobAnkh to make the final decision.
BobAnkh
left a comment
There was a problem hiding this comment.
Basically, I agree on the implementation. However, several things need to be checked and confirmed:
- Check the difference of the implementation (I assume you implement it according to the RFC) with kernel's. If any, document them.
- For the naming of the fileds in both queue config struct, I don't know if they are the same as what kernel calls them. If they are different, we should also document that (saying what in the kernel that field corresponds to in documentation comments)
- Since, both PIE queue and RED queue have complex configurations (than our normal queues), we should add documentation comments to both Config struct. Besides that, we can also leave some documentation comments, including the outer documentation comments used at the top of a file or module
cc @Centaurus99 Please also supervise on these things.
…tation with kernel
BobAnkh
left a comment
There was a problem hiding this comment.
I'm fine on the docs, and wait for @Centaurus99 to see the difference and squash-merge the PR
The issues raised in this review have been resolved, and the review is now outdated.
Design Choice
RED and Adaptive RED
adaptivefield inRedQueueConfig.pkt_tx_timeis used to compute the number of "virtual packet departures" during an idle period for average queue length decay:m = idle_time_us / pkt_tx_time. The upper layer computes this from link bandwidthCand average packet sizeavpktaspkt_tx_time = avpkt * 8 / C, then passes it down as a fixed config.PIE
The standard PIE algorithm (RFC 8033) updates the drop probability
pat a fixed intervalt_update(default 15ms). Rather than spawning a dedicated periodic timer, this implementation drives the update fromenqueue()by catching up on all missed update intervals using the packet's logical timestamp. The rationale is threefold:PacketQueuetrait defines a generic interface that all AQM algorithms implement. Inserting a timer intoPieQueuewould requireArc<Mutex<>>synchronization with the caller's task; placing the timer in the upper-layer loop (e.g.,BwCell) would couple it to PIE-specific logic and break composability.Packet::get_timestamp()) rather than wall-clock time. Using the packet's timestamp ensures thatupdate_drop_probability()sees the correct time delta regardless of how the simulation is paced relative to real time.t_update = 15msand typical packet inter-arrival times on the order of microseconds, at least oneenqueue()call falls within every update interval under load. Thewhileloop inenqueue()replays all missedupdate_drop_probability()calls in bulk, producing exactly the same state as a timer-driven implementation.