Allow specifying multiple interrupt handlers for the same IRQ number #143
Conversation
| /// Holds the interrupt handlers. The index is the IRQ number and this vector | ||
| /// contains `NUM_IRQ_NUMBERS` entries. | ||
| static IRQ_VECTORS: SpinLock<Vec<IrqVector>> = SpinLock::new(Vec::new()); | ||
|
|
||
| pub struct IrqVector { | ||
| handlers: Vec<Box<IrqHandler>>, |
There was a problem hiding this comment.
This is going to be vector of vectors and is going to hurt the cache badly. I'd suggest, leaving array:
static IRQ_VECTORS: SpinLock<[Vec<IrqVector>; 256]> = SpinLock::new([Vec::new(); 256]);so at least you'd have static array in fixed memory location without relocations on new base interrupt vector, @nuta.
There was a problem hiding this comment.
It sounds a sort of premature optimization. I do know it's not good for performance, but I don't want to consume too much time for it.
There was a problem hiding this comment.
I wouldn't call care for cache coherency in OS a premature optimisation, while Vector of Vector is well known for being bad at it. But, it's your call, @nuta.
There was a problem hiding this comment.
Moreover, original Linux Kernel uses a list for shared interrupts,¹. The nice summary is here:
https://unix.stackexchange.com/questions/47306/how-does-the-linux-kernel-handle-shared-irqs
Then I wonder if it would be better to use IDT on x86_64 to put some handlers directly there, but that might be hard in Rust without some assembler trickery.
¹ not all share; all drivers using shared interrupt have to declare using shared interrupts.
There was a problem hiding this comment.
I think we should not care until we actually need it. It's fast enough for me and I'd focus on adding more new feature at this early stage.
That said, I don't mean to give up the performance, in the safe way. Let's gradually consider the good approach to improve the performance by writing carefully written data structures in Rust 😃
| debug_assert!((irq as usize) < NUM_IRQ_NUMBERS); | ||
| let mut vectors = IRQ_VECTORS.lock(); | ||
| for handler in vectors[irq as usize].handlers_mut() { | ||
| handler(); | ||
| } |
There was a problem hiding this comment.
The alternative instead of holding the vector is to have a type held statically with a link to next element and the closure, that would let avoiding reallocation of vector for every new handler. Not very Rusty approach, unless there's idiomatic way for intrusive lists.
| pub fn init() { | ||
| let mut vectors = IRQ_VECTORS.lock(); | ||
| for _ in 0..NUM_IRQ_NUMBERS { | ||
| vectors.push(IrqVector::new()); | ||
| } |
There was a problem hiding this comment.
That itself is going to cause at least 8 reallocations as you haven't reserved the size of the vector in the first place.
No description provided.