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>

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);

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);

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

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);

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

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()

tests/thread/thread_heap_lock.py.exp

@@ -0,0 +1 @@
+[1, 2, 3]