Skip to content

Commit c2b46d7

Browse files
jimmojimmo
committed
py/scheduler: Fix scheduler race with pending exception.
The optimisation that allows a single check in the VM for either a pending exception or non-empty scheduler queue doesn't work when threading is enabled, as one thread can clear the sched_state if it has no pending exception, meaning the thread with the pending exception will never see it. This removes that optimisation for threaded builds. Signed-off-by: Jim Mussared <jim.mussared@gmail.com>
1 parent 99c2589 commit c2b46d7

File tree

5 files changed

+62
-62
lines changed

5 files changed

+62
-62
lines changed

‎py/profile.c

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -301,6 +301,7 @@ STATIC mp_obj_t mp_prof_callback_invoke(mp_obj_t callback, prof_callback_args_t
301301
if (MP_STATE_THREAD(mp_pending_exception) != MP_OBJ_NULL) {
302302
mp_handle_pending(true);
303303
}
304+
304305
return top;
305306
}
306307

‎py/runtime.c

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -64,7 +64,7 @@ MP_REGISTER_MODULE(MP_QSTR___main__, mp_module___main__);
6464
void mp_init(void) {
6565
qstr_init();
6666

67-
// no pending exceptions to start with
67+
// No pending exceptions (on the main thread) to start with
6868
MP_STATE_THREAD(mp_pending_exception) = MP_OBJ_NULL;
6969
#if MICROPY_ENABLE_SCHEDULER
7070
#if MICROPY_SCHEDULER_STATIC_NODES

‎py/runtime.h

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -76,11 +76,11 @@ void mp_deinit(void);
7676
void mp_sched_exception(mp_obj_t exc);
7777
void mp_sched_keyboard_interrupt(void);
7878
void mp_handle_pending(bool raise_exc);
79-
void mp_handle_pending_tail(mp_uint_t atomic_state);
8079

8180
#if MICROPY_ENABLE_SCHEDULER
8281
void mp_sched_lock(void);
8382
void mp_sched_unlock(void);
83+
void mp_sched_run_pending(void);
8484
#define mp_sched_num_pending() (MP_STATE_VM(sched_len))
8585
bool mp_sched_schedule(mp_obj_t function, mp_obj_t arg);
8686
bool mp_sched_schedule_node(mp_sched_node_t *node, mp_sched_callback_t callback);

‎py/scheduler.c

Lines changed: 39 additions & 34 deletions
Original file line numberDiff line numberDiff line change
@@ -32,7 +32,11 @@
3232
// sources such as interrupts and UNIX signal handlers).
3333
void MICROPY_WRAP_MP_SCHED_EXCEPTION(mp_sched_exception)(mp_obj_t exc) {
3434
MP_STATE_MAIN_THREAD(mp_pending_exception) = exc;
35-
#if MICROPY_ENABLE_SCHEDULER
35+
36+
#if MICROPY_ENABLE_SCHEDULER && !MICROPY_PY_THREAD
37+
// Optimisation for the case where we have scheduler but no threading.
38+
// Allows the VM to do a single check to exclude both pending exception
39+
// and queued tasks.
3640
if (MP_STATE_VM(sched_state) == MP_SCHED_IDLE) {
3741
MP_STATE_VM(sched_state) = MP_SCHED_PENDING;
3842
}
@@ -62,33 +66,19 @@ static inline bool mp_sched_empty(void) {
6266
return mp_sched_num_pending() == 0;
6367
}
6468

65-
// A variant of this is inlined in the VM at the pending exception check
66-
void mp_handle_pending(bool raise_exc) {
67-
if (MP_STATE_VM(sched_state) == MP_SCHED_PENDING) {
68-
mp_uint_t atomic_state = MICROPY_BEGIN_ATOMIC_SECTION();
69-
// Re-check state is still pending now that we're in the atomic section.
70-
if (MP_STATE_VM(sched_state) == MP_SCHED_PENDING) {
71-
mp_obj_t obj = MP_STATE_THREAD(mp_pending_exception);
72-
if (obj != MP_OBJ_NULL) {
73-
MP_STATE_THREAD(mp_pending_exception) = MP_OBJ_NULL;
74-
if (!mp_sched_num_pending()) {
75-
MP_STATE_VM(sched_state) = MP_SCHED_IDLE;
76-
}
77-
if (raise_exc) {
78-
MICROPY_END_ATOMIC_SECTION(atomic_state);
79-
nlr_raise(obj);
80-
}
81-
}
82-
mp_handle_pending_tail(atomic_state);
83-
} else {
84-
MICROPY_END_ATOMIC_SECTION(atomic_state);
85-
}
69+
// This function should only be called by mp_handle_pending, or by the VM's
70+
// inlined version, after checking that the scheduler state is pending.
71+
void mp_sched_run_pending(void) {
72+
mp_uint_t atomic_state = MICROPY_BEGIN_ATOMIC_SECTION();
73+
if (MP_STATE_VM(sched_state) != MP_SCHED_PENDING) {
74+
// Something else (e.g. hard IRQ) locked the scheduler while we
75+
// acquired the lock.
76+
MICROPY_END_ATOMIC_SECTION(atomic_state);
77+
return;
8678
}
87-
}
8879

89-
// This function should only be called by mp_handle_pending,
90-
// or by the VM's inlined version of that function.
91-
void mp_handle_pending_tail(mp_uint_t atomic_state) {
80+
// Equivalent to mp_sched_lock(), but we're already in the atomic
81+
// section and know that we're pending.
9282
MP_STATE_VM(sched_state) = MP_SCHED_LOCKED;
9383

9484
#if MICROPY_SCHEDULER_STATIC_NODES
@@ -118,14 +108,21 @@ void mp_handle_pending_tail(mp_uint_t atomic_state) {
118108
MICROPY_END_ATOMIC_SECTION(atomic_state);
119109
}
120110

111+
// Restore MP_STATE_VM(sched_state) to idle (or pending if there are still
112+
// tasks in the queue).
121113
mp_sched_unlock();
122114
}
123115

116+
// Locking the scheduler prevents tasks from executing (does not prevent new
117+
// tasks from being added). We lock the scheduler while executing scheduled
118+
// tasks and also in hard interrupts or GC finalisers.
124119
void mp_sched_lock(void) {
125120
mp_uint_t atomic_state = MICROPY_BEGIN_ATOMIC_SECTION();
126121
if (MP_STATE_VM(sched_state) < 0) {
122+
// Already locked, increment lock (recursive lock).
127123
--MP_STATE_VM(sched_state);
128124
} else {
125+
// Pending or idle.
129126
MP_STATE_VM(sched_state) = MP_SCHED_LOCKED;
130127
}
131128
MICROPY_END_ATOMIC_SECTION(atomic_state);
@@ -135,12 +132,17 @@ void mp_sched_unlock(void) {
135132
mp_uint_t atomic_state = MICROPY_BEGIN_ATOMIC_SECTION();
136133
assert(MP_STATE_VM(sched_state) < 0);
137134
if (++MP_STATE_VM(sched_state) == 0) {
138-
// vm became unlocked
139-
if (MP_STATE_THREAD(mp_pending_exception) != MP_OBJ_NULL
135+
// Scheduler became unlocked. Check if there are still tasks in the
136+
// queue and set sched_state accordingly.
137+
if (
138+
#if !MICROPY_PY_THREAD
139+
// See optimisation in mp_sched_exception.
140+
MP_STATE_THREAD(mp_pending_exception) != MP_OBJ_NULL ||
141+
#endif
140142
#if MICROPY_SCHEDULER_STATIC_NODES
141-
|| MP_STATE_VM(sched_head) != NULL
143+
MP_STATE_VM(sched_head) != NULL ||
142144
#endif
143-
|| mp_sched_num_pending()) {
145+
mp_sched_num_pending()) {
144146
MP_STATE_VM(sched_state) = MP_SCHED_PENDING;
145147
} else {
146148
MP_STATE_VM(sched_state) = MP_SCHED_IDLE;
@@ -196,9 +198,9 @@ bool mp_sched_schedule_node(mp_sched_node_t *node, mp_sched_callback_t callback)
196198
}
197199
#endif
198200

199-
#else // MICROPY_ENABLE_SCHEDULER
201+
#endif // MICROPY_ENABLE_SCHEDULER
200202

201-
// A variant of this is inlined in the VM at the pending exception check
203+
// This is also inlined in the VM at the pending exception check.
202204
void mp_handle_pending(bool raise_exc) {
203205
if (MP_STATE_THREAD(mp_pending_exception) != MP_OBJ_NULL) {
204206
mp_obj_t obj = MP_STATE_THREAD(mp_pending_exception);
@@ -207,6 +209,9 @@ void mp_handle_pending(bool raise_exc) {
207209
nlr_raise(obj);
208210
}
209211
}
212+
#if MICROPY_ENABLE_SCHEDULER
213+
if (MP_STATE_VM(sched_state) == MP_SCHED_PENDING) {
214+
mp_sched_run_pending();
215+
}
216+
#endif
210217
}
211-
212-
#endif // MICROPY_ENABLE_SCHEDULER

‎py/vm.c

Lines changed: 20 additions & 26 deletions
Original file line numberDiff line numberDiff line change
@@ -1297,36 +1297,30 @@ unwind_jump:;
12971297
pending_exception_check:
12981298
MICROPY_VM_HOOK_LOOP
12991299

1300-
#if MICROPY_ENABLE_SCHEDULER
1301-
// This is an inlined variant of mp_handle_pending
1302-
if (MP_STATE_VM(sched_state) == MP_SCHED_PENDING) {
1303-
mp_uint_t atomic_state = MICROPY_BEGIN_ATOMIC_SECTION();
1304-
// Re-check state is still pending now that we're in the atomic section.
1305-
if (MP_STATE_VM(sched_state) == MP_SCHED_PENDING) {
1300+
// This is an inlined, always-raising variant of mp_handle_pending.
1301+
1302+
#if MICROPY_ENABLE_SCHEDULER && !MICROPY_PY_THREAD
1303+
if (MP_STATE_VM(sched_state) == MP_SCHED_PENDING)
1304+
#endif
1305+
{
1306+
if (MP_STATE_THREAD(mp_pending_exception) != MP_OBJ_NULL) {
13061307
MARK_EXC_IP_SELECTIVE();
13071308
mp_obj_t obj = MP_STATE_THREAD(mp_pending_exception);
1308-
if (obj != MP_OBJ_NULL) {
1309-
MP_STATE_THREAD(mp_pending_exception) = MP_OBJ_NULL;
1310-
if (!mp_sched_num_pending()) {
1311-
MP_STATE_VM(sched_state) = MP_SCHED_IDLE;
1312-
}
1313-
MICROPY_END_ATOMIC_SECTION(atomic_state);
1314-
RAISE(obj);
1315-
}
1316-
mp_handle_pending_tail(atomic_state);
1317-
} else {
1318-
MICROPY_END_ATOMIC_SECTION(atomic_state);
1309+
MP_STATE_THREAD(mp_pending_exception) = MP_OBJ_NULL;
1310+
// Note (scheduler+non-threading optimisation):
1311+
// MP_STATE_VM(sched_state) will be cleared the next
1312+
// time the scheduler code below runs.
1313+
RAISE(obj);
13191314
}
1315+
#if MICROPY_ENABLE_SCHEDULER
1316+
#if MICROPY_PY_THREAD
1317+
if (MP_STATE_VM(sched_state) == MP_SCHED_PENDING)
1318+
#endif
1319+
{
1320+
mp_sched_run_pending();
1321+
}
1322+
#endif
13201323
}
1321-
#else
1322-
// This is an inlined variant of mp_handle_pending
1323-
if (MP_STATE_THREAD(mp_pending_exception) != MP_OBJ_NULL) {
1324-
MARK_EXC_IP_SELECTIVE();
1325-
mp_obj_t obj = MP_STATE_THREAD(mp_pending_exception);
1326-
MP_STATE_THREAD(mp_pending_exception) = MP_OBJ_NULL;
1327-
RAISE(obj);
1328-
}
1329-
#endif
13301324

13311325
#if MICROPY_PY_THREAD_GIL
13321326
#if MICROPY_PY_THREAD_GIL_VM_DIVISOR

0 commit comments

Comments
 (0)