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

Refactor event handling logic & Handle the data abort case #394

Merged
merged 5 commits into from
Nov 21, 2024

Conversation

bitboom
Copy link
Collaborator

@bitboom bitboom commented Nov 6, 2024

This PR

  • refactors event handling logic
  • removes temporary mapping for handling data abort (011e2fa)

NOTE: I will submit the PR for managing the Page Table as a pinned object separately, as it is experimental.

Refactor event handling logic

Problem

  • RMM handles two types of events: RSI and RMI,
    but RMI handler was located in the mainloop while RSI handler was part of RMM.
  • The mainloop, created and managed by RMM, had a cyclic dependency
    as the RMI handler in the mainloop referenced the monitor.
  • Handling RMI in the mainloop also required locking the event queue,
    leading to performance issues.

Patch

  • Centralized both RMI and RSI event handlers within RMM for consistent event management.
  • Refactored mainloop to only poll for RMI events, following SRP principle.
  • Removed mutual references between mainloop and monitor, breaking the cyclic dependency.
  • Eliminated the need for queue locking by shifting event handling to RMM, improving performance.

@bitboom bitboom force-pushed the refactor-event branch 4 times, most recently from 7201b4f to c2a5ee8 Compare November 19, 2024 04:38
@bitboom bitboom changed the title [DO-NOT-MERGE] Refactor event handling logic for Pinned Page Table Refactor event handling logic & Handle the data abort case Nov 19, 2024
@bitboom bitboom marked this pull request as ready for review November 19, 2024 06:12
Copy link
Collaborator

@zpzigi754 zpzigi754 left a comment

Choose a reason for hiding this comment

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

Good refactoring!

rmm/src/test_utils.rs Outdated Show resolved Hide resolved
Copy link
Collaborator

@nook1208 nook1208 left a comment

Choose a reason for hiding this comment

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

nice refactoring !

rmm/src/monitor.rs Show resolved Hide resolved
Problem:
- RMM handles two types of events: RSI and RMI,
  but RMI handler was located in the mainloop while RSI handler was part of RMM.
- The mainloop, created and managed by RMM, had a cyclic dependency
  as the RMI handler in the mainloop referenced the monitor.
- Handling RMI in the mainloop also required locking the event queue,
  leading to performance issues.

Patch:
- Centralized both RMI and RSI event handlers within RMM for consistent event management.
- Refactored mainloop to only poll for RMI events, following SRP principles.
- Removed mutual references between mainloop and monitor, breaking the cyclic dependency.
- Eliminated the need for queue locking by shifting event handling to RMM, improving performance.

Signed-off-by: Sangwan Kwon <[email protected]>
`event::Context` is a data structure that contains only the necessary
context for event handling, separate from RMI processing.

By allowing the monitor to directly own the handler and process
the events (RMI, RSI), the roles of each structure are clarified,
reducing unnecessary handler passing.

Signed-off-by: Sangwan Kwon <[email protected]>
Signed-off-by: Sangwan Kwon <[email protected]>
@bitboom bitboom merged commit 66fa71a into main Nov 21, 2024
9 checks passed
@bitboom bitboom deleted the refactor-event branch November 21, 2024 01:30
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.

5 participants