Use shared memory lock for threads generated from same module (bytecodealliance#1960)

eloparco · web-flow · commit 216dc43ab4fc · 2023-02-16T11:54:19.000+08:00
Multiple threads generated from the same module should use the same lock to protect the atomic operations. Before this PR, each thread used a different lock to protect atomic operations (e.g. atomic add), making the lock ineffective. Fix bytecodealliance#1958.
diff --git a/core/iwasm/common/wasm_shared_memory.c b/core/iwasm/common/wasm_shared_memory.c
@@ -172,6 +172,7 @@ shared_memory_dec_reference(WASMModuleCommon *module)
             bh_list_remove(shared_memory_list, node);
             os_mutex_unlock(&shared_memory_list_lock);
 
+            os_mutex_destroy(&node->shared_mem_lock);
             os_mutex_destroy(&node->lock);
             wasm_runtime_free(node);
         }
@@ -200,7 +201,14 @@ shared_memory_set_memory_inst(WASMModuleCommon *module,
     node->module = module;
     node->memory_inst = memory;
     node->ref_count = 1;
+
+    if (os_mutex_init(&node->shared_mem_lock) != 0) {
+        wasm_runtime_free(node);
+        return NULL;
+    }
+
     if (os_mutex_init(&node->lock) != 0) {
+        os_mutex_destroy(&node->shared_mem_lock);
         wasm_runtime_free(node);
         return NULL;
     }
diff --git a/core/iwasm/common/wasm_shared_memory.h b/core/iwasm/common/wasm_shared_memory.h
@@ -26,6 +26,8 @@ typedef struct WASMSharedMemNode {
     WASMModuleCommon *module;
     /* The memory information */
     WASMMemoryInstanceCommon *memory_inst;
+    /* Lock used for atomic operations */
+    korp_mutex shared_mem_lock;
 
     /* reference count */
     uint32 ref_count;
diff --git a/core/iwasm/interpreter/wasm.h b/core/iwasm/interpreter/wasm.h
@@ -510,8 +510,8 @@ struct WASMModule {
     uint64 load_size;
 #endif
 
-#if WASM_ENABLE_DEBUG_INTERP != 0                    \
-    || (WASM_ENABLE_FAST_JIT != 0 && WASM_ENABLE_JIT \
+#if WASM_ENABLE_DEBUG_INTERP != 0                         \
+    || (WASM_ENABLE_FAST_JIT != 0 && WASM_ENABLE_JIT != 0 \
         && WASM_ENABLE_LAZY_JIT != 0)
     /**
      * List of instances referred to this module. When source debugging
diff --git a/core/iwasm/interpreter/wasm_interp_classic.c b/core/iwasm/interpreter/wasm_interp_classic.c
diff --git a/core/iwasm/interpreter/wasm_interp_fast.c b/core/iwasm/interpreter/wasm_interp_fast.c
diff --git a/core/iwasm/interpreter/wasm_loader.c b/core/iwasm/interpreter/wasm_loader.c
@@ -3864,8 +3864,8 @@ create_module(char *error_buf, uint32 error_buf_size)
     bh_assert(ret == BH_LIST_SUCCESS);
 #endif
 
-#if WASM_ENABLE_DEBUG_INTERP != 0                    \
-    || (WASM_ENABLE_FAST_JIT != 0 && WASM_ENABLE_JIT \
+#if WASM_ENABLE_DEBUG_INTERP != 0                         \
+    || (WASM_ENABLE_FAST_JIT != 0 && WASM_ENABLE_JIT != 0 \
         && WASM_ENABLE_LAZY_JIT != 0)
     if (os_mutex_init(&module->instance_list_lock) != 0) {
         set_error_buf(error_buf, error_buf_size,
@@ -4253,7 +4253,8 @@ wasm_loader_unload(WASMModule *module)
     if (!module)
         return;
 
-#if WASM_ENABLE_FAST_JIT != 0 && WASM_ENABLE_JIT && WASM_ENABLE_LAZY_JIT != 0
+#if WASM_ENABLE_FAST_JIT != 0 && WASM_ENABLE_JIT != 0 \
+    && WASM_ENABLE_LAZY_JIT != 0
     module->orcjit_stop_compiling = true;
     if (module->llvm_jit_init_thread)
         os_thread_join(module->llvm_jit_init_thread, NULL);
@@ -4274,7 +4275,8 @@ wasm_loader_unload(WASMModule *module)
         aot_destroy_comp_data(module->comp_data);
 #endif
 
-#if WASM_ENABLE_FAST_JIT != 0 && WASM_ENABLE_JIT && WASM_ENABLE_LAZY_JIT != 0
+#if WASM_ENABLE_FAST_JIT != 0 && WASM_ENABLE_JIT != 0 \
+    && WASM_ENABLE_LAZY_JIT != 0
     if (module->tierup_wait_lock_inited) {
         os_mutex_destroy(&module->tierup_wait_lock);
         os_cond_destroy(&module->tierup_wait_cond);
@@ -4403,8 +4405,8 @@ wasm_loader_unload(WASMModule *module)
     }
 #endif
 
-#if WASM_ENABLE_DEBUG_INTERP != 0                    \
-    || (WASM_ENABLE_FAST_JIT != 0 && WASM_ENABLE_JIT \
+#if WASM_ENABLE_DEBUG_INTERP != 0                         \
+    || (WASM_ENABLE_FAST_JIT != 0 && WASM_ENABLE_JIT != 0 \
         && WASM_ENABLE_LAZY_JIT != 0)
     os_mutex_destroy(&module->instance_list_lock);
 #endif
diff --git a/core/iwasm/interpreter/wasm_mini_loader.c b/core/iwasm/interpreter/wasm_mini_loader.c
@@ -2725,7 +2725,8 @@ create_module(char *error_buf, uint32 error_buf_size)
     bh_assert(ret == BH_LIST_SUCCESS);
 #endif
 
-#if WASM_ENABLE_FAST_JIT != 0 && WASM_ENABLE_JIT && WASM_ENABLE_LAZY_JIT != 0
+#if WASM_ENABLE_FAST_JIT != 0 && WASM_ENABLE_JIT != 0 \
+    && WASM_ENABLE_LAZY_JIT != 0
     if (os_mutex_init(&module->instance_list_lock) != 0) {
         set_error_buf(error_buf, error_buf_size,
                       "init instance list lock failed");
@@ -2946,7 +2947,8 @@ wasm_loader_unload(WASMModule *module)
     if (!module)
         return;
 
-#if WASM_ENABLE_FAST_JIT != 0 && WASM_ENABLE_JIT && WASM_ENABLE_LAZY_JIT != 0
+#if WASM_ENABLE_FAST_JIT != 0 && WASM_ENABLE_JIT != 0 \
+    && WASM_ENABLE_LAZY_JIT != 0
     module->orcjit_stop_compiling = true;
     if (module->llvm_jit_init_thread)
         os_thread_join(module->llvm_jit_init_thread, NULL);
@@ -2967,7 +2969,8 @@ wasm_loader_unload(WASMModule *module)
         aot_destroy_comp_data(module->comp_data);
 #endif
 
-#if WASM_ENABLE_FAST_JIT != 0 && WASM_ENABLE_JIT && WASM_ENABLE_LAZY_JIT != 0
+#if WASM_ENABLE_FAST_JIT != 0 && WASM_ENABLE_JIT != 0 \
+    && WASM_ENABLE_LAZY_JIT != 0
     if (module->tierup_wait_lock_inited) {
         os_mutex_destroy(&module->tierup_wait_lock);
         os_cond_destroy(&module->tierup_wait_cond);
@@ -3063,7 +3066,8 @@ wasm_loader_unload(WASMModule *module)
     }
 #endif
 
-#if WASM_ENABLE_FAST_JIT != 0 && WASM_ENABLE_JIT && WASM_ENABLE_LAZY_JIT != 0
+#if WASM_ENABLE_FAST_JIT != 0 && WASM_ENABLE_JIT != 0 \
+    && WASM_ENABLE_LAZY_JIT != 0
     os_mutex_destroy(&module->instance_list_lock);
 #endif
 
diff --git a/core/iwasm/interpreter/wasm_runtime.c b/core/iwasm/interpreter/wasm_runtime.c
@@ -1587,15 +1587,6 @@ wasm_instantiate(WASMModule *module, bool is_sub_inst, uint32 stack_size,
     module_inst->e =
         (WASMModuleInstanceExtra *)((uint8 *)module_inst + extra_info_offset);
 
-#if WASM_ENABLE_SHARED_MEMORY != 0
-    if (os_mutex_init(&module_inst->e->mem_lock) != 0) {
-        set_error_buf(error_buf, error_buf_size,
-                      "create shared memory lock failed");
-        goto fail;
-    }
-    module_inst->e->mem_lock_inited = true;
-#endif
-
 #if WASM_ENABLE_MULTI_MODULE != 0
     module_inst->e->sub_module_inst_list =
         &module_inst->e->sub_module_inst_list_head;
@@ -2159,11 +2150,6 @@ wasm_deinstantiate(WASMModuleInstance *module_inst, bool is_sub_inst)
     }
 #endif
 
-#if WASM_ENABLE_SHARED_MEMORY != 0
-    if (module_inst->e->mem_lock_inited)
-        os_mutex_destroy(&module_inst->e->mem_lock);
-#endif
-
     if (module_inst->e->c_api_func_imports)
         wasm_runtime_free(module_inst->e->c_api_func_imports);
 
diff --git a/core/iwasm/interpreter/wasm_runtime.h b/core/iwasm/interpreter/wasm_runtime.h
@@ -223,12 +223,6 @@ typedef struct WASMModuleInstanceExtra {
     CApiFuncImport *c_api_func_imports;
     RunningMode running_mode;
 
-#if WASM_ENABLE_SHARED_MEMORY != 0
-    /* lock for shared memory atomic operations */
-    korp_mutex mem_lock;
-    bool mem_lock_inited;
-#endif
-
 #if WASM_ENABLE_MULTI_MODULE != 0
     bh_list sub_module_inst_list_head;
     bh_list *sub_module_inst_list;
@@ -240,8 +234,8 @@ typedef struct WASMModuleInstanceExtra {
     uint32 max_aux_stack_used;
 #endif
 
-#if WASM_ENABLE_DEBUG_INTERP != 0                    \
-    || (WASM_ENABLE_FAST_JIT != 0 && WASM_ENABLE_JIT \
+#if WASM_ENABLE_DEBUG_INTERP != 0                         \
+    || (WASM_ENABLE_FAST_JIT != 0 && WASM_ENABLE_JIT != 0 \
         && WASM_ENABLE_LAZY_JIT != 0)
     WASMModuleInstance *next;
 #endif
diff --git a/samples/multi-thread/wasm-apps/CMakeLists.txt b/samples/multi-thread/wasm-apps/CMakeLists.txt
@@ -41,3 +41,6 @@ target_link_libraries(test.wasm)
 
 add_executable(main_thread_exception.wasm  main_thread_exception.c)
 target_link_libraries(main_thread_exception.wasm)
+
+add_executable(main_global_atomic.wasm  main_global_atomic.c)
+target_link_libraries(main_global_atomic.wasm)
diff --git a/samples/multi-thread/wasm-apps/main_global_atomic.c b/samples/multi-thread/wasm-apps/main_global_atomic.c
@@ -0,0 +1,48 @@
+/*
+ * Copyright (C) 2023 Amazon.com Inc. or its affiliates. All rights reserved.
+ * SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+ */
+
+#include <stdio.h>
+#include <pthread.h>
+
+#define MAX_NUM_THREADS 4
+#define NUM_ITER 1000
+
+int g_count = 0;
+
+static void *
+thread(void *arg)
+{
+    for (int i = 0; i < NUM_ITER; i++) {
+        __atomic_fetch_add(&g_count, 1, __ATOMIC_SEQ_CST);
+    }
+
+    return NULL;
+}
+
+int
+main(int argc, char **argv)
+{
+    pthread_t tids[MAX_NUM_THREADS];
+
+    for (int i = 0; i < MAX_NUM_THREADS; i++) {
+        if (pthread_create(&tids[i], NULL, thread, NULL) != 0) {
+            printf("Thread creation failed\n");
+        }
+    }
+
+    for (int i = 0; i < MAX_NUM_THREADS; i++) {
+        if (pthread_join(tids[i], NULL) != 0) {
+            printf("Thread join failed\n");
+        }
+    }
+
+    printf("Value of counter after update: %d (expected=%d)\n", g_count,
+           MAX_NUM_THREADS * NUM_ITER);
+    if (g_count != MAX_NUM_THREADS * NUM_ITER) {
+        __builtin_trap();
+    }
+
+    return -1;
+}
