Improve congestion control #1627

Mallets · 2024-12-04T12:48:13Z

This PR introduces:

congestion detection in the transmission pipeline per priority queue
early dropping in case congestion is detected bypassing the wait_before_drop timeout

Early dropping drastically helps in the case of having a slow subscriber in the system, e.g.:

                       |-> | fast subscriber |
| pub | -> | router | -|
                       |-> | slow subscriber |

That means a slow subscriber will have a minima impact on the system when CongestionControl::Drop is used.
No change in behaviour is introduced in the case of using CongestionControl::Block.

Validation steps

Set CongestionControl::Block in the z_pub_thr example.

Modify the subscriber callback to block for some time in the z_sub example.

while let Ok(sample) = subscriber.recv_async().await {
    tokio::time::sleep(Duration::from_millis(100)).await;
}

Main

Run the router:

./target/release/zenohd

Run throughput publisher:

./target/release/examples/z_pub_thr 8 -m client

Run slow subscriber:

./target/release/examples/z_sub -m client -k test/thr

Run throughput subscriber:

./target/release/examples/z_sub_thr -m client -n 500 -s 1000
Press CTRL-C to quit...
774.8841943707797 msg/s
779.9427331103915 msg/s
775.5408884676764 msg/s
781.3848084140717 msg/s
782.5382842182961 msg/s
781.2933373648069 msg/s
781.5737730415275 msg/s

This PR

Run the router:

./target/release/zenohd

Run throughput publisher:

./target/release/examples/z_pub_thr 8 -m client

Run slow subscriber:

./target/release/examples/z_sub -m client -k test/thr

Run throughput subscriber:

./target/release/examples/z_sub_thr -m client -n 5000000 -s 1000
Press CTRL-C to quit...
3053345.844926824 msg/s
3126748.454285023 msg/s
3077751.70238141 msg/s
3115776.8953388007 msg/s
3103958.315081412 msg/s
3127926.9733049045 msg/s
3104865.110821202 msg/s

yellowhatter · 2024-12-04T13:58:30Z

io/zenoh-transport/src/common/pipeline.rs

+    // The whole pipeline is enabled or disabled
+    disabled: AtomicBool,
+    // Bitflags to indicate the given priority queue is congested
+    congested: AtomicU8,


Discussional: in order to reduce contention around this atomic, I'd recommend switching to a set of separate AtomicBool and make 64-byte padding (maybe indirect by putting them into StageIn etc). The cost of operation for multi-(thread,core,priority) setup should be smaller.

I thought about that but increasing the number of atomics also increases the amount of data to be kept in cache. Having one atomic per priority will significantly put more burden on moving data across different cache level I assume, thus reducing performance in any case. I don't have an experimental comparison but I'm not expecting a lot of contention in general.

yellowhatter · 2024-12-04T15:08:40Z

There is also some systematic throughput regression on my Linux laptop:

Ref:

This PR:

yellowhatter · 2024-12-04T15:18:39Z

The bench is using CC::Block, so I suppose we are having smth with data structure sizes, not with atomics...

Mallets · 2024-12-04T15:27:30Z

Interesting, on my laptop I've actually gained ~500K msg/s with 8 bytes payload with this PR...

yellowhatter · 2024-12-04T15:42:46Z

Interesting, on my laptop I've actually gained ~500K msg/s with 8 bytes payload with this PR...

I will retest it!

Mallets · 2024-12-04T15:51:25Z

I have to say that the gain in performance I observe on my laptop is not justified either... Nothing on this PR should provide better performance on the nominal case.

yellowhatter · 2024-12-04T16:45:13Z

I have to say that the gain in performance I observe on my laptop is not justified either... Nothing on this PR should provide better performance on the nominal case.

So, after reboot I've got results equal to the reference!

yellowhatter

A really great thing for slow subscriber problem!

eclipse-zenoh#1627 introduced a congestion shortcut mechanism, but this one was not correctly synchronized. There was indeed situations experienced by users in which congested flag was set and never reset, which implies a drop of all successive messages (the publisher becomes kind of dead). The congestion flag is in fact set after the deadline of a message is reached, while it is unset when batches were refilled, only with relaxed atomic operations. Also, after the deadline is reached, there is no further check of the queue. The most obvious synchronization flow here is that between the instant where the thread is waken up because the deadline has been reached, and the one where the congested flag is set, it is possible that the tx task is unblocked and all the batches are sent and refilled. The congested flags would been set after, so there would be no batch to refill to unset it back. This flow seems hard to achieve when there are many batches in the queue, but it is still theoretically possible. And when fragmentation chain is dropped in the middle, pushing the ephemeral batch takes more time before setting the congested flag; this is precisely the case where the bug was observed by the way, so it may indicate the described flow is the reason behind, but it's not sure. The proposed fix reverts eclipse-zenoh#1627, and use a simpler and correctly synchronized shortcut: a congested flag is added behind the mutex of `StageIn`, it is set when the pipeline is congested and unset if the message is written (for both network and transport); if the congested flag is set, the deadline is not awaited. This shortcut is not "as short" as the previous, but it is again a lot simpler.

