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

kernel: ring_buffer: remove volatile reads #991

Merged
merged 1 commit into from Jun 19, 2018

Conversation

Projects
None yet
4 participants
@bradjc
Copy link
Contributor

bradjc commented Jun 13, 2018

Pull Request Overview

This removes the need for unsafe in ring_buffer.rs, and it isn't clear why the reads needed to be volatile.

Testing Strategy

This pull request was tested by running the hail app.

TODO or Help Wanted

Anyone know if those volatile reads and writes are still useful?

Documentation Updated

  • Kernel: Updated the relevant files in /docs, or no updates are required.
  • Userland: Added/updated the application README, if needed.

Formatting

  • Ran make formatall.
kernel: ring_buffer: remove volatile reads
This removes the need for unsafe, and it isn't clear why these needed to
be volatile.
@phil-levis
Copy link
Collaborator

phil-levis left a comment

The volatile reads/writes are needed because the variables (well, tail) are modified in interrupt context. I.e., with inlining, it's possible that a loop on has_elements might read the variables once and then either return or loop forever.

Or is there some way the compiler knows enqueue might be called preemptively?

@brghena

This comment has been minimized.

Copy link
Contributor

brghena commented Jun 14, 2018

@phil-levis I don't think we're ever accessing ring_buffers from an interrupt context anymore. The only use of the ring buffer I found is for process tasks, entirely within process.rs (which is non-interrupt context).

@alevy

This comment has been minimized.

Copy link
Member

alevy commented Jun 16, 2018

@brghena is correct. We stopped using ring buffers for interrupts pre-1.0 and now the ring buffer is only used in the main kernel thread (e.g. for process callbacks). I think it's probably good practice to separate data structures that we use and provide for "normal" kernel code, vs those that we use in special cases like interrupt contexts (currently none).

@alevy

alevy approved these changes Jun 16, 2018

@alevy alevy added the P-Upkeep label Jun 16, 2018

@brghena and @alevy answered concern

@alevy alevy merged commit 856403e into master Jun 19, 2018

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
deploy/netlify Deploy preview ready!
Details

@alevy alevy deleted the kernel-ring-buffer-no-unsafe branch Jun 19, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment