-
Notifications
You must be signed in to change notification settings - Fork 18
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
PLIC refactors and performance improvements #312
Conversation
I just thought this was a little nicer...
Currently, the interrupt vector dispatch code uses a linear search over the interrupt vector array by checking each vector's ID against the interrupt number being dispatched. This is inefficient; because we already generate the vector array in order, we can simply index into it, which is _O_(1) instead of _O_(_n<sub>interrupts</sub>_). This commit changes the existing code to do that. We can rely on the ordering being correct as it's generated by a `const fn` that should always output the same order. However, I've also added debug assertions that the index and ID match, just in case the table somehow gets corrupted or something. I don't think this is really that necessary, but it seemed nice to have.
3ab577a
to
c2d8efa
Compare
@jamesmunns I'd love to get your review on this, since I'm hacking up your code. Also, I haven't actually tested this on my D1 yet, so I should make sure it like, works. |
hmm, i flashed this change on my D1 and observed the following:
which kind of suggests that interrupt handling is broken. however, the keyboard and SHARP display do seem to work. going to try flashing with a build from master and making sure we haven't just broken stuff on master too. |
Update: nevermind, the "broken UART" was just "i was looking at the wrong USB TTY", and after rebasing onto `main I now see a proper blinkenlight. False alarm! |
I just thought this was a little nicer...
Currently, the interrupt vector dispatch code uses a linear search over the interrupt vector array by checking each vector's ID against the interrupt number being dispatched. This is inefficient; because we already generate the vector array in order, we can simply index into it, which is _O_(1) instead of _O_(_n<sub>interrupts</sub>_). This commit changes the existing code to do that. We can rely on the ordering being correct as it's generated by a `const fn` that should always output the same order. However, I've also added debug assertions that the index and ID match, just in case the table somehow gets corrupted or something. I don't think this is really that necessary, but it seemed nice to have. This does trade space for time, a bit, as the interrupt vectors are sparse, so we must populate a 167-element array where many of the indices are `None`s (as 167 is the highest-vector hardware interrupt on the D1), rather than a 56-element array (the number of hardware interrupts). However, since we have 1GB of RAM, I think improving IRQ dispatch latency is probably worth it.
03d1734
to
d621e0d
Compare
I just thought this was a little nicer...
PR #312 (commit df41dec) inadvertently broke the `Plic` method to mask an interrupt by copy-pasting the code from `Plic::unmask`. Previously, `Plic::unmask` would write `MIE[offset]| irq_en` to `MIE[offset]`, setting the `IRQ_EN` bit for that interrupt; while `Plic::mask` would write `MIE[offset] & !irq_en` to MIE[offset]`, unsetting the IRQ_EN bit. But now they both *set* the `IRQ_EN` bit, so masking an interrupt actually unmasks it. Whoopsie. This seems to have occurred because I'm a LOSER FUCKING IDIOT. This commit puts it back the way it was supposed to be. Sorry.
PR #312 (commit df41dec) inadvertently broke the `Plic` method to mask an interrupt by copy-pasting the code from `Plic::unmask`. Previously, `Plic::unmask` would write `MIE[offset]| irq_en` to `MIE[offset]`, setting the `IRQ_EN` bit for that interrupt; while `Plic::mask` would write `MIE[offset] & !irq_en` to MIE[offset]`, unsetting the IRQ_EN bit. But now they both *set* the `IRQ_EN` bit, so masking an interrupt actually unmasks it. Whoopsie. This seems to have occurred because I'm a LOSER FUCKING IDIOT. This commit puts it back the way it was supposed to be. Sorry. Fixes #347
PR #312 (commit df41dec) inadvertently broke the `Plic` method to mask an interrupt by copy-pasting the code from `Plic::unmask`. Previously, `Plic::unmask` would write `MIE[offset] | irq_en` to `MIE[offset]`, setting the `IRQ_EN` bit for that interrupt; while `Plic::mask` would write `MIE[offset] & !irq_en` to `MIE[offset]`, unsetting the `IRQ_EN `bit. But now they both *set* the `IRQ_EN` bit, so masking an interrupt actually *unmasks* it. Whoopsie. This seems to have occurred because I'm a LOSER FUCKING IDIOT. This commit puts it back the way it was supposed to be. Sorry. Fixes #347
PR #312 (commit df41dec) inadvertently broke the `Plic` method to mask an interrupt by copy-pasting the code from `Plic::unmask`. Previously, `Plic::unmask` would write `MIE[offset]| irq_en` to `MIE[offset]`, setting the `IRQ_EN` bit for that interrupt; while `Plic::mask` would write `MIE[offset] & !irq_en` to `MIE[offset]`, unsetting the `IRQ_EN` bit. But now they both *set* the `IRQ_EN` bit, so masking an interrupt actually unmasks it. Whoopsie. This seems to have occurred because I'm a LOSER FUCKING IDIOT. This commit puts it back the way it was supposed to be. Sorry. Fixes #347
Currently, the interrupt vector dispatch code uses a linear search over
the interrupt vector array by checking each vector's ID against the
interrupt number being dispatched. This is inefficient; because we
already generate the vector array in order, we can simply index into it,
which is O(1) instead of O(ninterrupts). This commit
changes the existing code to do that.
We can rely on the ordering being correct as it's generated by a
const fn
that should always output the same order. However, I've also addeddebug assertions that the index and ID match, just in case the table
somehow gets corrupted or something. I don't think this is really that
necessary, but it seemed nice to have.