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

host: Fix RFNoC graph action queue lockup on action exceptions #730

Conversation

hannodewind
Copy link
Contributor

Pull Request Details

Description

Processing of the action queue gets locked up when any action being executed in the send_action call throws an exception. Exceptions are not caught in the loop handling the action queue, resulting in the handling_ongoing queue locking flag to never be released. Any subsequent call to enqueue_action will return on the early exit with the assumption that we're already handling the actions, yet the previous handler exited with an exception.

This fix uses a RAII wrapper rather than a manually claimed and released atomic flag to ensure that the handling_ongoing will be released even under exceptional conditions.

Related Issue

Relates to issue #611

Which devices/areas does this affect?

UHD hosts using RFNoC graph

Testing Done

X310 with dual 10GbE links to server, running both RF inputs at 200MHz sample rate using 2x RX streamers.
Stress the server with CPU load (can use stress-ng), inducing UDP packet drops. (Also relates to #611, which stressed the link using iperf, probably also causing UDP packet drops).
At some point (difficult to reproduce, but does happen every so often), one of the RX streamers will experience an overrun, which calls the _overrun_handler -> ACTION_KEY_RX_RESTART_REQ which calls get_time_now(), doing a peek64 to the device. This peek64 then throws an exception due to an ACK timeout.

This exception is caught all the way up in thread that called recv on the RX streamer, but the stream is irrecoverable since the graph action queue is locked up.

Checklist

  • I have read the CONTRIBUTING document.
  • My code follows the code style of this project. See CODING.md.
  • I have updated the documentation accordingly.
  • I have added tests to cover my changes, and all previous tests pass.
  • I have checked all compat numbers if they need updating (FPGA compat,
    MPM compat, noc_shell, specific RFNoC block, ...)

Copy link

github-actions bot commented Feb 19, 2024

CLA Assistant Lite bot All contributors have signed the CLA ✍️ ✅

@hannodewind
Copy link
Contributor Author

I have read the CLA Document and I hereby sign the CLA

@hannodewind
Copy link
Contributor Author

recheck

Processing of the action queue gets locked up when any action
being executed in the send_action call throws an exception.
Exceptions are not caught in the loop handling the action queue,
resulting in the handling_ongoing queue locking flag to never
be released. Any subsequent call to enqueue_action will return
on the early exit with the assumption that we're already handling
the actions, yet the previous handler exited with an exception.

This fix uses a RAII wrapper rather than a manually claimed and
released atomic flag to ensure that the handling_ongoing will be
released even under exceptional conditions.
@hannodewind hannodewind force-pushed the bugfix/host/rfnoc_graph_action_lockup branch from 42a1756 to 0d68db5 Compare February 19, 2024 10:24
@hannodewind
Copy link
Contributor Author

recheck

@mbr0wn
Copy link
Contributor

mbr0wn commented Feb 29, 2024

@hannodewind Don't worry about the CLA checker bot, it's a misconfig on our end (I think).

This is all we need for now:

image

Copy link
Contributor

@mbr0wn mbr0wn left a comment

Choose a reason for hiding this comment

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

@hannodewind your analysis is correct, and the solution is fine, too. I do want to double-check if we can have the atomic lock without adding this special-case class, but overall, this is an excellent solution.

@mbr0wn
Copy link
Contributor

mbr0wn commented Apr 17, 2024

@hannodewind I think I will modify this to use UHD's scope_exit instead of your bespoke class, but otherwise, this is a fantastic change. Kudos to figuring out the issue, and for modifying in such a great way so far inside its guts!

@hannodewind
Copy link
Contributor Author

@mbr0wn Thank you for the feedback, I am keen to see the scope_exit implementation. Let me know if there is anything else I can/should do on this PR, always happy to assist!

@joergho joergho closed this Apr 29, 2024
@github-actions github-actions bot locked and limited conversation to collaborators Apr 29, 2024
@joergho
Copy link
Contributor

joergho commented Apr 29, 2024

The change is now in master: 0f2007f

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

Successfully merging this pull request may close these issues.

3 participants