Skip to content

Commit b6b39bf

Browse files
dpgeorgedpgeorge
committed
py/gc: Make gc_lock_depth have a count per thread.
This commit makes gc_lock_depth have one counter per thread, instead of one global counter. This makes threads properly independent with respect to the GC, in particular threads can now independently lock the GC for themselves without locking it for other threads. It also means a given thread can run a hard IRQ without temporarily locking the GC for all other threads and potentially making them have MemoryError exceptions at random locations (this really only occurs on MCUs with multiple cores and no GIL, eg on the rp2 port). The commit also removes protection of the GC lock/unlock functions, which is no longer needed when the counter is per thread (and this also fixes the cas where a hard IRQ calling gc_lock() may stall waiting for the mutex). It also puts the check for `gc_lock_depth > 0` outside the GC mutex in gc_alloc, gc_realloc and gc_free, to potentially prevent a hard IRQ from waiting on a mutex if it does attempt to allocate heap memory (and putting the check outside the GC mutex is now safe now that there is a gc_lock_depth per thread). Signed-off-by: Damien George <damien@micropython.org>
1 parent d0de162 commit b6b39bf

7 files changed

Lines changed: 59 additions & 28 deletions

File tree

‎lib/utils/pyexec.c‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -588,8 +588,8 @@ int pyexec_friendly_repl(void) {
588588

589589
// If the GC is locked at this point there is no way out except a reset,
590590
// so force the GC to be unlocked to help the user debug what went wrong.
591-
if (MP_STATE_MEM(gc_lock_depth) != 0) {
592-
MP_STATE_MEM(gc_lock_depth) = 0;
591+
if (MP_STATE_THREAD(gc_lock_depth) != 0) {
592+
MP_STATE_THREAD(gc_lock_depth) = 0;
593593
}
594594

595595
vstr_reset(&line);

‎py/gc.c‎

Lines changed: 22 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -150,7 +150,7 @@ void gc_init(void *start, void *end) {
150150
MP_STATE_MEM(gc_last_free_atb_index) = 0;
151151

152152
// unlock the GC
153-
MP_STATE_MEM(gc_lock_depth) = 0;
153+
MP_STATE_THREAD(gc_lock_depth) = 0;
154154

155155
// allow auto collection
156156
MP_STATE_MEM(gc_auto_collect_enabled) = 1;
@@ -174,19 +174,20 @@ void gc_init(void *start, void *end) {
174174
}
175175

176176
void gc_lock(void) {
177-
GC_ENTER();
178-
MP_STATE_MEM(gc_lock_depth)++;
179-
GC_EXIT();
177+
// This does not need to be atomic or have the GC mutex because:
178+
// - each thread has its own gc_lock_depth so there are no races between threads;
179+
// - a hard interrupt will only change gc_lock_depth during its execution, and
180+
// upon return will restore the value of gc_lock_depth.
181+
MP_STATE_THREAD(gc_lock_depth)++;
180182
}
181183

182184
void gc_unlock(void) {
183-
GC_ENTER();
184-
MP_STATE_MEM(gc_lock_depth)--;
185-
GC_EXIT();
185+
// This does not need to be atomic, See comment above in gc_lock.
186+
MP_STATE_THREAD(gc_lock_depth)--;
186187
}
187188

188189
bool gc_is_locked(void) {
189-
return MP_STATE_MEM(gc_lock_depth) != 0;
190+
return MP_STATE_THREAD(gc_lock_depth) != 0;
190191
}
191192

192193
// ptr should be of type void*
@@ -320,7 +321,7 @@ STATIC void gc_sweep(void) {
320321

321322
void gc_collect_start(void) {
322323
GC_ENTER();
323-
MP_STATE_MEM(gc_lock_depth)++;
324+
MP_STATE_THREAD(gc_lock_depth)++;
324325
#if MICROPY_GC_ALLOC_THRESHOLD
325326
MP_STATE_MEM(gc_alloc_amount) = 0;
326327
#endif
@@ -360,13 +361,13 @@ void gc_collect_end(void) {
360361
gc_deal_with_stack_overflow();
361362
gc_sweep();
362363
MP_STATE_MEM(gc_last_free_atb_index) = 0;
363-
MP_STATE_MEM(gc_lock_depth)--;
364+
MP_STATE_THREAD(gc_lock_depth)--;
364365
GC_EXIT();
365366
}
366367

367368
void gc_sweep_all(void) {
368369
GC_ENTER();
369-
MP_STATE_MEM(gc_lock_depth)++;
370+
MP_STATE_THREAD(gc_lock_depth)++;
370371
MP_STATE_MEM(gc_stack_overflow) = 0;
371372
gc_collect_end();
372373
}
@@ -445,14 +446,13 @@ void *gc_alloc(size_t n_bytes, unsigned int alloc_flags) {
445446
return NULL;
446447
}
447448

448-
GC_ENTER();
449-
450449
// check if GC is locked
451-
if (MP_STATE_MEM(gc_lock_depth) > 0) {
452-
GC_EXIT();
450+
if (MP_STATE_THREAD(gc_lock_depth) > 0) {
453451
return NULL;
454452
}
455453

454+
GC_ENTER();
455+
456456
size_t i;
457457
size_t end_block;
458458
size_t start_block;
@@ -573,13 +573,13 @@ void *gc_alloc_with_finaliser(mp_uint_t n_bytes) {
573573
// force the freeing of a piece of memory
574574
// TODO: freeing here does not call finaliser
575575
void gc_free(void *ptr) {
576-
GC_ENTER();
577-
if (MP_STATE_MEM(gc_lock_depth) > 0) {
576+
if (MP_STATE_THREAD(gc_lock_depth) > 0) {
578577
// TODO how to deal with this error?
579-
GC_EXIT();
580578
return;
581579
}
582580

581+
GC_ENTER();
582+
583583
DEBUG_printf("gc_free(%p)\n", ptr);
584584

585585
if (ptr == NULL) {
@@ -674,15 +674,14 @@ void *gc_realloc(void *ptr_in, size_t n_bytes, bool allow_move) {
674674
return NULL;
675675
}
676676

677+
if (MP_STATE_THREAD(gc_lock_depth) > 0) {
678+
return NULL;
679+
}
680+
677681
void *ptr = ptr_in;
678682

679683
GC_ENTER();
680684

681-
if (MP_STATE_MEM(gc_lock_depth) > 0) {
682-
GC_EXIT();
683-
return NULL;
684-
}
685-
686685
// get the GC block number corresponding to this pointer
687686
assert(VERIFY_PTR(ptr));
688687
size_t block = BLOCK_FROM_PTR(ptr);

‎py/modmicropython.c‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -130,13 +130,13 @@ STATIC MP_DEFINE_CONST_FUN_OBJ_0(mp_micropython_heap_lock_obj, mp_micropython_he
130130

131131
STATIC mp_obj_t mp_micropython_heap_unlock(void) {
132132
gc_unlock();
133-
return MP_OBJ_NEW_SMALL_INT(MP_STATE_MEM(gc_lock_depth));
133+
return MP_OBJ_NEW_SMALL_INT(MP_STATE_THREAD(gc_lock_depth));
134134
}
135135
STATIC MP_DEFINE_CONST_FUN_OBJ_0(mp_micropython_heap_unlock_obj, mp_micropython_heap_unlock);
136136

137137
#if MICROPY_PY_MICROPYTHON_HEAP_LOCKED
138138
STATIC mp_obj_t mp_micropython_heap_locked(void) {
139-
return MP_OBJ_NEW_SMALL_INT(MP_STATE_MEM(gc_lock_depth));
139+
return MP_OBJ_NEW_SMALL_INT(MP_STATE_THREAD(gc_lock_depth));
140140
}
141141
STATIC MP_DEFINE_CONST_FUN_OBJ_0(mp_micropython_heap_locked_obj, mp_micropython_heap_locked);
142142
#endif

‎py/modthread.c‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -171,6 +171,9 @@ STATIC void *thread_entry(void *args_in) {
171171
mp_pystack_init(mini_pystack, &mini_pystack[128]);
172172
#endif
173173

174+
// The GC starts off unlocked on this thread.
175+
ts.gc_lock_depth = 0;
176+
174177
// set locals and globals from the calling context
175178
mp_locals_set(args->dict_locals);
176179
mp_globals_set(args->dict_globals);

‎py/mpstate.h‎

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -80,7 +80,6 @@ typedef struct _mp_state_mem_t {
8080

8181
int gc_stack_overflow;
8282
MICROPY_GC_STACK_ENTRY_TYPE gc_stack[MICROPY_ALLOC_GC_STACK_SIZE];
83-
uint16_t gc_lock_depth;
8483

8584
// This variable controls auto garbage collection. If set to 0 then the
8685
// GC won't automatically run when gc_alloc can't find enough blocks. But
@@ -253,6 +252,9 @@ typedef struct _mp_state_thread_t {
253252
uint8_t *pystack_cur;
254253
#endif
255254

255+
// Locking of the GC is done per thread.
256+
uint16_t gc_lock_depth;
257+
256258
////////////////////////////////////////////////////////////
257259
// START ROOT POINTER SECTION
258260
// Everything that needs GC scanning must start here, and

‎tests/thread/thread_heap_lock.py‎

Lines changed: 26 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,26 @@
1+
# test interaction of micropython.heap_lock with threads
2+
3+
import _thread, micropython
4+
5+
lock1 = _thread.allocate_lock()
6+
lock2 = _thread.allocate_lock()
7+
8+
9+
def thread_entry():
10+
lock1.acquire()
11+
print([1, 2, 3])
12+
lock2.release()
13+
14+
15+
lock1.acquire()
16+
lock2.acquire()
17+
18+
_thread.start_new_thread(thread_entry, ())
19+
20+
micropython.heap_lock()
21+
lock1.release()
22+
lock2.acquire()
23+
micropython.heap_unlock()
24+
25+
lock1.release()
26+
lock2.release()

‎tests/thread/thread_heap_lock.py.exp‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1 @@
1+
[1, 2, 3]

0 commit comments

Comments
 (0)