-
Notifications
You must be signed in to change notification settings - Fork 82
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
Enable RCU #752
base: main
Are you sure you want to change the base?
Enable RCU #752
Conversation
@LeslieKid Really excited to see this PR. Good job on submitting a working RCU 👍 Here is some initial feedbacks. I will add more when I have the time to look into the detail.
|
@tatetian Thanks for your feedbacks! I tried to use the RCU for the ramfs and it seems to run correctly. Do I need to create a new branch and then pull request? Or just commit to this branch? |
#748 is getting merged. So you should be able to add unit tests for RCU.
I am not sure whether ramfs is a good user or not. Does Linux use RCU for ramfs? If you have good confidence, submit the changes and we shall see.
The size of this PR is well under control. It won't be a problem to have more commits added to it. |
/// # Non-Preemptible RCU | ||
/// | ||
/// In non-preemptible RCU, the read-side critical section is delimited using | ||
/// a `PreemptGuard`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Our current implementation is non-preemptible RCU. To avoid repeating "Non-Preemptible RCU" in each method, I think it should be moved to the type-level Rust doc.
/// This method returns a `RcuReadGuard` which allows read-only access to the | ||
/// underlying data protected by the RCU mechanism. | ||
/// | ||
/// This function has the semantics of _subscribe_ in RCU mechanism. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You mention "publish" and "subscribe" in the Rust docs. But I can't see the value of mentioning "publish" and "sub scribe". Where do these terms come from? I don't think Linux use these terms.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These terms come from LWN. I think that the roles of get()
and replace()
can fit these terms. Should I delete it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mentioning the two versions only complicates things and confuses the reader. Decide which one is better: get
/replace
or publish
/subscribe
. Choose one version and stick with it.
@@ -56,6 +59,9 @@ pub(crate) fn get_idle_task_cx_ptr() -> *mut TaskContext { | |||
|
|||
/// call this function to switch to other task by using GLOBAL_SCHEDULER | |||
pub fn schedule() { | |||
unsafe { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Every usage of unsafe
should be justified with well-written comment. Inserting pass_quiescent_state
is actually unsound. Consider the following code.
let read_guard = rcu.get();
use_rcu_data(read_guard);
schedule(); // Oops: RCU-protected data is accessed across a quiescent state!
use_rcu_data(read_guard);
To prevent such situations, pass_quiescent_state
must be called after checking whether any preemption guards exist.
abeb197
to
9719fe7
Compare
} | ||
|
||
pub fn set_atime(&self, time: Duration) { | ||
self.metadata.write().atime = time; | ||
self.writer_lock.lock(); | ||
let reclaimer = self.metadata.replace({ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, this is not a good use case for RCU. Yes, using RCU reduces the overhead of the getter methods. But the setter methods become much expensive. For file systems, updating the file attributes may be frequent. So making the setter methods significantly more expensive is highly undesirable.
Enable non-preemptible RCU to the pointer for aster-framework.