Adding delay cells that can reorder packets

[maxime-bruno]

Adds a new cell ReorderDelayCell.

This cell takes a delay generator. The packets are delayed in a way (priority queue) that allows potential reordering.

I added support for :

• Constant delay
• Normal Law based delay
• Log-Normal Law based delay

It can be a potential solution for issue #32

I also added benchmarks to test the bandwidth, delay, and reorder delay cell.

[BobAnkh]

@maxime-bruno We really appreciate your contributions and your overall design! However, there remain some points to be discussed besides the changes requested:

1. Naming conventions: Could you please rename the reorder_delay or ReorderDelay just to reorder or Reorder for simplicity? For our design choice to choose delay as our trace definition, we can add some words in documentations. Additionally, could you please rename the DelayGenerator trait to ReorderTrace and its method to next_delay to ensure compatibility with other trace traits in our crate?
2. Could you please remove the file .pre-commit-config.yaml?
3. For the DelayGenerator (or ReorderTrace as I expected) trait (and its specific models, normal or log normal), I will move it out of this crate to crate netem-trace in the near future, as we did with other cells for decoupling. You can also do this directly by sending a PR to that codebase if you prefer.
4. Could you please add a demo configuration section to the file config.example.toml?
5. Could you please add a test for correctness verification to tests/integration, similar to what we did for other cells?
6. For the definition of ReorderCellBuildConfig, I do think we can change it to be similar to DelayReplayCellBuildConfig. In this way, we can easily extend reorder trace with more models beside normal or log normal in other crates (i.e., netem-trace) without modifying rattan. If you want to preserve some of them for simpler configuration, you can also add a new ReorderReplayCellBuildConfig struct.

[maxime-bruno]

Hi,

It seems that most of your requests are pretty logical, I just have a few points:

1. I haven't understood how replay cells are supposed to work, that's why I didn't implement one. Because I guess, at some point the delay should then be deterministic, which is not the case here
2. The name ReorderDelayCell was due to the fact that it is mostly a delay cell (that can perform reordering), and technically could replace the DelayCell using just a constant delay generator. And one may want to implement a cell that just really reorders packets in the future

[BobAnkh]

Hi,

For your concerns:

I haven't understood how replay cells are supposed to work, that's why I didn't implement one. Because I guess, at some point the delay should then be deterministic, which is not the case here

I can explain more details to you. For all such cells, it will replay a so-called trace (e.g., BwTrace, DelayTrace, etc.), and in our case DelayGenerator (or ReorderTrace as I expected). The trace should give a deterministic sequence on a given input, but this doesn't mean the sequence cannot be randomly generated. The output sequence can be subject to some distributions. For example, for bandwidth replay cell, it can replay bandwidth generated from a normal distribution (NormalizedBw and BwReplayCell). I forget to mention this in my reviews: you have to fix the seed or provide option to set the seed for the random-generated distribution. You can see how I implement this in NormalizedBw.

The name ReorderDelayCell was due to the fact that it is mostly a delay cell (that can perform reordering), and technically could replace the DelayCell using just a constant delay generator. And one may want to implement a cell that just really reorders packets in the future

While it may technically replace the DelayCell, it cannot replace the complicated DelayReplayCell. But I agree with you that we can leave its name as ReorderDelayCell, as we may have other reordering implementations in the future. But for the folder or file name, I think we can shrink it to reorder.

Feel free to let me know if you have any further thoughts or concerns. I can also assist with some of the implementations.

[maxime-bruno]

So the replay cell would be a cell with a set of laws with their seed and at which moment switching from one to another ?

[BobAnkh]

In detail, the replay cell is a cell that contains a trait object, which produces a sequence. As you can see the Option<Box<dyn BwTraceConfig>> in the BwReplayCellConfig:

pub struct BwReplayCellConfig<P, Q>
where
    P: Packet,
    Q: PacketQueue<P>,
{
    pub trace_config: Option<Box<dyn BwTraceConfig>>,
    pub queue_config: Option<Q::Config>,
    pub bw_type: Option<BwType>,
}

or the trace: Box<dyn BwTrace> in BwReplayCellEgress:

pub struct BwReplayCellEgress<P, Q>
where
    P: Packet,
    Q: PacketQueue<P>,
{
    egress: mpsc::UnboundedReceiver<P>,
    bw_type: BwType,
    trace: Box<dyn BwTrace>,
    packet_queue: Q,
    current_bandwidth: Bandwidth,
    next_available: Instant,
    next_change: Instant,
    config_rx: mpsc::UnboundedReceiver<BwReplayCellConfig<P, Q>>,
    send_timer: Timer,
    change_timer: Timer,
    state: AtomicI32,
}

So that we have different models implementing BwTrace (e.g., NormalizedBw, similar to your NormalLawDelayGenerator. We can even have a more complex combination of models), we can then use the same cell to replay them, whether it is Normal or LogNormal. I believe this is the same as your design and implementation of ReorderDelayCell. So the ReorderDelayCell is indeed similar to other replay cells. That's why I think we can change the definition of ReorderCellBuildConfig to be the same as other config structs of replay cells.

For an example usage, see tests/integration/bandwidth.rs#L773:

let trace = RepeatedBwPatternConfig::new()
    .pattern(vec![Box::new(StaticBwConfig {
        bw: Some(Bandwidth::from_mbps(1)),
        duration: Some(Duration::from_secs(10)),
    }) as Box<dyn BwTraceConfig>])
    .build();
BwReplayCell::new(
    Box::new(trace) as Box<dyn BwTrace>,
    DropTailQueue::new(DropTailQueueConfig::new(100, None, BwType::default())),
    None,
)

[BobAnkh]

One more thing to add: the definition of BwReplayCellBuildConfig is an enum while DelayReplayCellBuildConfig is a struct is because the limited choices of queue types are inside rattan, and are not a part of the bandwidth traces. The models of generating traces (i.e., a sequence of delays to reorder packets) are left to netem-trace, where we define trace traits BwTrace or ReorderTrace (DelayGenerator) and a lot of models (normal, log normal, etc.).

[maxime-bruno]

So a proper implementation of what I called DelayGenerator would be

/// This is a trait that represents a trace of per-packet delays.
///
/// The trace is a sequence of `delay`.
/// The delay describes how long the packet is delayed when going through.
///
/// For example, if the sequence is [10ms, 20ms, 30ms],
/// then the delay will be 10ms for the first packet, then 20ms for second, then 30ms for third.
///
/// The next_delay function either returns **the next delay**
/// in the sequence, or **None** if the trace goes to end.
pub trait DelayPerPacketTrace: Send {
    fn next_delay(&mut self) -> Option<Delay>;
}

DelayPerPacket seems more pertinent than Reorder and follows the guidelines (if I'm right)

[BobAnkh]

Yes, I agree entirely with your design and implementation. It's just a matter of whether to call it DelayPerPacketTrace or ReorderDelayTrace, which you can decide on your own.

[BobAnkh]

I have published a new version of netem-trace as v0.4.2.

[maxime-bruno]

As requested I made several changes (most of the requested ones):

• I moved the config to netem-trace, currently it links against my fork (I'm doing minor modifications before committing them to your repo)
• I change the name to DelayPerPacketCell, that way it matches with the config name
• I moved everything to a per_packet module, that way it is ready if someone wants to implement other cells that work on a per packet configuration (for example for loss)

To be done:

• writing the integration tests
• writing some doc (I think)
• closing the pr for human_bandwidth Adding support for binary prefix system human-bandwidth#4 and netem-trace (not opened yet)

[maxime-bruno]

So normally I'm finished, I just have to make a PR for netem-trace

I'm waiting for your reviews before making the PR

[BobAnkh]

Most of the changes are great! In addition to the requested changes, here are some requirements regarding cleaning up the commit history that you can refer to and modify before the final merge:

• Can you avoid modifying the svg figures inside assets? This makes the PR a little messy.
• And could you seperate the style format of files（e.g., devcontainer.json, .sockerignore, .github/workflows, ci/images) into a single commit in this PR so that I can easily maintain a clear git history?
• If you want to squash the commits in the PR (into two, one main feature and the other formatting) according to our commit conventions before merging, you can wait for my final commit message requirements. If you prefer me to handle it directly, you can change the target branch to the dev branch I newly created, and I will rebase after merging this PR.

[maxime-bruno]

Seems doable

The modification to assets and things like that are just trailing whitespace and missing new lines, I'll make a clean commit for that

For squashing the commits, I'll let you handle it to be sure it has the names you want, or if you need me to split commits, for features separation, I'll do it

[BobAnkh]

The modification to assets and things like that are just trailing whitespace and missing new lines, I'll make a clean commit for that

Then it's fine to include them in this PR as a separate commit.

For squashing the commits, I'll let you handle it to be sure it has the names you want, or if you need me to split commits, for features separation, I'll do it

You can commit whatever you want for now, and we will handle this just before the merging process. For the final history, I expect there to be two squashed commits, one for all the features and the other for the style format (i.e., the trailing whitespaces or new lines, etc.). I will provide you with the commit message I need for the two commits before the merge, and you can assist me in rebasing the commit history so that I can merge them directly into the main branch.

[maxime-bruno]

Hi,

Normally everything is done, I just have to push a PR for netem-trace once you are ok with everything and then it should be able to be merged

[BobAnkh]

LGTM

[maxime-bruno]

Normally everything is ready

[maxime-bruno]

I did not test this part and just noticed this issue

The fact that the DelayPerPacketCellBuildConfig is an enum, the variant starts with a capital letter and that the TOML keyword is not capitalized does not work

[BobAnkh]

I did not test this part and just noticed this issue

The fact that the DelayPerPacketCellBuildConfig is an enum, the variant starts with a capital letter and that the TOML keyword is not capitalized does not work

Ok, can you send me a new PR to fix this issue? I think we may have some serde option to change this behavior (maybe something similar to rename_all_fileds = snake_case, I think)?