Read one mouse packet byte per interrupt #953
Conversation
There was a problem hiding this comment.
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.
| /// 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, |
There was a problem hiding this comment.
if len is always a statically-known value, we can use const generics here.
There was a problem hiding this comment.
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"
| inner: Mutex<[u8; PS2_MAX_MOUSE_BYTES]>, | ||
| // where to push the next element | ||
| cursor: Mutex<usize>, |
There was a problem hiding this comment.
there's no real benefit here to using two mutexes. I'd condense this into one.
There was a problem hiding this comment.
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.
| @@ -41,9 +81,13 @@ pub fn init(mut mouse: PS2Mouse<'static>, mouse_queue_producer: Queue<Event>) -> | |||
| "PS/2 mouse IRQ was already in use! Sharing IRQs is currently unsupported." | |||
| })?; | |||
|
|
|||
| // 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()); | |||
There was a problem hiding this comment.
here's where you'd specify the const generic parameter, either 3 or 4 depending upon mouse ID.
There was a problem hiding this comment.
Nice idea, didn't consider const generics.
| packet_bytes.push(mouse.read_packet_byte()); | ||
|
|
||
| if let Some(bytes) = packet_bytes.filled_bytes() { |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Again, thanks, exactly what I need to learn.
There was a problem hiding this comment.
yea np at all, this is just the kind of thing that only becomes apparent once you have another set of eyes on it. 😄
| @@ -56,18 +100,20 @@ pub fn init(mut mouse: PS2Mouse<'static>, mouse_queue_producer: Queue<Event>) -> | |||
| /// | |||
| /// In some cases (e.g. on device init), [the PS/2 controller can also send an interrupt](https://wiki.osdev.org/%228042%22_PS/2_Controller#Interrupts). | |||
| 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 | |||
There was a problem hiding this comment.
I've tried using a loop here and will try again, since some of the code changed.
This would handle multi-byte interrupts.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Yes it does, I've seen some osdev wiki people use while here 👍
|
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. |
|
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 |
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)
Ok thanks, will do. |
|
can't find it, I think I just removed the folder when switching computers |
Fix #832
Mutexshould be good here, right?Can these handlers be called from multiple cores at the same time? Interleaved, or nested?