ISSUE-29: Switch IRQ handler's map from BTreeMap to array #35
Conversation
kernel/interrupt.rs
Outdated
| // TODO: Use a simple array for faster access. | ||
| static IRQ_HANDLERS: SpinLock<BTreeMap<u8, Box<dyn FnMut() + Send + Sync>>> = | ||
| SpinLock::new(BTreeMap::new()); | ||
| static IRQ_HANDLERS: SpinLock<[Option<Box<dyn FnMut() + Send + Sync>>; 255]> = SpinLock::new([ |
There was a problem hiding this comment.
An IRQ number can be in the range of [0, 255]. We need 256 slots.
| static IRQ_HANDLERS: SpinLock<[Option<Box<dyn FnMut() + Send + Sync>>; 255]> = SpinLock::new([ | |
| static IRQ_HANDLERS: SpinLock<[Option<Box<dyn FnMut() + Send + Sync>>; 256]> = SpinLock::new([ |
There was a problem hiding this comment.
How about using a const to avoid writing a lot of None?
const DEFAULT_IRQ_HANDLER: Option<Box<dyn FnMut() + Send + Sync>> = None;
static IRQ_HANDLERS: SpinLock<[Option<Box<dyn FnMut() + Send + Sync>>; 256]> =
SpinLock::new([DEFAULT_IRQ_HANDLER; 256]);
There was a problem hiding this comment.
I was googling for a solution like this but somehow did not find it :)
Replaced
And size is also fixed.
There was a problem hiding this comment.
I found this stackoverflow answer. I was beginning to give up on that too ;)
Good point. it will be helpful if there's an assertion to ensure no handlers are already registered.
I think so too. It should be moved to somewhere else.
I agree these points should be improved, but I'd merge them as separate PRs. |
|
Merged. Thank you @Lurk! |
Fixes #29
I have very little knowledge of how OS should work, but some parts of IRQ handlers do not feel right.
Here
https://github.com/Lurk/kerla/blob/b213c965d3127a35b429da62fa9d134304107fbf/kernel/interrupt.rs#L28-L31
we do not check if the handler is already assigned. That will definitely lead to strange bugs.
And here
https://github.com/Lurk/kerla/blob/b213c965d3127a35b429da62fa9d134304107fbf/kernel/interrupt.rs#L33-L38
The call of net::process_packets feels like it should be done in the handler.
If I am right, should I tackle it in this pull request or create separate issues and pull requests?