Fix memory sharing #2415
Fix memory sharing #2415
Conversation
- inherit shared memory from the parent instance, instead of trying to look it up by the underlying module. the old method works correctly only when every clusters uses different modules. - reference count WASMMemoryInstance/AOTMemoryInstance directly - retire WASMSharedMemNode - for atomic opcode implementations in the interpreters, use a global lock for now. - update the internal API users. (wasi-threads, lib-pthread, wasm_runtime_spawn_thread) Fixes bytecodealliance#1962
| if (module_inst->memory_count > 0) | ||
| shared_memory_lock(module_inst->memories[0]); |
There was a problem hiding this comment.
@wenyongh Do you remember why we need to lock the shared memory when setting exception?
There was a problem hiding this comment.
yes, I remember that one instance's exception may be accessed by other threads when spreading non "wasi proc exit" exception. It was introduced in this PR #1995
In thread_manager.c, function set_exception_visitor:
/* Only spread non "wasi proc exit" exception */
#if WASM_ENABLE_SHARED_MEMORY != 0
WASMSharedMemNode *shared_mem_node = wasm_module_get_shared_memory(
(WASMModuleCommon *)curr_wasm_inst->module);
if (shared_mem_node)
os_mutex_lock(&shared_mem_node->shared_mem_lock);
#endif
if (!strstr(wasm_inst->cur_exception, "wasi proc exit")) {
bh_memcpy_s(curr_wasm_inst->cur_exception,
sizeof(curr_wasm_inst->cur_exception),
wasm_inst->cur_exception,
sizeof(wasm_inst->cur_exception));
}
#if WASM_ENABLE_SHARED_MEMORY != 0
if (shared_mem_node)
os_mutex_unlock(&shared_mem_node->shared_mem_lock);
#endif
There was a problem hiding this comment.
yes, I remember that one instance's exception may be accessed by other threads when spreading non "wasi proc exit" exception. It was introduced in this PR #1995
Yes, that's the reason. When a thread runs into an exception it spreads it to other threads by using set_exception_visitor and that other threads could try to read it at the same time, causing concurrency issues.
| if (module_inst->memory_count > 0) | ||
| shared_memory_lock(module_inst->memories[0]); |
There was a problem hiding this comment.
@wenyongh Do you remember why we need to lock the shared memory when setting exception?
| @@ -710,28 +710,28 @@ trunc_f64_to_int(WASMModuleInstance *module, uint32 *frame_sp, float64 src_min, | |||
| CHECK_BULK_MEMORY_OVERFLOW(addr + offset, 1, maddr); \ | |||
| CHECK_ATOMIC_MEMORY_ACCESS(); \ | |||
| \ | |||
| os_mutex_lock(&node->shared_mem_lock); \ | |||
| shared_memory_lock(memory); \ | |||
There was a problem hiding this comment.
An concern is about the performance, calling shared_memory_lock/unlock adds two function calls, it may impact the performance. How about getting the lock pointer, and here os_mutex_{lock|unlock}(shared_mem_lock)?
There was a problem hiding this comment.
does it matter? if we really care the performance here, i guess we should use atomic ops as we do for aot.
There was a problem hiding this comment.
I am not very sure, the performance may be not so important here, I just see an opportunity to refine the code😊. Using atomic operations may be also a choice in the future.
@loganek, any idea?
- Inherit shared memory from the parent instance, instead of trying to look it up by the underlying module. The old method works correctly only when every cluster uses different module. - Use reference count in WASMMemoryInstance/AOTMemoryInstance to mark whether the memory is shared or not - Retire WASMSharedMemNode - For atomic opcode implementations in the interpreters, use a global lock for now - Update the internal API users (wasi-threads, lib-pthread, wasm_runtime_spawn_thread) Fixes bytecodealliance#1962
inherit shared memory from the parent instance, instead of
trying to look it up by the underlying module. the old method
works correctly only when every clusters use different modules.
reference count WASMMemoryInstance/AOTMemoryInstance directly
retire WASMSharedMemNode
for atomic instructions in interpreters, use a global lock for now.
update users (wasi-threads, lib-pthread, wasm_runtime_spawn_thread)
fixes #1962
todo:
aotclassic interpreterlib-pthreadtest wasm_runtime_spawn_threadtest lib-pthread