From c0f13de3757f2dd80e21d7cae11031e25c76530c Mon Sep 17 00:00:00 2001 From: godjan Date: Wed, 8 Mar 2023 11:18:42 +0000 Subject: [PATCH 01/14] Fix wait_info data race for deletion --- core/iwasm/common/wasm_shared_memory.c | 16 ++++++++++++++-- 1 file changed, 14 insertions(+), 2 deletions(-) diff --git a/core/iwasm/common/wasm_shared_memory.c b/core/iwasm/common/wasm_shared_memory.c index eb66f2f096..c2f9dd73e4 100644 --- a/core/iwasm/common/wasm_shared_memory.c +++ b/core/iwasm/common/wasm_shared_memory.c @@ -21,6 +21,7 @@ typedef struct AtomicWaitInfo { korp_mutex wait_list_lock; bh_list wait_list_head; bh_list *wait_list; + int count_acquisition; } AtomicWaitInfo; typedef struct AtomicWaitNode { @@ -305,8 +306,10 @@ acquire_wait_info(void *address, bool create) os_mutex_lock(&wait_map_lock); /* Make find + insert atomic */ - if (address) + if (address) wait_info = (AtomicWaitInfo *)bh_hash_map_find(wait_map, address); + if (wait_info) + ++wait_info->count_acquisition; if (!create) { os_mutex_unlock(&wait_map_lock); @@ -380,11 +383,20 @@ static bool map_remove_wait_info(HashMap *wait_map_, AtomicWaitInfo *wait_info, void *address) { + os_mutex_lock(&wait_map_lock); + --wait_info->count_acquisition; + if (wait_info->wait_list->len > 0) { + os_mutex_unlock(&wait_map_lock); + return false; + } + if (wait_info->count_acquisition > 0) { + os_mutex_unlock(&wait_map_lock); return false; } bh_hash_map_remove(wait_map_, address, NULL, NULL); + os_mutex_unlock(&wait_map_lock); return true; } @@ -486,8 +498,8 @@ wasm_runtime_atomic_wait(WASMModuleInstanceCommon *module, void *address, wasm_runtime_free(wait_node); /* Release wait info if no wait nodes attached */ - removed_from_map = map_remove_wait_info(wait_map, wait_info, address); os_mutex_unlock(&wait_info->wait_list_lock); + removed_from_map = map_remove_wait_info(wait_map, wait_info, address); if (removed_from_map) destroy_wait_info(wait_info); os_mutex_unlock(&node->shared_mem_lock); From 8fdbc122a813700bbf324c91729037883a80c7db Mon Sep 17 00:00:00 2001 From: godjan Date: Wed, 8 Mar 2023 11:29:15 +0000 Subject: [PATCH 02/14] clang format --- core/iwasm/common/wasm_shared_memory.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/core/iwasm/common/wasm_shared_memory.c b/core/iwasm/common/wasm_shared_memory.c index c2f9dd73e4..8c5c435b04 100644 --- a/core/iwasm/common/wasm_shared_memory.c +++ b/core/iwasm/common/wasm_shared_memory.c @@ -306,7 +306,7 @@ acquire_wait_info(void *address, bool create) os_mutex_lock(&wait_map_lock); /* Make find + insert atomic */ - if (address) + if (address) wait_info = (AtomicWaitInfo *)bh_hash_map_find(wait_map, address); if (wait_info) ++wait_info->count_acquisition; From b30a14c6cfbe857d6045ce88e48f7858d4005e70 Mon Sep 17 00:00:00 2001 From: godjan Date: Wed, 8 Mar 2023 11:33:18 +0000 Subject: [PATCH 03/14] Wait_info->wait_list access always should be under its lock actually --- core/iwasm/common/wasm_shared_memory.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/core/iwasm/common/wasm_shared_memory.c b/core/iwasm/common/wasm_shared_memory.c index 8c5c435b04..11a9cae899 100644 --- a/core/iwasm/common/wasm_shared_memory.c +++ b/core/iwasm/common/wasm_shared_memory.c @@ -386,10 +386,13 @@ map_remove_wait_info(HashMap *wait_map_, AtomicWaitInfo *wait_info, os_mutex_lock(&wait_map_lock); --wait_info->count_acquisition; + os_mutex_lock(&wait_info->wait_list_lock); if (wait_info->wait_list->len > 0) { + os_mutex_unlock(&wait_info->wait_list_lock); os_mutex_unlock(&wait_map_lock); return false; } + os_mutex_unlock(&wait_info->wait_list_lock); if (wait_info->count_acquisition > 0) { os_mutex_unlock(&wait_map_lock); return false; From 962e46fd13b3cf5573f2dadd2e5ff90ffb031d29 Mon Sep 17 00:00:00 2001 From: godjan Date: Wed, 8 Mar 2023 15:42:02 +0000 Subject: [PATCH 04/14] forgot to set initial count_acquisition to 1 --- core/iwasm/common/wasm_shared_memory.c | 1 + 1 file changed, 1 insertion(+) diff --git a/core/iwasm/common/wasm_shared_memory.c b/core/iwasm/common/wasm_shared_memory.c index 11a9cae899..3bb922cfc2 100644 --- a/core/iwasm/common/wasm_shared_memory.c +++ b/core/iwasm/common/wasm_shared_memory.c @@ -337,6 +337,7 @@ acquire_wait_info(void *address, bool create) if (!bh_hash_map_insert(wait_map, address, (void *)wait_info)) { goto fail3; } + wait_info->count_acquisition = 1; } os_mutex_unlock(&wait_map_lock); From 2f69b0d98f56c0f2c1c16f1f2beee64899077da8 Mon Sep 17 00:00:00 2001 From: godjan Date: Fri, 10 Mar 2023 13:27:14 +0000 Subject: [PATCH 05/14] use wait_list->len, not counter --- core/iwasm/common/wasm_shared_memory.c | 77 ++++++++++++-------------- 1 file changed, 34 insertions(+), 43 deletions(-) diff --git a/core/iwasm/common/wasm_shared_memory.c b/core/iwasm/common/wasm_shared_memory.c index 3bb922cfc2..38a8703a6c 100644 --- a/core/iwasm/common/wasm_shared_memory.c +++ b/core/iwasm/common/wasm_shared_memory.c @@ -21,7 +21,6 @@ typedef struct AtomicWaitInfo { korp_mutex wait_list_lock; bh_list wait_list_head; bh_list *wait_list; - int count_acquisition; } AtomicWaitInfo; typedef struct AtomicWaitNode { @@ -299,7 +298,7 @@ notify_wait_list(bh_list *wait_list, uint32 count) } static AtomicWaitInfo * -acquire_wait_info(void *address, bool create) +acquire_wait_info(void *address, bool create, AtomicWaitNode* wait_node) { AtomicWaitInfo *wait_info = NULL; bh_list_status ret; @@ -308,10 +307,8 @@ acquire_wait_info(void *address, bool create) if (address) wait_info = (AtomicWaitInfo *)bh_hash_map_find(wait_map, address); - if (wait_info) - ++wait_info->count_acquisition; - if (!create) { + if (!create && !wait_info) { os_mutex_unlock(&wait_map_lock); return wait_info; } @@ -337,7 +334,13 @@ acquire_wait_info(void *address, bool create) if (!bh_hash_map_insert(wait_map, address, (void *)wait_info)) { goto fail3; } - wait_info->count_acquisition = 1; + } + if (wait_node) { + os_mutex_lock(&wait_info->wait_list_lock); + ret = bh_list_insert(wait_info->wait_list, wait_node); + os_mutex_unlock(&wait_info->wait_list_lock); + bh_assert(ret == BH_LIST_SUCCESS); + (void)ret; } os_mutex_unlock(&wait_map_lock); @@ -385,8 +388,6 @@ map_remove_wait_info(HashMap *wait_map_, AtomicWaitInfo *wait_info, void *address) { os_mutex_lock(&wait_map_lock); - --wait_info->count_acquisition; - os_mutex_lock(&wait_info->wait_list_lock); if (wait_info->wait_list->len > 0) { os_mutex_unlock(&wait_info->wait_list_lock); @@ -394,10 +395,6 @@ map_remove_wait_info(HashMap *wait_map_, AtomicWaitInfo *wait_info, return false; } os_mutex_unlock(&wait_info->wait_list_lock); - if (wait_info->count_acquisition > 0) { - os_mutex_unlock(&wait_map_lock); - return false; - } bh_hash_map_remove(wait_map_, address, NULL, NULL); os_mutex_unlock(&wait_map_lock); @@ -434,14 +431,6 @@ wasm_runtime_atomic_wait(WASMModuleInstanceCommon *module, void *address, return -1; } - /* acquire the wait info, create new one if not exists */ - wait_info = acquire_wait_info(address, true); - - if (!wait_info) { - wasm_runtime_set_exception(module, "failed to acquire wait_info"); - return -1; - } - node = search_module((WASMModuleCommon *)module_inst->module); os_mutex_lock(&node->shared_mem_lock); no_wait = (!wait64 && *(uint32 *)address != (uint32)expect) @@ -451,32 +440,34 @@ wasm_runtime_atomic_wait(WASMModuleInstanceCommon *module, void *address, if (no_wait) { return 1; } - else { - bh_list_status ret; - if (!(wait_node = wasm_runtime_malloc(sizeof(AtomicWaitNode)))) { - wasm_runtime_set_exception(module, "failed to create wait node"); - return -1; - } - memset(wait_node, 0, sizeof(AtomicWaitNode)); + if (!(wait_node = wasm_runtime_malloc(sizeof(AtomicWaitNode)))) { + wasm_runtime_set_exception(module, "failed to create wait node"); + return -1; + } + memset(wait_node, 0, sizeof(AtomicWaitNode)); - if (0 != os_mutex_init(&wait_node->wait_lock)) { - wasm_runtime_free(wait_node); - return -1; - } + if (0 != os_mutex_init(&wait_node->wait_lock)) { + wasm_runtime_free(wait_node); + return -1; + } - if (0 != os_cond_init(&wait_node->wait_cond)) { - os_mutex_destroy(&wait_node->wait_lock); - wasm_runtime_free(wait_node); - return -1; - } + if (0 != os_cond_init(&wait_node->wait_cond)) { + os_mutex_destroy(&wait_node->wait_lock); + wasm_runtime_free(wait_node); + return -1; + } - wait_node->status = S_WAITING; - os_mutex_lock(&wait_info->wait_list_lock); - ret = bh_list_insert(wait_info->wait_list, wait_node); - os_mutex_unlock(&wait_info->wait_list_lock); - bh_assert(ret == BH_LIST_SUCCESS); - (void)ret; + wait_node->status = S_WAITING; + + /* acquire the wait info, create new one if not exists */ + wait_info = acquire_wait_info(address, true, wait_node); + + if (!wait_info) { + os_mutex_destroy(&wait_node->wait_lock); + wasm_runtime_free(wait_node); + wasm_runtime_set_exception(module, "failed to acquire wait_info"); + return -1; } /* condition wait start */ @@ -539,7 +530,7 @@ wasm_runtime_atomic_notify(WASMModuleInstanceCommon *module, void *address, return -1; } - wait_info = acquire_wait_info(address, false); + wait_info = acquire_wait_info(address, false, NULL); /* Nobody wait on this address */ if (!wait_info) { From 3e1a97421de5392409410b74a9bcd595b085e9a9 Mon Sep 17 00:00:00 2001 From: godjan Date: Fri, 10 Mar 2023 14:41:09 +0000 Subject: [PATCH 06/14] clang format --- core/iwasm/common/wasm_shared_memory.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/core/iwasm/common/wasm_shared_memory.c b/core/iwasm/common/wasm_shared_memory.c index 38a8703a6c..a195b5d17e 100644 --- a/core/iwasm/common/wasm_shared_memory.c +++ b/core/iwasm/common/wasm_shared_memory.c @@ -298,7 +298,7 @@ notify_wait_list(bh_list *wait_list, uint32 count) } static AtomicWaitInfo * -acquire_wait_info(void *address, bool create, AtomicWaitNode* wait_node) +acquire_wait_info(void *address, bool create, AtomicWaitNode *wait_node) { AtomicWaitInfo *wait_info = NULL; bh_list_status ret; From bc9fa8b2889ff40923954bb3046e5fcdeb378f32 Mon Sep 17 00:00:00 2001 From: godjan Date: Sat, 11 Mar 2023 00:27:07 +0000 Subject: [PATCH 07/14] Fix wromg waiting on atomic when proc_exit was called before thread reached wait on conditional variable stage, but already started wasm instruction atomic_wait --- core/iwasm/common/wasm_shared_memory.c | 15 +++++++++++++-- core/iwasm/common/wasm_shared_memory.h | 7 ++++++- core/iwasm/interpreter/wasm_interp_classic.c | 4 ++-- 3 files changed, 21 insertions(+), 5 deletions(-) diff --git a/core/iwasm/common/wasm_shared_memory.c b/core/iwasm/common/wasm_shared_memory.c index a195b5d17e..a4fa5f6b62 100644 --- a/core/iwasm/common/wasm_shared_memory.c +++ b/core/iwasm/common/wasm_shared_memory.c @@ -5,6 +5,9 @@ #include "bh_log.h" #include "wasm_shared_memory.h" +#if WASM_ENABLE_THREAD_MGR != 0 +#include "../libraries/thread-mgr/thread_manager.h" +#endif static bh_list shared_memory_list_head; static bh_list *const shared_memory_list = &shared_memory_list_head; @@ -403,7 +406,7 @@ map_remove_wait_info(HashMap *wait_map_, AtomicWaitInfo *wait_info, uint32 wasm_runtime_atomic_wait(WASMModuleInstanceCommon *module, void *address, - uint64 expect, int64 timeout, bool wait64) + uint64 expect, int64 timeout, bool wait64, WASMExecEnv *exec_env) { WASMModuleInstance *module_inst = (WASMModuleInstance *)module; AtomicWaitInfo *wait_info; @@ -473,9 +476,17 @@ wasm_runtime_atomic_wait(WASMModuleInstanceCommon *module, void *address, /* condition wait start */ os_mutex_lock(&wait_node->wait_lock); - os_cond_reltimedwait(&wait_node->wait_cond, &wait_node->wait_lock, + #if WASM_ENABLE_THREAD_MGR != 0 + if (!wasm_cluster_is_thread_terminated(exec_env)) { + #endif + os_cond_reltimedwait(&wait_node->wait_cond, &wait_node->wait_lock, timeout < 0 ? BHT_WAIT_FOREVER : (uint64)timeout / 1000); + #if WASM_ENABLE_THREAD_MGR != 0 + } + #endif + + is_timeout = wait_node->status == S_WAITING ? true : false; os_mutex_unlock(&wait_node->wait_lock); diff --git a/core/iwasm/common/wasm_shared_memory.h b/core/iwasm/common/wasm_shared_memory.h index 98683f32b3..fde5a8a9a4 100644 --- a/core/iwasm/common/wasm_shared_memory.h +++ b/core/iwasm/common/wasm_shared_memory.h @@ -7,6 +7,10 @@ #define _WASM_SHARED_MEMORY_H #include "bh_common.h" +#include "wasm_exec_env.h" +#if WASM_ENABLE_THREAD_MGR != 0 +#include "../libraries/thread-mgr/thread_manager.h" +#endif #if WASM_ENABLE_INTERP != 0 #include "wasm_runtime.h" #endif @@ -14,6 +18,7 @@ #include "aot_runtime.h" #endif + #ifdef __cplusplus extern "C" { #endif @@ -60,7 +65,7 @@ shared_memory_set_memory_inst(WASMModuleCommon *module, uint32 wasm_runtime_atomic_wait(WASMModuleInstanceCommon *module, void *address, - uint64 expect, int64 timeout, bool wait64); + uint64 expect, int64 timeout, bool wait64, WASMExecEnv *exec_env); uint32 wasm_runtime_atomic_notify(WASMModuleInstanceCommon *module, void *address, diff --git a/core/iwasm/interpreter/wasm_interp_classic.c b/core/iwasm/interpreter/wasm_interp_classic.c index dd45865ba8..1f4cc26c1d 100644 --- a/core/iwasm/interpreter/wasm_interp_classic.c +++ b/core/iwasm/interpreter/wasm_interp_classic.c @@ -3428,7 +3428,7 @@ wasm_interp_call_func_bytecode(WASMModuleInstance *module, ret = wasm_runtime_atomic_wait( (WASMModuleInstanceCommon *)module, maddr, - (uint64)expect, timeout, false); + (uint64)expect, timeout, false, exec_env); if (ret == (uint32)-1) goto got_exception; @@ -3452,7 +3452,7 @@ wasm_interp_call_func_bytecode(WASMModuleInstance *module, ret = wasm_runtime_atomic_wait( (WASMModuleInstanceCommon *)module, maddr, expect, - timeout, true); + timeout, true, exec_env); if (ret == (uint32)-1) goto got_exception; From 388684d080a63ad3de52b27dc2d1bddf01009dbd Mon Sep 17 00:00:00 2001 From: godjan Date: Sat, 11 Mar 2023 00:31:06 +0000 Subject: [PATCH 08/14] clang-format --- core/iwasm/common/wasm_shared_memory.c | 17 ++++++++--------- core/iwasm/common/wasm_shared_memory.h | 4 ++-- core/iwasm/interpreter/wasm_interp_classic.c | 10 ++++++++-- 3 files changed, 18 insertions(+), 13 deletions(-) diff --git a/core/iwasm/common/wasm_shared_memory.c b/core/iwasm/common/wasm_shared_memory.c index a4fa5f6b62..a5e469472a 100644 --- a/core/iwasm/common/wasm_shared_memory.c +++ b/core/iwasm/common/wasm_shared_memory.c @@ -406,7 +406,8 @@ map_remove_wait_info(HashMap *wait_map_, AtomicWaitInfo *wait_info, uint32 wasm_runtime_atomic_wait(WASMModuleInstanceCommon *module, void *address, - uint64 expect, int64 timeout, bool wait64, WASMExecEnv *exec_env) + uint64 expect, int64 timeout, bool wait64, + WASMExecEnv *exec_env) { WASMModuleInstance *module_inst = (WASMModuleInstance *)module; AtomicWaitInfo *wait_info; @@ -476,17 +477,15 @@ wasm_runtime_atomic_wait(WASMModuleInstanceCommon *module, void *address, /* condition wait start */ os_mutex_lock(&wait_node->wait_lock); - #if WASM_ENABLE_THREAD_MGR != 0 +#if WASM_ENABLE_THREAD_MGR != 0 if (!wasm_cluster_is_thread_terminated(exec_env)) { - #endif +#endif os_cond_reltimedwait(&wait_node->wait_cond, &wait_node->wait_lock, - timeout < 0 ? BHT_WAIT_FOREVER - : (uint64)timeout / 1000); - #if WASM_ENABLE_THREAD_MGR != 0 + timeout < 0 ? BHT_WAIT_FOREVER + : (uint64)timeout / 1000); +#if WASM_ENABLE_THREAD_MGR != 0 } - #endif - - +#endif is_timeout = wait_node->status == S_WAITING ? true : false; os_mutex_unlock(&wait_node->wait_lock); diff --git a/core/iwasm/common/wasm_shared_memory.h b/core/iwasm/common/wasm_shared_memory.h index fde5a8a9a4..8e57a826cf 100644 --- a/core/iwasm/common/wasm_shared_memory.h +++ b/core/iwasm/common/wasm_shared_memory.h @@ -18,7 +18,6 @@ #include "aot_runtime.h" #endif - #ifdef __cplusplus extern "C" { #endif @@ -65,7 +64,8 @@ shared_memory_set_memory_inst(WASMModuleCommon *module, uint32 wasm_runtime_atomic_wait(WASMModuleInstanceCommon *module, void *address, - uint64 expect, int64 timeout, bool wait64, WASMExecEnv *exec_env); + uint64 expect, int64 timeout, bool wait64, + WASMExecEnv *exec_env); uint32 wasm_runtime_atomic_notify(WASMModuleInstanceCommon *module, void *address, diff --git a/core/iwasm/interpreter/wasm_interp_classic.c b/core/iwasm/interpreter/wasm_interp_classic.c index 1f4cc26c1d..1d875b57a7 100644 --- a/core/iwasm/interpreter/wasm_interp_classic.c +++ b/core/iwasm/interpreter/wasm_interp_classic.c @@ -1195,7 +1195,10 @@ wasm_interp_call_func_bytecode(WASMModuleInstance *module, goto got_exception; } - HANDLE_OP(WASM_OP_NOP) { HANDLE_OP_END(); } + HANDLE_OP(WASM_OP_NOP) + { + HANDLE_OP_END(); + } HANDLE_OP(EXT_OP_BLOCK) { @@ -3032,7 +3035,10 @@ wasm_interp_call_func_bytecode(WASMModuleInstance *module, HANDLE_OP(WASM_OP_I32_REINTERPRET_F32) HANDLE_OP(WASM_OP_I64_REINTERPRET_F64) HANDLE_OP(WASM_OP_F32_REINTERPRET_I32) - HANDLE_OP(WASM_OP_F64_REINTERPRET_I64) { HANDLE_OP_END(); } + HANDLE_OP(WASM_OP_F64_REINTERPRET_I64) + { + HANDLE_OP_END(); + } HANDLE_OP(WASM_OP_I32_EXTEND8_S) { From 8bf4aab1eb858f55684ca6051afc594ba940a027 Mon Sep 17 00:00:00 2001 From: godjan Date: Sat, 11 Mar 2023 00:33:25 +0000 Subject: [PATCH 09/14] remove timeouts to catch actual errors for atomic_wait with BLOCKING_TASK_BUSY_WAIT and BLOCKING_TASK_ATOMIC_WAIT --- core/iwasm/libraries/lib-wasi-threads/test/common.h | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/core/iwasm/libraries/lib-wasi-threads/test/common.h b/core/iwasm/libraries/lib-wasi-threads/test/common.h index a531e39dc4..99a52ca099 100644 --- a/core/iwasm/libraries/lib-wasi-threads/test/common.h +++ b/core/iwasm/libraries/lib-wasi-threads/test/common.h @@ -36,12 +36,11 @@ void run_long_task() { if (blocking_task_type == BLOCKING_TASK_BUSY_WAIT) { - for (int i = 0; i < TIMEOUT_SECONDS; i++) + for (;;) sleep(1); } else if (blocking_task_type == BLOCKING_TASK_ATOMIC_WAIT) { - __builtin_wasm_memory_atomic_wait32( - 0, 0, TIMEOUT_SECONDS * 1000 * 1000 * 1000); + __builtin_wasm_memory_atomic_wait32(0, 0, -1); } else { sleep(TIMEOUT_SECONDS); From 1efcb6417aee23254d94880dd5aa165ddc08868a Mon Sep 17 00:00:00 2001 From: godjan Date: Sat, 11 Mar 2023 00:38:47 +0000 Subject: [PATCH 10/14] clang-format-12 --- core/iwasm/interpreter/wasm_interp_classic.c | 10 ++-------- 1 file changed, 2 insertions(+), 8 deletions(-) diff --git a/core/iwasm/interpreter/wasm_interp_classic.c b/core/iwasm/interpreter/wasm_interp_classic.c index 1d875b57a7..1f4cc26c1d 100644 --- a/core/iwasm/interpreter/wasm_interp_classic.c +++ b/core/iwasm/interpreter/wasm_interp_classic.c @@ -1195,10 +1195,7 @@ wasm_interp_call_func_bytecode(WASMModuleInstance *module, goto got_exception; } - HANDLE_OP(WASM_OP_NOP) - { - HANDLE_OP_END(); - } + HANDLE_OP(WASM_OP_NOP) { HANDLE_OP_END(); } HANDLE_OP(EXT_OP_BLOCK) { @@ -3035,10 +3032,7 @@ wasm_interp_call_func_bytecode(WASMModuleInstance *module, HANDLE_OP(WASM_OP_I32_REINTERPRET_F32) HANDLE_OP(WASM_OP_I64_REINTERPRET_F64) HANDLE_OP(WASM_OP_F32_REINTERPRET_I32) - HANDLE_OP(WASM_OP_F64_REINTERPRET_I64) - { - HANDLE_OP_END(); - } + HANDLE_OP(WASM_OP_F64_REINTERPRET_I64) { HANDLE_OP_END(); } HANDLE_OP(WASM_OP_I32_EXTEND8_S) { From bcd639da0c89a6678970427ff5209f2f60b42399 Mon Sep 17 00:00:00 2001 From: godjan Date: Sat, 11 Mar 2023 00:44:51 +0000 Subject: [PATCH 11/14] fix passed args for fast interpreter --- core/iwasm/interpreter/wasm_interp_fast.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/core/iwasm/interpreter/wasm_interp_fast.c b/core/iwasm/interpreter/wasm_interp_fast.c index 628f4d065f..6b708d0929 100644 --- a/core/iwasm/interpreter/wasm_interp_fast.c +++ b/core/iwasm/interpreter/wasm_interp_fast.c @@ -3270,7 +3270,7 @@ wasm_interp_call_func_bytecode(WASMModuleInstance *module, ret = wasm_runtime_atomic_wait( (WASMModuleInstanceCommon *)module, maddr, - (uint64)expect, timeout, false); + (uint64)expect, timeout, false, exec_env); if (ret == (uint32)-1) goto got_exception; @@ -3294,7 +3294,7 @@ wasm_interp_call_func_bytecode(WASMModuleInstance *module, ret = wasm_runtime_atomic_wait( (WASMModuleInstanceCommon *)module, maddr, expect, - timeout, true); + timeout, true, exec_env); if (ret == (uint32)-1) goto got_exception; From 6c2141308c06b3987d4d793cd16fd4e533cb8536 Mon Sep 17 00:00:00 2001 From: godjan Date: Sat, 11 Mar 2023 02:26:11 +0000 Subject: [PATCH 12/14] Address review comments --- core/iwasm/common/wasm_shared_memory.c | 33 ++++++++++--------- core/iwasm/common/wasm_shared_memory.h | 6 +--- core/iwasm/interpreter/wasm_interp_classic.c | 4 +-- core/iwasm/interpreter/wasm_interp_fast.c | 4 +-- .../libraries/lib-wasi-threads/test/common.h | 8 ++--- 5 files changed, 27 insertions(+), 28 deletions(-) diff --git a/core/iwasm/common/wasm_shared_memory.c b/core/iwasm/common/wasm_shared_memory.c index a5e469472a..4127499d0b 100644 --- a/core/iwasm/common/wasm_shared_memory.c +++ b/core/iwasm/common/wasm_shared_memory.c @@ -301,7 +301,7 @@ notify_wait_list(bh_list *wait_list, uint32 count) } static AtomicWaitInfo * -acquire_wait_info(void *address, bool create, AtomicWaitNode *wait_node) +acquire_wait_info(void *address, AtomicWaitNode *wait_node) { AtomicWaitInfo *wait_info = NULL; bh_list_status ret; @@ -311,7 +311,7 @@ acquire_wait_info(void *address, bool create, AtomicWaitNode *wait_node) if (address) wait_info = (AtomicWaitInfo *)bh_hash_map_find(wait_map, address); - if (!create && !wait_info) { + if (!wait_node && !wait_info) { os_mutex_unlock(&wait_map_lock); return wait_info; } @@ -386,34 +386,34 @@ destroy_wait_info(void *wait_info) } } -static bool -map_remove_wait_info(HashMap *wait_map_, AtomicWaitInfo *wait_info, - void *address) +static void +map_try_release_wait_info(HashMap *wait_map_, AtomicWaitInfo *wait_info, + void *address) { os_mutex_lock(&wait_map_lock); os_mutex_lock(&wait_info->wait_list_lock); if (wait_info->wait_list->len > 0) { os_mutex_unlock(&wait_info->wait_list_lock); os_mutex_unlock(&wait_map_lock); - return false; + return; } os_mutex_unlock(&wait_info->wait_list_lock); bh_hash_map_remove(wait_map_, address, NULL, NULL); os_mutex_unlock(&wait_map_lock); - return true; + destroy_wait_info(wait_info); } uint32 wasm_runtime_atomic_wait(WASMModuleInstanceCommon *module, void *address, - uint64 expect, int64 timeout, bool wait64, - WASMExecEnv *exec_env) + uint64 expect, int64 timeout, bool wait64) { WASMModuleInstance *module_inst = (WASMModuleInstance *)module; AtomicWaitInfo *wait_info; AtomicWaitNode *wait_node; WASMSharedMemNode *node; - bool check_ret, is_timeout, no_wait, removed_from_map; + WASMExecEnv *exec_env; + bool check_ret, is_timeout, no_wait; bh_assert(module->module_type == Wasm_Module_Bytecode || module->module_type == Wasm_Module_AoT); @@ -465,7 +465,7 @@ wasm_runtime_atomic_wait(WASMModuleInstanceCommon *module, void *address, wait_node->status = S_WAITING; /* acquire the wait info, create new one if not exists */ - wait_info = acquire_wait_info(address, true, wait_node); + wait_info = acquire_wait_info(address, wait_node); if (!wait_info) { os_mutex_destroy(&wait_node->wait_lock); @@ -474,6 +474,11 @@ wasm_runtime_atomic_wait(WASMModuleInstanceCommon *module, void *address, return -1; } +#if WASM_ENABLE_THREAD_MGR != 0 + exec_env = + wasm_clusters_search_exec_env((WASMModuleInstanceCommon *)module_inst); +#endif + /* condition wait start */ os_mutex_lock(&wait_node->wait_lock); @@ -504,9 +509,7 @@ wasm_runtime_atomic_wait(WASMModuleInstanceCommon *module, void *address, /* Release wait info if no wait nodes attached */ os_mutex_unlock(&wait_info->wait_list_lock); - removed_from_map = map_remove_wait_info(wait_map, wait_info, address); - if (removed_from_map) - destroy_wait_info(wait_info); + map_try_release_wait_info(wait_map, wait_info, address); os_mutex_unlock(&node->shared_mem_lock); (void)check_ret; @@ -540,7 +543,7 @@ wasm_runtime_atomic_notify(WASMModuleInstanceCommon *module, void *address, return -1; } - wait_info = acquire_wait_info(address, false, NULL); + wait_info = acquire_wait_info(address, NULL); /* Nobody wait on this address */ if (!wait_info) { diff --git a/core/iwasm/common/wasm_shared_memory.h b/core/iwasm/common/wasm_shared_memory.h index 8e57a826cf..27019ff530 100644 --- a/core/iwasm/common/wasm_shared_memory.h +++ b/core/iwasm/common/wasm_shared_memory.h @@ -8,9 +8,6 @@ #include "bh_common.h" #include "wasm_exec_env.h" -#if WASM_ENABLE_THREAD_MGR != 0 -#include "../libraries/thread-mgr/thread_manager.h" -#endif #if WASM_ENABLE_INTERP != 0 #include "wasm_runtime.h" #endif @@ -64,8 +61,7 @@ shared_memory_set_memory_inst(WASMModuleCommon *module, uint32 wasm_runtime_atomic_wait(WASMModuleInstanceCommon *module, void *address, - uint64 expect, int64 timeout, bool wait64, - WASMExecEnv *exec_env); + uint64 expect, int64 timeout, bool wait64); uint32 wasm_runtime_atomic_notify(WASMModuleInstanceCommon *module, void *address, diff --git a/core/iwasm/interpreter/wasm_interp_classic.c b/core/iwasm/interpreter/wasm_interp_classic.c index 1f4cc26c1d..dd45865ba8 100644 --- a/core/iwasm/interpreter/wasm_interp_classic.c +++ b/core/iwasm/interpreter/wasm_interp_classic.c @@ -3428,7 +3428,7 @@ wasm_interp_call_func_bytecode(WASMModuleInstance *module, ret = wasm_runtime_atomic_wait( (WASMModuleInstanceCommon *)module, maddr, - (uint64)expect, timeout, false, exec_env); + (uint64)expect, timeout, false); if (ret == (uint32)-1) goto got_exception; @@ -3452,7 +3452,7 @@ wasm_interp_call_func_bytecode(WASMModuleInstance *module, ret = wasm_runtime_atomic_wait( (WASMModuleInstanceCommon *)module, maddr, expect, - timeout, true, exec_env); + timeout, true); if (ret == (uint32)-1) goto got_exception; diff --git a/core/iwasm/interpreter/wasm_interp_fast.c b/core/iwasm/interpreter/wasm_interp_fast.c index 6b708d0929..628f4d065f 100644 --- a/core/iwasm/interpreter/wasm_interp_fast.c +++ b/core/iwasm/interpreter/wasm_interp_fast.c @@ -3270,7 +3270,7 @@ wasm_interp_call_func_bytecode(WASMModuleInstance *module, ret = wasm_runtime_atomic_wait( (WASMModuleInstanceCommon *)module, maddr, - (uint64)expect, timeout, false, exec_env); + (uint64)expect, timeout, false); if (ret == (uint32)-1) goto got_exception; @@ -3294,7 +3294,7 @@ wasm_interp_call_func_bytecode(WASMModuleInstance *module, ret = wasm_runtime_atomic_wait( (WASMModuleInstanceCommon *)module, maddr, expect, - timeout, true, exec_env); + timeout, true); if (ret == (uint32)-1) goto got_exception; diff --git a/core/iwasm/libraries/lib-wasi-threads/test/common.h b/core/iwasm/libraries/lib-wasi-threads/test/common.h index 99a52ca099..d032f824c3 100644 --- a/core/iwasm/libraries/lib-wasi-threads/test/common.h +++ b/core/iwasm/libraries/lib-wasi-threads/test/common.h @@ -9,6 +9,7 @@ #include #include #include +#include #include "wasi_thread_start.h" @@ -23,7 +24,6 @@ static bool termination_by_trap; static bool termination_in_main_thread; static blocking_task_type_t blocking_task_type; -#define TIMEOUT_SECONDS 10ll #define NUM_THREADS 3 static pthread_barrier_t barrier; @@ -36,14 +36,14 @@ void run_long_task() { if (blocking_task_type == BLOCKING_TASK_BUSY_WAIT) { - for (;;) - sleep(1); + for (;;) { + } } else if (blocking_task_type == BLOCKING_TASK_ATOMIC_WAIT) { __builtin_wasm_memory_atomic_wait32(0, 0, -1); } else { - sleep(TIMEOUT_SECONDS); + sleep(UINT_MAX); } } From e81fa955780ab89f73e289e1685e794d98bd3f5c Mon Sep 17 00:00:00 2001 From: godjan Date: Sat, 11 Mar 2023 02:29:58 +0000 Subject: [PATCH 13/14] Added warning about using wait-info->wait_list --- core/iwasm/common/wasm_shared_memory.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/core/iwasm/common/wasm_shared_memory.c b/core/iwasm/common/wasm_shared_memory.c index 4127499d0b..9355789ba7 100644 --- a/core/iwasm/common/wasm_shared_memory.c +++ b/core/iwasm/common/wasm_shared_memory.c @@ -24,6 +24,8 @@ typedef struct AtomicWaitInfo { korp_mutex wait_list_lock; bh_list wait_list_head; bh_list *wait_list; + // WARNING: insert to the list allowed only in acquire_wait_info + // otherwise there will be data race as described in PR #2016 } AtomicWaitInfo; typedef struct AtomicWaitNode { From 2970e7d000defa54c5abf8dedfc61192e6e39e9d Mon Sep 17 00:00:00 2001 From: godjan Date: Sat, 11 Mar 2023 18:41:43 +0000 Subject: [PATCH 14/14] fix comments --- core/iwasm/common/wasm_shared_memory.c | 33 +++++++++++++++----------- 1 file changed, 19 insertions(+), 14 deletions(-) diff --git a/core/iwasm/common/wasm_shared_memory.c b/core/iwasm/common/wasm_shared_memory.c index 9355789ba7..dc7cf1f99f 100644 --- a/core/iwasm/common/wasm_shared_memory.c +++ b/core/iwasm/common/wasm_shared_memory.c @@ -24,8 +24,8 @@ typedef struct AtomicWaitInfo { korp_mutex wait_list_lock; bh_list wait_list_head; bh_list *wait_list; - // WARNING: insert to the list allowed only in acquire_wait_info - // otherwise there will be data race as described in PR #2016 + /* WARNING: insert to the list allowed only in acquire_wait_info + otherwise there will be data race as described in PR #2016 */ } AtomicWaitInfo; typedef struct AtomicWaitNode { @@ -313,7 +313,7 @@ acquire_wait_info(void *address, AtomicWaitNode *wait_node) if (address) wait_info = (AtomicWaitInfo *)bh_hash_map_find(wait_map, address); - if (!wait_node && !wait_info) { + if (!wait_node) { os_mutex_unlock(&wait_map_lock); return wait_info; } @@ -340,13 +340,12 @@ acquire_wait_info(void *address, AtomicWaitNode *wait_node) goto fail3; } } - if (wait_node) { - os_mutex_lock(&wait_info->wait_list_lock); - ret = bh_list_insert(wait_info->wait_list, wait_node); - os_mutex_unlock(&wait_info->wait_list_lock); - bh_assert(ret == BH_LIST_SUCCESS); - (void)ret; - } + + os_mutex_lock(&wait_info->wait_list_lock); + ret = bh_list_insert(wait_info->wait_list, wait_node); + os_mutex_unlock(&wait_info->wait_list_lock); + bh_assert(ret == BH_LIST_SUCCESS); + (void)ret; os_mutex_unlock(&wait_map_lock); @@ -479,20 +478,26 @@ wasm_runtime_atomic_wait(WASMModuleInstanceCommon *module, void *address, #if WASM_ENABLE_THREAD_MGR != 0 exec_env = wasm_clusters_search_exec_env((WASMModuleInstanceCommon *)module_inst); + bh_assert(exec_env); #endif + os_mutex_lock(&node->shared_mem_lock); + no_wait = (!wait64 && *(uint32 *)address != (uint32)expect) + || (wait64 && *(uint64 *)address != expect); + os_mutex_unlock(&node->shared_mem_lock); + /* condition wait start */ os_mutex_lock(&wait_node->wait_lock); + if (!no_wait #if WASM_ENABLE_THREAD_MGR != 0 - if (!wasm_cluster_is_thread_terminated(exec_env)) { + && !wasm_cluster_is_thread_terminated(exec_env) #endif + ) { os_cond_reltimedwait(&wait_node->wait_cond, &wait_node->wait_lock, timeout < 0 ? BHT_WAIT_FOREVER : (uint64)timeout / 1000); -#if WASM_ENABLE_THREAD_MGR != 0 } -#endif is_timeout = wait_node->status == S_WAITING ? true : false; os_mutex_unlock(&wait_node->wait_lock); @@ -515,7 +520,7 @@ wasm_runtime_atomic_wait(WASMModuleInstanceCommon *module, void *address, os_mutex_unlock(&node->shared_mem_lock); (void)check_ret; - return is_timeout ? 2 : 0; + return no_wait ? 1 : is_timeout ? 2 : 0; } uint32