Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

bandwidth: introduce configurable queue length #1025

Conversation

middaywords
Copy link

Currently, the bandwidth plugin uses tc tbf and set default latency to 25ms, which easily causes Head-Of-Line blocking and bufferbloat. This commit proposes to make it a configurable value.

Currently, the bandwidth plugin uses tc tbf and set default latency
to 25ms, which easily causes Head-Of-Line blocking and bufferbloat. This
commit proposes to make it a configurable value.

Signed-off-by: Kangjie Xu <xkjrst@outlook.com>
@s1061123
Copy link
Contributor

s1061123 commented Apr 7, 2024

@middaywords thank you for the PR. But I suppose that it could be implemented by another CNI plugin as different purpose.
Currently bandwidth has lots of features and code and your feature is different (i.e. bandwidth is slightly different from latency, even though tc is used). bandwidth is not 'perfect swiss army knife of TC CNI plugin', just for 'bandwidth'. Having another plugin makes code simple and feature centric (and have benefit as 'chained CNI' mechanism!).

@middaywords middaywords changed the title bandwidth: introduce configurable latency bandwidth: introduce configurable queue length Apr 7, 2024
@middaywords
Copy link
Author

middaywords commented Apr 7, 2024

@s1061123 the latency in title may be misleading, it means queue depth in tc tbf, i've changed it.
The key problem here is that, the bandwidth plugin uses a deep queue by default, and it's not configurable, which causes head-of-line blocking.
Specifically, when using the bandwidth plugin to limit the bandwidth, it brings extra latency about tens of milliseconds, which means it cannot go into real production env.

@middaywords
Copy link
Author

middaywords commented Apr 7, 2024

here is the test results when I test the plugin bandwidth using netperf, in local kind env. ping latency of server and client is about 50us, but the measured latency with traffic is about >10ms.
So I'd suggest to make the queue length configurable to reduce the latency.

Data flow direction Throughput(Mbps) for TCP_STREAM flow Latency(us) for TCP_RR flow
netperf-client -> netperf-server (egress rate limit) 95.6 10760.05
netperf-server -> netperf-client (ingress rate limit) 95.6 226120.05

@s1061123
Copy link
Contributor

s1061123 commented Apr 8, 2024

Understand your comment. BTW, we'll merge this PR soon and you must need to rebase (it is pretty big).

#921

So please check the repo tomorrow and please rebase once repo is updated with above PR. Appreciated your help.

@middaywords
Copy link
Author

middaywords commented Apr 14, 2024

#921 uses HTB class for QoS and rate limiting, it can avoid close Head-Of-LIne blocking using many different subclasses, and its queue length for traffic shaping is not configurable.
Close this PR because it's no longer needed in HTB case

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants