supervisor: x2apic/ioapic and infrastructure#220
Conversation
…Us from x2APIC entries.
…e, run queues, counters).
…d interrupt routing.
…, inhibit-scheduling.
… flag in idle loop.
…ocation - Change mcs-node slot initform from (%make-mcs-node) to nil to avoid cold-generator eval issue with cross-environment function references - Pre-allocate mcs-node in cold-generator and CPU init functions - Add %cpuid-1-ecx-early: save/restore EBX around CPUID for early boot - Skip lapic registers invalid in x2APIC mode (ARP, PPR, logical dest) - Initialize *io-apics*/*io-apic-active-p* with boundp checks
…eclaim dead threads
…-button to use broadcast-ipi
… remove stale TODO
froggey
left a comment
froggey
left a comment
There was a problem hiding this comment.
this is a pretty huge patch with a bunch of different features in it. could it be broken up into smaller PRs? it's been pretty difficult to review. Maybe
- x2apic
- redesign of tlb shootdowns
- mcs locks
- priority inheritance
are the mcs locks supposed to supersede the existing test-and-set spinlocks or live alongside them? if it's possible to completely replace the old spinlocks that'd be nice. I'll point out that the existing spinlock functions/macros are named in a way that's independent of their implementation, (place and symbol refer to where the spinlock lives, rather than the kind of implementation)
| (setf (cpu-inhibit-scheduling (local-cpu)) | ||
| (1+ (cpu-inhibit-scheduling (local-cpu)))) |
There was a problem hiding this comment.
This (along with the bit that undoes it) feels like something that should be factored out into a macro
| (ensure (not *tlb-shootdown-in-progress*) "TLB shootdown already in progress!") | ||
| (setf *tlb-shootdown-in-progress* t) | ||
| (setf (cpu-inhibit-scheduling (local-cpu)) | ||
| (1+ (cpu-inhibit-scheduling (local-cpu))))) |
There was a problem hiding this comment.
same here, could factor out into a separate function
| (broadcast-ipi +wakeup-sgi-id+)) | ||
| (dolist (cpu *cpus*) | ||
| (when (eql (arm64-cpu-state cpu) :online) | ||
| (wake-cpu cpu)))) | ||
|
|
||
| (defun wake-cpu (cpu) | ||
| (when (cpu-idle-p cpu) | ||
| (send-ipi-to-cpu cpu +wakeup-sgi-id+))) |
There was a problem hiding this comment.
there's something a bit funny about this, if a cpu is offline/never came up then it shouldn't be idle so the online check seems redundant
There was a problem hiding this comment.
ok, i think i see. x86-64's broadcast-ipi checks if the cpu is online but arm's doesn't. I don't think it's needed on arm as the gic wouldn't be configured for an offline cpu and the cpu wouldn't be capable of receiving it.
| (cond (*debug-magic-button-hold-variable* | ||
| (magic-button-ipi-handler interrupt-frame)) | ||
| (t |
There was a problem hiding this comment.
why did this get merged into the quiesce ipi handler?
| ;; Return this thread to the run queue. | ||
| (setf (thread-state current) :runnable) | ||
| (push-run-queue current) | ||
| (preemption-timer-reset nil) | ||
| ;; Save thread state. | ||
| (save-fpu-state current) | ||
| (save-interrupted-state current interrupt-frame) | ||
| ;; Partially switch to the idle thread. | ||
| (setf (thread-state idle) :active)) | ||
| ;; Have now reached a quiescent state. | ||
| (sys.int::%atomic-fixnum-add-symbol '*non-quiescent-cpus-remaining* | ||
| -1) | ||
| (when was-active | ||
| ;; Finally, return to the idle thread. |
There was a problem hiding this comment.
all these comments got removed. they're not great comments, but they help me follow the phases of this function
| (defun %cleanup-dead-threads () | ||
| "Remove threads marked :dead from the *all-threads* list. | ||
| The global thread lock must be held." | ||
| (ensure-global-thread-lock-held) | ||
| (dolist (thread *rcu-deferred-list*) | ||
| (when (thread-global-next thread) | ||
| (setf (thread-global-prev (thread-global-next thread)) (thread-global-prev thread))) | ||
| (when (thread-global-prev thread) | ||
| (setf (thread-global-next (thread-global-prev thread)) (thread-global-next thread))) | ||
| (when (eql thread *all-threads*) | ||
| (setf *all-threads* (thread-global-next thread)))) | ||
| (setf *rcu-deferred-list* nil)) | ||
|
|
||
| (defun cleanup-dead-threads () | ||
| "Remove threads marked :dead from the *all-threads* list." | ||
| (without-interrupts | ||
| (acquire-global-thread-lock) | ||
| (%cleanup-dead-threads) | ||
| (release-global-thread-lock))) |
There was a problem hiding this comment.
as the rcu-synchronize code all seems dead it looks like this is dead too and dead threads will never be cleaned up?
| (when (wait-queue-p wi) | ||
| (lock-wait-queue wi)) | ||
| (with-symbol-spinlock (*global-thread-lock*) | ||
| (with-global-thread-lock nil |
| (flet ((push-foothold () | ||
| (safe-without-interrupts (thread push-cons) | ||
| (with-symbol-spinlock (*global-thread-lock*) | ||
| (with-global-thread-lock nil |
| (defun sample-thread-state (thread) | ||
| (safe-without-interrupts (thread) | ||
| (with-symbol-spinlock (*global-thread-lock*) | ||
| (with-global-thread-lock nil |
| (setf (env:cross-symbol-value environment 'mezzano.supervisor::*bsp-cpu*) | ||
| (env:make-structure environment 'mezzano.supervisor::arm64-cpu)) | ||
| (let ((bsp-cpu (env:make-structure environment 'mezzano.supervisor::arm64-cpu))) | ||
| (setf (env:structure-slot-value environment bsp-cpu 'mezzano.supervisor::mcs-node) | ||
| (env:make-structure environment 'mezzano.supervisor::mcs-node)) | ||
| (setf (env:cross-symbol-value environment 'mezzano.supervisor::*bsp-cpu*) bsp-cpu)) |
There was a problem hiding this comment.
ok, looks like the cold-generator does populate the mcs-node slot, so the conditional initialization can be removed
|
thx a lot for taking the time for the code review and putting up with the quality; sorry that I let so much through the cracks, I did not read the code thoroughly enough before sending. I'll start working on these issues later. The unused stuff though: that's the "skeleton for the scheduler rewrite" part. Technically it could be deferred for later PRs. I will look into that. I am not used to splitting PRs into series, dealing with their dependencies and all that. So I was kinda scared, and didn't work on MSI or the scheduler while waiting. And oh yeah 0 way of testing ARM. |
|
yeah splitting a big pr into smaller ones can be a bit of a pain but it's a useful skill to have I'm happy to test on arm when it gets to a more stable state (and make any fixes that might be needed too) |
2 participants
Adds:
x2apic
io-apic
directed IPIs
lazy TLB shootdown
fair MCS spinlocks
the skeleton for MSI + irq balancing and scheduler rewrite branches.
Boots into desktop on my machine. Not tested on ARM.