Skip to content
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

Merged
merged 2 commits into from
Apr 15, 2024
Merged

Conversation

hawkw
Copy link
Contributor

@hawkw hawkw commented Apr 9, 2024

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 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.

hawkw added a commit that referenced this pull request Apr 9, 2024
I just thought this was a little nicer...
hawkw added a commit that referenced this pull request Apr 9, 2024
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.
@hawkw hawkw force-pushed the eliza/simplify-plic branch from 3ab577a to c2d8efa Compare April 9, 2024 15:35
@hawkw
Copy link
Contributor Author

hawkw commented Apr 9, 2024

@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.

@hawkw hawkw requested a review from jamesmunns April 9, 2024 15:38
@hawkw
Copy link
Contributor Author

hawkw commented Apr 13, 2024

hmm, i flashed this change on my D1 and observed the following:

  • the UART doesn't seem to work (no output in crowtty)
  • the blue blinky light is solid on rather than blinky

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.

@hawkw
Copy link
Contributor Author

hawkw commented Apr 15, 2024

hmm, i flashed this change on my D1 and observed the following:

* the UART doesn't seem to work (no output in crowtty)

* the blue blinky light is solid on rather than blinky

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!

hawkw added 2 commits April 15, 2024 10:37
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.
@hawkw hawkw force-pushed the eliza/simplify-plic branch from 03d1734 to d621e0d Compare April 15, 2024 17:40
@hawkw hawkw enabled auto-merge (rebase) April 15, 2024 17:41
@hawkw hawkw merged commit 7c463cc into main Apr 15, 2024
10 checks passed
@hawkw hawkw deleted the eliza/simplify-plic branch April 15, 2024 17:47
hawkw added a commit that referenced this pull request Apr 15, 2024
I just thought this was a little nicer...
hawkw added a commit that referenced this pull request Dec 27, 2024
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.
hawkw added a commit that referenced this pull request Dec 27, 2024
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
hawkw added a commit that referenced this pull request Dec 27, 2024
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
github-merge-queue bot pushed a commit that referenced this pull request Dec 27, 2024
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant