-
Notifications
You must be signed in to change notification settings - Fork 983
fix(rp2350): add software spinlocks #5034
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
base: dev
Are you sure you want to change the base?
Conversation
Oh, whoops. My go fmt extension has been flaking out on me. Will have the missing rp2040 imports updated in a moment. Here's the lock/unlock disassembled output with inlining disabled:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I support the switch to atomic instructions, but if you need something that works right away, RP2350-E2 mentions that some spinlocks are not affected:
The following SIO spinlocks can be used normally because they don’t alias with writable registers: 5, 6, 7,
10, 11, and 18 through 31. Some of the other lock addresses may be used safely depending on which of
the high-addressed SIO registers are in use.
Locks 18 through 24 alias with some read-only TMDS encoder registers, which is safe as only writes are
mis-decoded.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See my comment below.
Also,
Another thing I noticed in that section, they also always disable interrupts when the spinlocks are being held, we may want to do the same: [...]
The runtime does this in various places if needed. The spinlock implementation doesn't need to disable interrupts too.
Thank you for tracking those down. I figured there would probably be some, but I couldn't find them |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Other than a nit, this looks good to me. @aykevl WDYT?
src/runtime/runtime_rp2.go
Outdated
printLock = spinLock{id: 0} | ||
schedulerLock = spinLock{id: 1} | ||
atomicsLock = spinLock{id: 2} | ||
futexLock = spinLock{id: 3} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
id
is an implementation detail of rp2040 spinlocks, whereas on rp2350 id
s have some meaning but are unusued. I suggest moving the spinlock variables to the rpXXXX.go files and avoid the id
field on rp2350.
src/runtime/runtime_rp2350.go
Outdated
|
||
type spinLock struct { | ||
atomic.Uint32 | ||
id uint8 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Delete field.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That one's still needed for compatibility with how the rp2040 hardware spinlocks are initialized here:
tinygo/src/runtime/runtime_rp2.go
Lines 295 to 298 in 109e076
printLock = spinLock{id: 20} | |
schedulerLock = spinLock{id: 21} | |
atomicsLock = spinLock{id: 22} | |
futexLock = spinLock{id: 23} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indeed. Above, I suggested moving that var ()
block to the runtime_rp2xxx.go
files to get rid of that dependency.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah yep, I missed that
After doing some testing, I think this might not actually be locking properly. Going to do some more thorough testing |
Yes, this looks good. @mikesmitty can you apply the changes proposed by @eliasnaur? Also, you may want to rebase on the dev branch to hopefully fix the assert-test-linux CI failure. |
1f2642c
to
cee4537
Compare
Sure, here's the rebase. I haven't had time to test or diagnose it yet, been stuck working on a project that's consumed all my free time, but I'm pretty sure it's not functioning properly as confirmed by that CI failure |
Hmm, no I guess it is actually locking. I'm not sure what's happening with that CI test though |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thank you!
Restarting CI, these are probably flaky tests. |
|
||
// #include <stdint.h> | ||
// | ||
// static void spinlock_lock(void *lock) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What was wrong with the sync/atomic
operations? Custom assembly for just one target (rp2350) seems to me like a maintenance headache for little gain.
As it turns out, the RP2350 has hardware spinlocks that can be unlocked by writes to nearby addresses, the lower spinlocks currently in use in TinyGo happen to be unlocked by writes to the doorbell interrupt registers used to signal between cores, very possibly leading to some unexpected unlocks. This was not corrected in the A3 or A4 steppings and instead software spinlocks are used by default on RP2350 in pico-sdk:
https://www.raspberrypi.com/documentation/pico-sdk/hardware.html#group_hardware_sync
Another thing I noticed in that section, they also always disable interrupts when the spinlocks are being held, we may want to do the same:
These are the software spinlock macros ported over:
https://github.com/raspberrypi/pico-sdk/blob/2.2.0/src/rp2_common/hardware_sync_spin_lock/include/hardware/sync/spin_lock.h#L112
https://github.com/raspberrypi/pico-sdk/blob/2.2.0/src/rp2_common/hardware_sync_spin_lock/include/hardware/sync/spin_lock.h#L197