Read one mouse packet byte per interrupt by hecatia-elegua · Pull Request #953 · theseus-os/Theseus

hecatia-elegua · 2023-05-08T09:44:01Z

Fix #832
Mutex should be good here, right?
Can these handlers be called from multiple cores at the same time? Interleaved, or nested?

kevinaboos

Mutex should be good here, right?
Can these handlers be called from multiple cores at the same time? Interleaved, or nested?

In our current configuration, a device interrupt is only delivered to one CPU core at once, and they generally aren't able to be nested (i.e., the next mouse interrupt cannot occur until the prior mouse interrupt has been completely handled). However, using just a Mutex for this generally isn't advised, considering that those aspects could change at any moment.

MutexIrqSafe is required when another execution context (like a normal function) is also accessing variables that are shared with and accessed by an interrupt handler. However, here, only the interrupt handler accesses this state, so we don't technically need one.

On a related note, see my inline comments about using an AtomicU64 to pack all the mouse packet bytes and the cursor into an efficient u64. This approach would be best.
If you don't want to do that and would prefer to stick with a Mutex instead, you should move that static mutex instance into the interrupt handler function scope itself, in order to ensure that nobody else can access it.

One other question: is it valid/possible to read multiple bytes from the mouse for a single interrupt? For example, if a lot of mouse activity is occurring, must we wait for each successive interrupt to read all the bytes? I'm not saying you should always forcibly read either 3 or 4 bytes at once, since that could indeed cause perf issues or infinite loops, but rather that you could iteratively check whether more bytes are available and read them all until no more bytes are available, all within a single interrupt handler invocation.

I'm asking because I'm not sure if the PS/2 mouse behavior allows/supports this.
In other devices, like the serial port, you do receive an interrupt when data is ready to be read in, but you can read more than just one byte per interrupt, which is faster than only reading a single byte per interrupt.

kevinaboos · 2023-06-06T00:13:30Z

kernel/mouse/src/lib.rs

+/// of a mouse packet per interrupt into an array.
+/// This can handle MouseId 0 (3 bytes) and 3, 4 (4 bytes).
+struct PacketBytes {
+    len: usize,


if len is always a statically-known value, we can use const generics here.

Also len is a bit vague, give it a more precise name (and docs) that conveys it's an "expected byte count" or the "number of bytes in a complete mouse packet"

kevinaboos · 2023-06-06T00:16:01Z

kernel/mouse/src/lib.rs

+    inner: Mutex<[u8; PS2_MAX_MOUSE_BYTES]>,
+    // where to push the next element
+    cursor: Mutex<usize>,


there's no real benefit here to using two mutexes. I'd condense this into one.

If you want to get even fancier, you can pack all of this data into an atomic u64 since you only need 5 bytes. That would be the fastest option of all.

kevinaboos · 2023-06-06T00:18:01Z

kernel/mouse/src/lib.rs


+    // Initialize the mouse packet bytes, which will be filled by 3-4 interrupts,
+    // depending on the MouseId
+    let packet_bytes = PacketBytes::new(mouse.packet_size());


here's where you'd specify the const generic parameter, either 3 or 4 depending upon mouse ID.

Nice idea, didn't consider const generics.

kevinaboos · 2023-06-06T00:22:27Z

kernel/mouse/src/lib.rs

+            packet_bytes.push(mouse.read_packet_byte());
+
+            if let Some(bytes) = packet_bytes.filled_bytes() {


these two instructions acquire and release a total of 4 locks. You can likely combine their functionality together such that a "push" operation returns an Option<[u8; N]> indicating whether the expected number of bytes have been received (and providing a copy of those 3 or 4 bytes).

Also, if you adopt a single-Mutex design (or better yet, the AtomicU64 design), that would reduce this overhead from 4 to 1 lock acquisitions.

Again, thanks, exactly what I need to learn.

yea np at all, this is just the kind of thing that only becomes apparent once you have another set of eyes on it. 😄

hecatia-elegua · 2023-06-06T06:21:46Z

kernel/mouse/src/lib.rs

 extern "x86-interrupt" fn ps2_mouse_handler(_stack_frame: InterruptStackFrame) {
-    if let Some(MouseInterruptParams { mouse, queue }) = MOUSE.get() {
+    if let Some(MouseInterruptParams { mouse, queue, packet_bytes }) = MOUSE.get() {
+        // using `while` here didn't interact well with the window manager and increases handler runtime


I've tried using a loop here and will try again, since some of the code changed.
This would handle multi-byte interrupts.

yeah, I recall the previous code there. I phrased my comment as a question because legitimately not sure if the PS/2 mouse device actually allows for multiple bytes to be read per interrupt (i.e., does it do "batching" of multiple bytes into a single interrupt?); once we answer that then we can determine what the best choice is here.

Yes it does, I've seen some osdev wiki people use while here 👍

kevinaboos · 2023-10-18T17:16:51Z

Hi @hecatia-elegua, I'm doing some PR triage and noticed this one was mostly ready, just needed a few minor fixes.

If you have time, would you be interested in making those changes? If not, I totally understand; just let me know and I can take over and fix things up before merging.

hecatia-elegua · 2023-10-18T17:59:58Z

I think back when I tried switching this to const generics + AtomicU64 it didn't just work easily and then I switched to other tasks. Feel free I guess

kevinaboos · 2023-10-18T18:25:00Z

I think back when I tried switching this to const generics + AtomicU64 it didn't just work easily and then I switched to other tasks.

Oh interesting! What did you try, and do you have your attempted code anywhere where I could check it out? (even if it's messy)

Feel free I guess

Ok thanks, will do.

hecatia-elegua · 2023-10-18T19:18:09Z

can't find it, I think I just removed the folder when switching computers

Read one mouse packet byte per interrupt

5aab4df

hecatia-elegua mentioned this pull request May 12, 2023

Moving PS/2 mouse and typing at the same time #832

Open

kevinaboos requested changes Jun 6, 2023

View reviewed changes

hecatia-elegua commented Jun 6, 2023

View reviewed changes

		packet_bytes.push(mouse.read_packet_byte());

		if let Some(bytes) = packet_bytes.filled_bytes() {

Uh oh!

Conversation

hecatia-elegua commented May 8, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kevinaboos left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kevinaboos commented Oct 18, 2023

Uh oh!

hecatia-elegua commented Oct 18, 2023

Uh oh!

kevinaboos commented Oct 18, 2023

Uh oh!

hecatia-elegua commented Oct 18, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

hecatia-elegua commented May 8, 2023 •

edited

Loading