Repository navigation
rp2: fix atomic section - #13312
Merged
Merged
rp2: fix atomic section#13312
Conversation
|
It works! At least the simple test script runs both processors and reports results form them. Excellent. |
projectgus
reviewed
Jan 1, 2024
dpgeorge
force-pushed
the
rp2-fix-atomic-section
branch
from
January 2, 2024 03:52
d062a6f to
130b4a9
Compare
Member
Author
|
I have changed this to use a new set of mutex functions that also disable/restore interrupts when obtaining/releasing the mutex. |
dpgeorge
force-pushed
the
rp2-fix-atomic-section
branch
from
January 2, 2024 05:52
130b4a9 to
3586586
Compare
Contributor
|
New approach looks good to me! |
Using the multicore lockout feature in the general atomic section makes it much more difficult to get correct. Signed-off-by: Damien George <[email protected]>
These allow entering/exiting a mutex and also disabling/restoring interrupts, in an atomic way. Signed-off-by: Damien George <[email protected]>
Prior to this commit there is a potential deadlock in mp_thread_begin_atomic_section(), when obtaining the atomic_mutex, in the following situation: - main thread calls mp_thread_begin_atomic_section() (for whatever reason, doesn't matter) - the second core is running so the main thread grabs the mutex via the call mp_thread_mutex_lock(&atomic_mutex, 1), and this succeeds - before the main thread has a chance to run save_and_disable_interrupts() a USB IRQ comes in and the main thread jumps off to process this IRQ - that USB processing triggers a call to the dcd_event_handler() wrapper from commit bcbdee2 - that then calls mp_sched_schedule_node() - that then attempts to obtain the atomic section, calling mp_thread_begin_atomic_section() - that call then blocks trying to obtain atomic_mutex - core0 is now deadlocked on itself, because the main thread has the mutex but the IRQ handler (which preempted the main thread) is blocked waiting for the mutex, which will never be free The solution in this commit is to use mutex enter/exit functions that also atomically disable/restore interrupts. Fixes issues micropython#12980 and micropython#13288. Signed-off-by: Damien George <[email protected]>
dpgeorge
force-pushed
the
rp2-fix-atomic-section
branch
from
January 3, 2024 05:00
3586586 to
dc2a4e3
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This is an attempt to fix #12980 and #13288.
There are two fixes to theMICROPY_BEGIN_ATOMIC_SECTION/MICROPY_END_ATOMIC_SECTIONon the rp2 port:Use a recursive mutex to prevent deadlocks on the same thread, in the case an IRQ races against the thread level (on the same core) when acquiring the atomic section (mutex may be taken but IRQs not yet disabled, then the IRQ waits forever trying to acquire the mutux).Unlock the mutex if it was locked, not ifcore1_entryis still non-null, to fix the case where the second core finishes (and setscore1_entrytoNULL) while the first core is in the middle of an atomic operation.Edit: the two changes are now: