From b6b39bff47b5ff1ffc322a6e795d57451d4215a8 Mon Sep 17 00:00:00 2001 From: Damien George Date: Tue, 4 May 2021 23:56:43 +1000 Subject: [PATCH] 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 --- lib/utils/pyexec.c | 4 +-- py/gc.c | 45 ++++++++++++++-------------- py/modmicropython.c | 4 +-- py/modthread.c | 3 ++ py/mpstate.h | 4 ++- tests/thread/thread_heap_lock.py | 26 ++++++++++++++++ tests/thread/thread_heap_lock.py.exp | 1 + 7 files changed, 59 insertions(+), 28 deletions(-) create mode 100644 tests/thread/thread_heap_lock.py create mode 100644 tests/thread/thread_heap_lock.py.exp diff --git a/lib/utils/pyexec.c b/lib/utils/pyexec.c index d1e955d65b..4446b36b61 100644 --- a/lib/utils/pyexec.c +++ b/lib/utils/pyexec.c @@ -588,8 +588,8 @@ friendly_repl_reset: // If the GC is locked at this point there is no way out except a reset, // so force the GC to be unlocked to help the user debug what went wrong. - if (MP_STATE_MEM(gc_lock_depth) != 0) { - MP_STATE_MEM(gc_lock_depth) = 0; + if (MP_STATE_THREAD(gc_lock_depth) != 0) { + MP_STATE_THREAD(gc_lock_depth) = 0; } vstr_reset(&line); diff --git a/py/gc.c b/py/gc.c index 53a0d9da4a..88adf2045e 100644 --- a/py/gc.c +++ b/py/gc.c @@ -150,7 +150,7 @@ void gc_init(void *start, void *end) { MP_STATE_MEM(gc_last_free_atb_index) = 0; // unlock the GC - MP_STATE_MEM(gc_lock_depth) = 0; + MP_STATE_THREAD(gc_lock_depth) = 0; // allow auto collection MP_STATE_MEM(gc_auto_collect_enabled) = 1; @@ -174,19 +174,20 @@ void gc_init(void *start, void *end) { } void gc_lock(void) { - GC_ENTER(); - MP_STATE_MEM(gc_lock_depth)++; - GC_EXIT(); + // This does not need to be atomic or have the GC mutex because: + // - each thread has its own gc_lock_depth so there are no races between threads; + // - a hard interrupt will only change gc_lock_depth during its execution, and + // upon return will restore the value of gc_lock_depth. + MP_STATE_THREAD(gc_lock_depth)++; } void gc_unlock(void) { - GC_ENTER(); - MP_STATE_MEM(gc_lock_depth)--; - GC_EXIT(); + // This does not need to be atomic, See comment above in gc_lock. + MP_STATE_THREAD(gc_lock_depth)--; } bool gc_is_locked(void) { - return MP_STATE_MEM(gc_lock_depth) != 0; + return MP_STATE_THREAD(gc_lock_depth) != 0; } // ptr should be of type void* @@ -320,7 +321,7 @@ STATIC void gc_sweep(void) { void gc_collect_start(void) { GC_ENTER(); - MP_STATE_MEM(gc_lock_depth)++; + MP_STATE_THREAD(gc_lock_depth)++; #if MICROPY_GC_ALLOC_THRESHOLD MP_STATE_MEM(gc_alloc_amount) = 0; #endif @@ -360,13 +361,13 @@ void gc_collect_end(void) { gc_deal_with_stack_overflow(); gc_sweep(); MP_STATE_MEM(gc_last_free_atb_index) = 0; - MP_STATE_MEM(gc_lock_depth)--; + MP_STATE_THREAD(gc_lock_depth)--; GC_EXIT(); } void gc_sweep_all(void) { GC_ENTER(); - MP_STATE_MEM(gc_lock_depth)++; + MP_STATE_THREAD(gc_lock_depth)++; MP_STATE_MEM(gc_stack_overflow) = 0; gc_collect_end(); } @@ -445,14 +446,13 @@ void *gc_alloc(size_t n_bytes, unsigned int alloc_flags) { return NULL; } - GC_ENTER(); - // check if GC is locked - if (MP_STATE_MEM(gc_lock_depth) > 0) { - GC_EXIT(); + if (MP_STATE_THREAD(gc_lock_depth) > 0) { return NULL; } + GC_ENTER(); + size_t i; size_t end_block; size_t start_block; @@ -573,13 +573,13 @@ void *gc_alloc_with_finaliser(mp_uint_t n_bytes) { // force the freeing of a piece of memory // TODO: freeing here does not call finaliser void gc_free(void *ptr) { - GC_ENTER(); - if (MP_STATE_MEM(gc_lock_depth) > 0) { + if (MP_STATE_THREAD(gc_lock_depth) > 0) { // TODO how to deal with this error? - GC_EXIT(); return; } + GC_ENTER(); + DEBUG_printf("gc_free(%p)\n", ptr); if (ptr == NULL) { @@ -674,15 +674,14 @@ void *gc_realloc(void *ptr_in, size_t n_bytes, bool allow_move) { return NULL; } + if (MP_STATE_THREAD(gc_lock_depth) > 0) { + return NULL; + } + void *ptr = ptr_in; GC_ENTER(); - if (MP_STATE_MEM(gc_lock_depth) > 0) { - GC_EXIT(); - return NULL; - } - // get the GC block number corresponding to this pointer assert(VERIFY_PTR(ptr)); size_t block = BLOCK_FROM_PTR(ptr); diff --git a/py/modmicropython.c b/py/modmicropython.c index f7eadf79bd..180f7f186c 100644 --- a/py/modmicropython.c +++ b/py/modmicropython.c @@ -130,13 +130,13 @@ STATIC MP_DEFINE_CONST_FUN_OBJ_0(mp_micropython_heap_lock_obj, mp_micropython_he STATIC mp_obj_t mp_micropython_heap_unlock(void) { gc_unlock(); - return MP_OBJ_NEW_SMALL_INT(MP_STATE_MEM(gc_lock_depth)); + return MP_OBJ_NEW_SMALL_INT(MP_STATE_THREAD(gc_lock_depth)); } STATIC MP_DEFINE_CONST_FUN_OBJ_0(mp_micropython_heap_unlock_obj, mp_micropython_heap_unlock); #if MICROPY_PY_MICROPYTHON_HEAP_LOCKED STATIC mp_obj_t mp_micropython_heap_locked(void) { - return MP_OBJ_NEW_SMALL_INT(MP_STATE_MEM(gc_lock_depth)); + return MP_OBJ_NEW_SMALL_INT(MP_STATE_THREAD(gc_lock_depth)); } STATIC MP_DEFINE_CONST_FUN_OBJ_0(mp_micropython_heap_locked_obj, mp_micropython_heap_locked); #endif diff --git a/py/modthread.c b/py/modthread.c index 1306dc642b..64fbb3f198 100644 --- a/py/modthread.c +++ b/py/modthread.c @@ -171,6 +171,9 @@ STATIC void *thread_entry(void *args_in) { mp_pystack_init(mini_pystack, &mini_pystack[128]); #endif + // The GC starts off unlocked on this thread. + ts.gc_lock_depth = 0; + // set locals and globals from the calling context mp_locals_set(args->dict_locals); mp_globals_set(args->dict_globals); diff --git a/py/mpstate.h b/py/mpstate.h index 2519c77e2d..a0e3d4f143 100644 --- a/py/mpstate.h +++ b/py/mpstate.h @@ -80,7 +80,6 @@ typedef struct _mp_state_mem_t { int gc_stack_overflow; MICROPY_GC_STACK_ENTRY_TYPE gc_stack[MICROPY_ALLOC_GC_STACK_SIZE]; - uint16_t gc_lock_depth; // This variable controls auto garbage collection. If set to 0 then the // GC won't automatically run when gc_alloc can't find enough blocks. But @@ -253,6 +252,9 @@ typedef struct _mp_state_thread_t { uint8_t *pystack_cur; #endif + // Locking of the GC is done per thread. + uint16_t gc_lock_depth; + //////////////////////////////////////////////////////////// // START ROOT POINTER SECTION // Everything that needs GC scanning must start here, and diff --git a/tests/thread/thread_heap_lock.py b/tests/thread/thread_heap_lock.py new file mode 100644 index 0000000000..2837e0f366 --- /dev/null +++ b/tests/thread/thread_heap_lock.py @@ -0,0 +1,26 @@ +# test interaction of micropython.heap_lock with threads + +import _thread, micropython + +lock1 = _thread.allocate_lock() +lock2 = _thread.allocate_lock() + + +def thread_entry(): + lock1.acquire() + print([1, 2, 3]) + lock2.release() + + +lock1.acquire() +lock2.acquire() + +_thread.start_new_thread(thread_entry, ()) + +micropython.heap_lock() +lock1.release() +lock2.acquire() +micropython.heap_unlock() + +lock1.release() +lock2.release() diff --git a/tests/thread/thread_heap_lock.py.exp b/tests/thread/thread_heap_lock.py.exp new file mode 100644 index 0000000000..b5d8bb58d9 --- /dev/null +++ b/tests/thread/thread_heap_lock.py.exp @@ -0,0 +1 @@ +[1, 2, 3]