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 ingress thread deadlock for resubmit packets #730

Conversation

antoninbas
Copy link
Member

Resubmit packets were written to the input_buffer with a blocking call
by the ingress thread. Since the ingress thread is also in charge of
draining the input_buffer, this could have lead to a deadlock. Resubmit
packets can now be dropped if the buffer is full. To limit the number of
resubmit packets being lost, we place them is a higher priority queue
than "normal" packets.

Fixes #729

Resubmit packets were written to the input_buffer with a blocking call
by the ingress thread. Since the ingress thread is also in charge of
draining the input_buffer, this could have lead to a deadlock. Resubmit
packets can now be dropped if the buffer is full. To limit the number of
resubmit packets being lost, we place them is a higher priority queue
than "normal" packets.

Fixes #729
@antoninbas antoninbas force-pushed the antonin/simple-switch-fix-ingress-thread-deadlock-for-resubmit-packets branch from e373de4 to 1e650aa Compare March 7, 2019 17:38
//! As for QueueingLogicRL, the write behavior (push_front()) is blocking: once
//! a logical queue is full, subsequent incoming elements will be dropped until
//! the queue starts draining again.
//! As for QueueingLogicRL, the write behavior (push_front()) is not blocking:
Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry, I do not know the BMv2 internals very well at all, so just throwing up a random red flag on comment changes -- After your changes is the phrase "As for QueueingLogicRL" still correct?

Copy link
Member Author

Choose a reason for hiding this comment

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

I didn't change anything in this file. I realized this comment was not correct so I fixed it, but it has nothing to do with the rest of the changes.

@codecov-io
Copy link

Codecov Report

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

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #730      +/-   ##
==========================================
+ Coverage   74.45%   74.46%   +0.01%     
==========================================
  Files         115      115              
  Lines        9970     9970              
==========================================
+ Hits         7423     7424       +1     
+ Misses       2547     2546       -1
Impacted Files Coverage Δ
src/bm_sim/learning.cpp 82.17% <0%> (+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 ca27b9f...1e650aa. Read the comment docs.

@antoninbas antoninbas merged commit edce7af into master Mar 11, 2019
@antoninbas antoninbas deleted the antonin/simple-switch-fix-ingress-thread-deadlock-for-resubmit-packets branch March 11, 2019 22:18
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.

3 participants