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

#0: Use CV to wait for cq_reader in production mode. Remove enqueue_record_event for NB calls #9793

Merged
merged 1 commit into from
Jun 29, 2024

Conversation

tt-asaigal
Copy link
Contributor

@tt-asaigal tt-asaigal commented Jun 27, 2024

Ticket

Link to Github Issue

Problem description

Issuing a record_event when making non-blocking calls leads to bad perf in async mode, since the Completion Queue Reader and Worker Threads are bound to the same CPU core.

Similarly, a while loop in finish (polling the Completion Queue Reader status) is bad, due to worker polling CQ reader and CQ reader polling device.

What's changed

Remove record_event calls, since they're not needed for functionality.

Have CQ reader poll device and send an interrupt to worker thread, using condition vars. No more double polling, and worker thread yields CPU while waiting.

Checklist

  • Post commit CI passes
  • Model regression CI testing passes (if applicable)
  • New/Existing tests provide coverage for changes

@tt-asaigal tt-asaigal requested a review from pgkeller as a code owner June 27, 2024 22:12
@tt-asaigal tt-asaigal merged commit 852e657 into main Jun 29, 2024
84 checks passed
@tt-asaigal tt-asaigal deleted the asaigal/cq_opts branch June 29, 2024 05:22
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