Skip to content

Fix runtime on rp2350/2040 for multicore scheduler #5286

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.

Sign up for GitHub

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

Open
vejmarie wants to merge 5 commits into
base: dev
Choose a base branch
from
Open

Fix runtime on rp2350/2040 for multicore scheduler #5286

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
df199cc
Fix runtime on rp2350/2040 for multicore scheduler
vejmarie Apr 8, 2026
293bb3f
Format the file to pass validation test
vejmarie Apr 8, 2026
44879bb
Run golint to format file
vejmarie Apr 8, 2026
a254b27
Remove call to SetACTLR_EXTEXCLALL as it is CPU specific
vejmarie Apr 8, 2026
fe5c6e2
Create a specific init function for each chip architecture when
vejmarie Apr 8, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
92 changes: 63 additions & 29 deletions src/runtime/runtime_rp2.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
//go:build rp2040 || rp2350

// Used to track contribution from various authors automatically
// SPDX-FileCopyrightText: Copyright Hewlett Packard Enterprise Development LP
package runtime

import (
Expand All @@ -10,6 +12,7 @@ import (
_ "machine/usb/cdc"
"runtime/interrupt"
"runtime/volatile"
"sync/atomic"
"unsafe"
)

Expand Down Expand Up @@ -116,6 +119,19 @@ func currentCPU() uint32 {
// Start the secondary cores for this chip.
// On the RP2040/RP2350, there is only one other core to start.
func startSecondaryCores() {

// Reset Core1
arm.DisableIRQ(uint32(sioIrqFifoProc0))
rp.PSM.SetFRCE_OFF_PROC1(1)
// Now we need to restart the core
for rp.PSM.GetFRCE_OFF_PROC1() == 0 {
// Sleep a little bit
sleepTicks(100)
}
rp.PSM.SetFRCE_OFF_PROC1(0)
multicore_fifo_pop_blocking()
Copy link

Copilot AI Apr 9, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

multicore_fifo_pop_blocking() here can deadlock boot if the FIFO is empty (which is a valid state after reset/power-on). If the intent is to clear stale FIFO data before restarting core1, use multicore_fifo_drain() (non-blocking) or gate the pop behind a FIFO_ST.VLD check to avoid blocking forever.

Suggested change
multicore_fifo_pop_blocking()
multicore_fifo_drain()

Copilot uses AI. Check for mistakes.
Copy link
Copy Markdown
Author

@vejmarie vejmarie Apr 9, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The FIFO will be empty but CORE1 will write to it as soon as it starts. The logic is to reset the CORE and wait for it to tell us it is ready, so we block while waiting for the message coming from CORE1 into the FIFO

sleepTicks(1000)

// Start the second core of the RP2040/RP2350.
// See sections 2.8.2 and 5.3 in the datasheets for RP2040 and RP2350 respectively.
seq := 0
Expand All @@ -141,9 +157,16 @@ func startSecondaryCores() {
// We can only do this after we don't need the FIFO anymore for starting the
// second core.
intr := interrupt.New(sioIrqFifoProc0, func(intr interrupt.Interrupt) {
switch rp.SIO.FIFO_RD.Get() {
case 1:
gcInterruptHandler(0)
status := rp.SIO.GetFIFO_ST_VLD()
if status != 0 {
switch rp.SIO.FIFO_RD.Get() {
case 1:
if currentCPU() == 1 {
gcInterruptHandler(1)
} else {
gcInterruptHandler(0)
}
}
}
})
intr.Enable()
Expand All @@ -169,15 +192,26 @@ var stack1TopSymbol [0]uint32
func runCore1() {
// Clear sticky bit that seems to have been set while starting this core.
rp.SIO.FIFO_ST.Set(rp.SIO_FIFO_ST_ROE)
rp.SIO.FIFO_ST.Set(rp.SIO_FIFO_ST_WOF)

// Enable the FIFO interrupt, mainly used for the stop-the-world phase of
// the GC.
// Use the lowest possible priority (highest priority value), so that other
// interrupts can still happen while the GC is running.
intr := interrupt.New(sioIrqFifoProc1, func(intr interrupt.Interrupt) {
switch rp.SIO.FIFO_RD.Get() {
case 1:
gcInterruptHandler(1)
status := rp.SIO.GetFIFO_ST_VLD()
// ok we have just one interrupt vector ... sor
// we shall be acting based on CPU number calling in

if status != 0 {
switch rp.SIO.FIFO_RD.Get() {
case 1:
if currentCPU() == 1 {
gcInterruptHandler(1)
} else {
gcInterruptHandler(0)
}
}
}
})
intr.Enable()
Expand Down Expand Up @@ -291,41 +325,39 @@ func coreStackTop(core uint32) uintptr {
}

// These spinlocks are needed by the runtime.
var (
printLock = spinLock{id: 20}
schedulerLock = spinLock{id: 21}
atomicsLock = spinLock{id: 22}
futexLock = spinLock{id: 23}
)

func resetSpinLocks() {
for i := uint8(0); i < numSpinlocks; i++ {
l := &spinLock{id: i}
l.spinlock().Set(0)
}
}

// A hardware spinlock, one of the 32 spinlocks defined in the SIO peripheral.
type spinLock struct {
id uint8
atomic.Uint32
}

// Return the spinlock register: rp.SIO.SPINLOCKx
func (l *spinLock) spinlock() *volatile.Register32 {
return (*volatile.Register32)(unsafe.Add(unsafe.Pointer(&rp.SIO.SPINLOCK0), l.id*4))
var (
schedulerLock spinLock
futexLock spinLock
atomicsLock spinLock
printLock spinLock
)

func resetSpinLocks() {
schedulerLock.Store(0)
futexLock.Store(0)
atomicsLock.Store(0)
printLock.Store(0)
}

func (l *spinLock) Lock() {
// Wait for the lock to be available.
spinlock := l.spinlock()
for spinlock.Get() == 0 {
arm.Asm("wfe")

for !l.Uint32.CompareAndSwap(0, 1) {
spinLoopWait()
}
Comment on lines 329 to 352
Copy link

Copilot AI Apr 9, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The new spinLock implementation relies on atomic.Uint32.CompareAndSwap. On RP2040 (thumbv6m), atomic CAS operations are typically lowered to the __sync/__atomic libcalls implemented in src/runtime/atomics_critical.go, which enter lockAtomics(). With the multicore scheduler, lockAtomics() itself takes atomicsLock (scheduler_cores.go:281-289), creating a recursion/deadlock (spinLock -> CAS libcall -> lockAtomics -> atomicsLock.Lock -> spinLock...). Consider keeping the previous hardware-spinlock-based implementation for RP2040, or implement this lock using a mechanism that does not depend on sync/atomic CAS on targets without native atomics.

Copilot uses AI. Check for mistakes.
Copy link
Copy Markdown
Contributor

@soypat soypat Apr 9, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks like BS at a glance. Thoughts @vejmarie ?

Copy link
Copy Markdown
Author

@vejmarie vejmarie Apr 9, 2026 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it might be right. The M0 from the rp2040 doesn't seems to support CompareAndSwap in an atomic way. AI will keep surprising me. My challenge is that I don't have an rp2040 handy to test. I can say it works on rp2350 without any issue (I tested it dozen times). The hardware spin lock works on the rp2040, so I can create in some way wrappers and push them into the related architectural specific files which will be compiled based on target selection. The deadlock mentioned could appeared as we will face a recursive call to the lock subsystem in such cases. But if a core grab a lock it will go through I think. Let me know what you would prefer.

Copy link
Copy Markdown
Author

@vejmarie vejmarie Apr 9, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just checked the code, and I bet it shall work on rp2040, as CompareAndSwap exist for that platform even if it shall call lockAtomics recursively, it will either wait (which means other core is having the lock and will get out), or it will acquire it. It the second core crash with an Atomics lock acquired, it means there is a bug somewhere


}

func (l *spinLock) Unlock() {
l.spinlock().Set(0)
arm.Asm("sev")
if schedulerAsserts && l.Uint32.Load() != 1 {
runtimePanic("unlock of unlocked spinlock")
}
l.Uint32.Store(0)
Copy link

Copilot AI Apr 9, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

spinLock.Lock() uses WFE in the contention loop (spinLoopWait), but Unlock() no longer executes SEV after releasing the lock. If the waiter is sleeping in WFE with interrupts masked (common in runtime critical sections), it may not wake promptly (or at all) when the lock becomes available. Consider restoring an explicit SEV in Unlock (after the Store(0)) or switching the wait strategy away from WFE if you don't want to send events on unlock.

Suggested change
l.Uint32.Store(0)
l.Uint32.Store(0)
arm.Asm("sev")

Copilot uses AI. Check for mistakes.
Copy link
Copy Markdown
Author

@vejmarie vejmarie Apr 9, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Non necessary the scheduler is taking care about waking up the core when needed. Tested with multiple g routines running at the same time and calls to GC.

}

// Wait until a signal is received, indicating that it can resume from the
Expand Down Expand Up @@ -361,6 +393,8 @@ func init() {
machineInit()

machine.InitSerial()

initRP()
}

func prerun() {
Expand Down
3 changes: 3 additions & 0 deletions src/runtime/runtime_rp2040.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,3 +10,6 @@ const (
sioIrqFifoProc0 = rp.IRQ_SIO_IRQ_PROC0
sioIrqFifoProc1 = rp.IRQ_SIO_IRQ_PROC1
)

func initRP() {
}
4 changes: 4 additions & 0 deletions src/runtime/runtime_rp2350.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,3 +14,7 @@ const (
sioIrqFifoProc0 = rp.IRQ_SIO_IRQ_FIFO
sioIrqFifoProc1 = rp.IRQ_SIO_IRQ_FIFO
)

func initRP() {
rp.PPB.SetACTLR_EXTEXCLALL(1)
}
Loading