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

Optimizations to improve the performance of Ramfs #826

Merged
merged 5 commits into from
May 16, 2024

Conversation

liqinggd
Copy link
Contributor

@liqinggd liqinggd commented May 9, 2024

Refer to #791

@StevenJiang1110 Please review this commit, thanks.

@liqinggd liqinggd changed the title Optimize the SigQueues to rapidly dequeue signals Optimize the SigQueues to return early without lock May 10, 2024
@liqinggd liqinggd force-pushed the dev-sigqueue branch 3 times, most recently from 2d232de to 84e1fc8 Compare May 11, 2024 06:35
@liqinggd liqinggd changed the title Optimize the SigQueues to return early without lock Optimizations to improve the performance May 11, 2024
@liqinggd liqinggd force-pushed the dev-sigqueue branch 3 times, most recently from 826831b to 8d0519f Compare May 11, 2024 06:47
@liqinggd liqinggd changed the title Optimizations to improve the performance Optimizations to improve the performance of Ramfs May 11, 2024
current as u8,
new as u8,
Ordering::Acquire,
Ordering::Relaxed,
Copy link
Contributor

Choose a reason for hiding this comment

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

I think that you mean Ordering::Release, although I'm not sure whether or not this memory order is necessary for the thread status.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The success describes the required ordering for the read-modify-write operation that takes place if the comparison with current succeeds.

I think AcqRel is correct, it has the effects of both Acquire and Release together: For loads it uses Acquire ordering. For stores it uses the Release ordering.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, but you are writing Order::Relaxed. So you should write Order::Release instead. I mean, there is a typo.

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 misunderstood your comment, so it seems we should write success = Ordering::AcqRel, failure = Ordering::Relaxed/Acquire (the failure order depends on whether we want to synchronize with the last status write if the exchange fails).

@lrh2000
Copy link
Contributor

lrh2000 commented May 13, 2024

  • SigQueue::dequeue (that serves a sink) works exactly the opposite way of WaitQueue::wake_one (that serves a source). In the case of SigQueue, the count's acquire-release semantics is correct but redundant as it is already guaranteed by the internal lock, so accessing count should be fine even with Ordering::Relaxed.

Well, I thought it would be another great example to discuss together in #831. So leaving this as a TODO for #831 is fine.

@liqinggd
Copy link
Contributor Author

  • SigQueue::dequeue (that serves a sink) works exactly the opposite way of WaitQueue::wake_one (that serves a source). In the case of SigQueue, the count's acquire-release semantics is correct but redundant as it is already guaranteed by the internal lock, so accessing count should be fine even with Ordering::Relaxed.

Yes, you are correct, the count will be fine with Ordering::Relaxed.

kernel/aster-nix/src/process/signal/sig_queues.rs Outdated Show resolved Hide resolved
@@ -31,7 +31,7 @@ pub struct Thread {
data: Box<dyn Send + Sync + Any>,

// mutable part
status: Mutex<ThreadStatus>,
status: AtomicU8,
Copy link
Contributor

Choose a reason for hiding this comment

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

To simplify thread/mod.rs, I think a new type named AtomicThreadStatus should be provided by thread/status.rs and used by thread/mod.rs. Most changes made to thread/mod.rs can be extracted to and hidden inside AtomicThreadStatus. This makes the code easier to understand and maintain.

pub struct AtomicThreadStatus(AtomicU8);

impl AtomicThreadStatus {
    pub fn load(&self, ordering: Ordering) -> ThreadStatus { ... }

    pub fn store(&self, new_status: ThreadStatus, ordering: Ordering) -> ThreadStatus { ... }

    pub fn compare_exchange( &self, current: ThreadStatus, new: ThreadStatus, success: Ordering, failure: Ordering ) -> Result<ThreadStatus, ThreadStatus>
}

The methods of AtomicThreadStatus mirrors that of AtomicU8. They take Ordering as input so that the user can decide the ordering.

kernel/aster-nix/src/fs/ramfs/fs.rs Outdated Show resolved Hide resolved
kernel/aster-nix/src/fs/ramfs/fs.rs Outdated Show resolved Hide resolved
kernel/aster-nix/src/fs/ramfs/fs.rs Outdated Show resolved Hide resolved
@liqinggd liqinggd force-pushed the dev-sigqueue branch 2 times, most recently from 9692570 to 366bae6 Compare May 16, 2024 08:13
kernel/aster-nix/src/thread/mod.rs Outdated Show resolved Hide resolved
kernel/aster-nix/src/fs/ramfs/fs.rs Show resolved Hide resolved
kernel/aster-nix/src/fs/ramfs/fs.rs Show resolved Hide resolved
Copy link
Contributor

@tatetian tatetian left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks for the contribution!

@tatetian tatetian merged commit f4ea23b into asterinas:main May 16, 2024
5 checks passed
@liqinggd liqinggd deleted the dev-sigqueue branch June 5, 2024 02:13
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.

None yet

4 participants