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

Fix undesired reordering in queueing logic #903

Merged

Conversation

antoninbas
Copy link
Member

C++ clocks can return the same value in consecutive calls, which was
causing potential reordering in the queueing logic, in particular for
rate-limited queues when the pps rate was set to 0.

This was causing some unit tests to fail.

Fixes #900

C++ clocks can return the same value in consecutive calls, which was
causing potential reordering in the queueing logic, in particular for
rate-limited queues when the pps rate was set to 0.

This was causing some unit tests to fail.

Fixes #900
@codecov-commenter
Copy link

codecov-commenter commented Jun 2, 2020

Codecov Report

Merging #903 into master will increase coverage by 0.00%.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #903   +/-   ##
=======================================
  Coverage   74.31%   74.32%           
=======================================
  Files         115      115           
  Lines       10243    10243           
=======================================
+ Hits         7612     7613    +1     
+ Misses       2631     2630    -1     
Impacted Files Coverage Δ
src/bm_sim/learning.cpp 82.06% <0.00%> (+0.38%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 00b8e4a...605e788. Read the comment docs.

Copy link
Contributor

@jafingerhut jafingerhut left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me. Nice catch figuring out what was going wrong here. I guess there is no practical need to deal with wrapping in time values or wrapping_counter values in BMv2, since we never expect it to run that long? You can imagine how much fun hardware folks have dealing with small values that do wrap. :-)

@antoninbas
Copy link
Member Author

Even if the counter wraps, it may just create packet re-ordering for a very small time window, which is still strictly better than the current implementation. BTW even now, packet re-ordering is very uncommon. It seems to happen more frequently on some machines or depending on how bmv2 is run (e.g. in a docker container).

@antoninbas antoninbas merged commit 77cc1a3 into master Jun 2, 2020
@antoninbas antoninbas deleted the antonin/fix-undesired-reordering-in-queueing-logic branch June 2, 2020 18:49
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.

make check: FAIL: test_queueing
3 participants