eclipse-zenoh#1627 introduced a congestion shortcut mechanism, but this one was not correctly synchronized. There was indeed situations experienced by users in which congested flag was set and never reset, which implies a drop of all successive messages (the publisher becomes kind of dead). The congestion flag is in fact set after the deadline of a message is reached, while it is unset when batches were refilled, only with relaxed atomic operations. Also, after the deadline is reached, there is no further check of the queue. The most obvious synchronization flow here is that between the instant where the thread is waken up because the deadline has been reached, and the one where the congested flag is set, it is possible that the tx task is unblocked and all the batches are sent and refilled. The congested flags would been set after, so there would be no batch to refill to unset it back. This flow seems hard to achieve when there are many batches in the queue, but it is still theoretically possible. And when fragmentation chain is dropped in the middle, pushing the ephemeral batch takes more time before setting the congested flag; this is precisely the case where the bug was observed by the way, so it may indicate the described flow is the reason behind, but it's not sure. The proposed fix adds a additional synchronization step after setting the congested flag: we retry to push the message, this time with an already expired deadline. If the batches were refilled, the message should be able to be sent and the congestion flag will be reset. If batches were in fact refilled, but the message was fragmented and still ends by being dropped, it still means batches have been pushed and will be refilled later, still unsetting the congested flag.

* fix: fix pipeline congestion shortcut synchronization #1627 introduced a congestion shortcut mechanism, but this one was not correctly synchronized. There was indeed situations experienced by users in which congested flag was set and never reset, which implies a drop of all successive messages (the publisher becomes kind of dead). The congestion flag is in fact set after the deadline of a message is reached, while it is unset when batches were refilled, only with relaxed atomic operations. Also, after the deadline is reached, there is no further check of the queue. The most obvious synchronization flow here is that between the instant where the thread is waken up because the deadline has been reached, and the one where the congested flag is set, it is possible that the tx task is unblocked and all the batches are sent and refilled. The congested flags would been set after, so there would be no batch to refill to unset it back. This flow seems hard to achieve when there are many batches in the queue, but it is still theoretically possible. And when fragmentation chain is dropped in the middle, pushing the ephemeral batch takes more time before setting the congested flag; this is precisely the case where the bug was observed by the way, so it may indicate the described flow is the reason behind, but it's not sure. The proposed fix adds a additional synchronization step after setting the congested flag: we retry to push the message, this time with an already expired deadline. If the batches were refilled, the message should be able to be sent and the congestion flag will be reset. If batches were in fact refilled, but the message was fragmented and still ends by being dropped, it still means batches have been pushed and will be refilled later, still unsetting the congested flag. * Fix typo * fix: typo --------- Co-authored-by: Oussama Teffahi <70609372+oteffahi@users.noreply.github.com>

Mallets added 4 commits December 3, 2024 16:45

Perform early dropping

0836fcb

Example slow subscriber

528ec49

TransmissionPipelineStatus

30341ff

Restore examples

bceabb1

Mallets added the enhancement Existing things could work better label Dec 4, 2024

Mallets requested a review from yellowhatter December 4, 2024 13:03

Mallets added 3 commits December 4, 2024 14:05

Use set_congested function

5b24ff5

Fix Priority usage in TransmissionPipeline API

7e66014

Reset examples

94b06be

Mallets marked this pull request as ready for review December 4, 2024 13:09

yellowhatter reviewed Dec 4, 2024

View reviewed changes

yellowhatter approved these changes Dec 4, 2024

View reviewed changes

Mallets merged commit b8a2979 into main Dec 4, 2024
24 checks passed

Mallets deleted the fix/congestion branch December 5, 2024 10:49

YuanYuYuan mentioned this pull request Dec 18, 2024

Bump up zenoh version ros2/rmw_zenoh#347

Merged

wyfo added a commit to ZettaScaleLabs/zenoh that referenced this pull request Jan 23, 2025

fix: revert eclipse-zenoh#1627 and add congestion shortcut

740cdeb

wyfo mentioned this pull request Jan 23, 2025

fix: revert #1627 and add congestion shortcut #1737

Closed

wyfo mentioned this pull request Jan 24, 2025

fix: fix pipeline congestion shortcut synchronization #1740

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve congestion control #1627

Improve congestion control #1627

Mallets commented Dec 4, 2024 •

edited

Loading

yellowhatter Dec 4, 2024

Mallets Dec 4, 2024

yellowhatter commented Dec 4, 2024 •

edited

Loading

yellowhatter commented Dec 4, 2024

Mallets commented Dec 4, 2024 •

edited

Loading

yellowhatter commented Dec 4, 2024

Mallets commented Dec 4, 2024

yellowhatter commented Dec 4, 2024

yellowhatter left a comment

Improve congestion control #1627

Improve congestion control #1627

Conversation

Mallets commented Dec 4, 2024 • edited Loading

Validation steps

Main

This PR

yellowhatter Dec 4, 2024

Choose a reason for hiding this comment

Mallets Dec 4, 2024

Choose a reason for hiding this comment

yellowhatter commented Dec 4, 2024 • edited Loading

yellowhatter commented Dec 4, 2024

Mallets commented Dec 4, 2024 • edited Loading

yellowhatter commented Dec 4, 2024

Mallets commented Dec 4, 2024

yellowhatter commented Dec 4, 2024

yellowhatter left a comment

Choose a reason for hiding this comment

Mallets commented Dec 4, 2024 •

edited

Loading

yellowhatter commented Dec 4, 2024 •

edited

Loading

Mallets commented Dec 4, 2024 •

edited

Loading