RP2040: avoid XIP hangs during flash operations with scheduler=cores#5411
RP2040: avoid XIP hangs during flash operations with scheduler=cores#5411rdon-key wants to merge 11 commits into
Conversation
59911c7 to
4c44c4a
Compare
|
Rebased onto the latest dev after #5391 (usb/cdc: fix RP2 USB CDC TX race with cores scheduler) was merged. Re-ran the RP2040 Pico reproducer:
|
| interrupt.Restore(state) | ||
| } | ||
|
|
||
| func rp2FlashSafeInterruptHandler(core uint32) { |
There was a problem hiding this comment.
You could add a comment here that it is a no-op on single core.
| } | ||
|
|
||
| func rp2FlashSafeInterruptHandler(core uint32) { | ||
| _ = core |
There was a problem hiding this comment.
If you add the above comment you can remove this line.
|
|
||
| package runtime | ||
|
|
||
| func rp2FlashSafeInterruptHandler(core uint32) { |
|
Thank you for your review. I addressed the comments. Also, the println / monitor output appears to be affected by a separate issue, so I attached a reduced test case with fewer messages. Details |
e3dc1bd to
27373a0
Compare
|
Rebased this branch onto current dev. This branch conflicted with #5482, runtime/rp2: handle RP2350 shared FIFO IRQ for GC. During the rebase, I resolved the conflict by keeping the current per-chip SIO FIFO IRQ handler split introduced by #5482, and applying the flash-safe FIFO command handling to that structure instead of restoring the older inline handlers in runtime_rp2.go. The resolved structure is:
Tested after the rebase: GC stress test
Each target survived 10,000 cycles of runtime.Gosched() + runtime.GC(). RP2040 flash-safe stress test
The flash-safe stress test completed 100 rounds successfully. Each round erased, read back, wrote, verified, and erased a flash sector again. For scheduler=cores, another goroutine was running on the other core. It continuously generated XIP pressure by randomly reading from a 64 KB flash-resident const array, and also periodically performed flash erase/write operations to a separate sector. Result:
I also ran the flash test on pico2 with scheduler=tasks as a sanity check. I did not run the pico2 scheduler=cores flash test, because this PR does not implement RP2350 flash-safe handling; the RP2350 flash-safe handler is intentionally a no-op stub, and the pico2 GC stress test already covers the shared FIFO IRQ path. |
aykevl
left a comment
There was a problem hiding this comment.
Looks reasonable, a few comments below.
| // | ||
| // id: 24 is reserved here. ids 20-23 are already used by printLock, | ||
| // schedulerLock, atomicsLock, futexLock (see runtime_rp2.go). | ||
| var flashSafeLock = spinLock{id: 24} |
There was a problem hiding this comment.
Please either put this spinLock in runtime_rp2.go along with the rest (preferably), or add a comment there referring here that ID 24 is in use. Otherwise it's easy to miss this other spinlock when adding a new spinlock in the future.
There was a problem hiding this comment.
Thanks. I moved flashSafeLock next to the other RP2 runtime spinLock definitions in runtime_rp2.go, so the spinLock IDs remain visible in one place.
| txbuf := make([]byte, len(tx)) | ||
| copy(txbuf, tx) |
There was a problem hiding this comment.
Why does this need to be copied to a newly heap-allocated array, instead of just using tx directly?
There was a problem hiding this comment.
Thanks, this is an important concern. I removed the unconditional RAM copy here.
I originally added it after seeing failures when the source buffer itself was placed in flash, but that is a separate pre-existing limitation: using an XIP flash address as the source buffer is not safe even with the tasks scheduler.
For this PR, I think it is better to rely on the caller passing a RAM-resident buffer, as before, and avoid adding an unconditional heap allocation. XIP-flash source buffers should be handled explicitly in a separate change, either by copying only when the source overlaps XIP flash or by returning an error for that case.
| buf := make([]byte, len(padded)) | ||
| copy(buf, padded) |
There was a problem hiding this comment.
Same here, this is potentially very expensive. In fact, flashPad already can heap allocate if it isn't aligned and you're allocating even more here (where p can be a potentially large buffer).
There was a problem hiding this comment.
Same here. I removed the unconditional RAM copy and now use the padded slice directly, so flashPad's buffer is not copied again.
| } | ||
|
|
||
| func rp2040FlashSafePauseCore(core uint32) { | ||
| _ = core // RP2040 SIO FIFO writes to the other core. |
There was a problem hiding this comment.
Nit: this isn't doing anything (though the comment is helpful and would be useful to keep)
| _ = core // RP2040 SIO FIFO writes to the other core. |
There was a problem hiding this comment.
Done. I removed the unused assignment and kept the RP2040 SIO FIFO comment.
| // | ||
| //go:section .ramfuncs | ||
| func rp2FlashSafeInterruptHandler(core uint32) { | ||
| _ = core |
There was a problem hiding this comment.
same here, is this for linter reasons? Right not it just adds noise.
There was a problem hiding this comment.
Done. It wasn't for linter reasons; removing it doesn't cause any warning, so I removed the unused assignment here as well.
|
I addressed the review comments and re-ran the RP2040 flash-safe stress test with both scheduler=tasks and scheduler=cores. Both completed 100 rounds successfully after removing the unconditional RAM copies. This PR is still scoped to RP2040 only; the RP2350 flash-safe handler remains a no-op stub. If this direction looks good, I'm happy to implement similar handling for RP2350 as well — would you prefer that here, or as a separate follow-up PR? |
Fixes #5288
RP2040 flash program / erase / command operations temporarily disable XIP.
With
scheduler=cores, the other core may continue executing instructions from XIP flash during that window, which can cause a system hang.What was done
This change adds an RP2040-specific flash-safe section.
Runtime changes:
scheduler=cores, send a SIO FIFO command to the other core before starting a flash operation.Machine changes:
flash_range_writeflash_erase_blocksflash_do_cmdThese flash operations now run inside
rp2040EnterFlashSafeSection/rp2040ExitFlashSafeSection.Notes
This is intentionally limited to RP2040 flash operations that temporarily disable XIP.
It is not intended to be a general multicore lock.
Other shared peripherals should be protected by their own ownership or locking rules.
RP2350 behavior is intentionally left unchanged.
The RP2350 handler added here is only a build-only stub for shared RP2 runtime code.
If the monitor output becomes corrupted under
scheduler=cores, for example:please also test this reproducer with pull request #5391:
That output corruption appears to be a separate USB CDC multicore output issue, and it can make the flash test result difficult to read.
Reproducer
Warning: Flash memory has a limited number of program/erase cycles. This reproducer repeatedly erases and writes flash, so run it only when needed.
main.go
Test results
Tested on RP2040/Pico.
With
-scheduler=cores(the failing case before this PR)Reproducer: a worker goroutine on core 1 performs continuous random reads
from a 64 KB flash-resident const slice (which defeats the 16 KB XIP cache)
and periodic flash writes to a fixed low offset. Main on core 0 does
100 rounds of erase / write / read-back / verify on the top sectors of the
data region.
Before this PR: hangs at the first
write starton core 0.After this PR:
Evidence:
worker started on core: 1andworker core: 1in every round headerconfirm the test condition (worker is actually running on core 1, not 0).
worker writes: Ngrows from 0 to 156, meaning the worker successfullyperformed 156 flash writes concurrently with main's flash ops. Each of
those is a moment when both cores were contending for the flash-safe
section, exercising the spinlock added in this PR.
any
mismatchorerror, so the cross-core lockout protocol did notcorrupt either core's data.
worker stopped cleanlyconfirms no deadlock at shutdown.With
-scheduler=tasks(non-regression check)Single-core builds go through
runtime_rp2040_flashsafe_single.go, whichis a plain
interrupt.Disable()/interrupt.Restore()pair. 100 roundscomplete with no regression.
worker core: 99is the sentinel valuemeaning the worker goroutine was never started (
NumCPU == 1).Symbol placement (
//go:section .ramfuncs)rp2FlashSafeInterruptHandleris placed at0x20001184(RP2040 SRAM),not in the XIP-mapped flash region. The flash-side symbol is the long-branch
thunk LLVM auto-generates for Cortex-M0; it is fetched while XIP is still
enabled, so it is safe.