Skip to content

Fix memory sharing #2415

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
wenyongh merged 4 commits into from
Aug 4, 2023
Merged

Fix memory sharing #2415

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
6e3e075
Fix shared memory for multi cluster usage
yamt Jul 31, 2023
2f5e85a
move assertions to shared_memory_dec_reference/shared_memory_inc_refe…
yamt Aug 3, 2023
fb2e000
comment style
yamt Aug 3, 2023
e556747
uint32_t -> uint32
yamt Aug 3, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
67 changes: 23 additions & 44 deletions core/iwasm/aot/aot_runtime.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -339,11 +339,8 @@ memories_deinstantiate(AOTModuleInstance *module_inst)
memory_inst = module_inst->memories[i];
if (memory_inst) {
#if WASM_ENABLE_SHARED_MEMORY != 0
if (memory_inst->is_shared) {
int32 ref_count = shared_memory_dec_reference(
(WASMModuleCommon *)module_inst->module);
bh_assert(ref_count >= 0);

if (shared_memory_is_shared(memory_inst)) {
uint32 ref_count = shared_memory_dec_reference(memory_inst);
/* if the reference count is not zero,
don't free the memory */
if (ref_count > 0)
Expand Down Expand Up @@ -373,9 +370,10 @@ memories_deinstantiate(AOTModuleInstance *module_inst)
}

static AOTMemoryInstance *
memory_instantiate(AOTModuleInstance *module_inst, AOTModule *module,
AOTMemoryInstance *memory_inst, AOTMemory *memory,
uint32 heap_size, char *error_buf, uint32 error_buf_size)
memory_instantiate(AOTModuleInstance *module_inst, AOTModuleInstance *parent,
AOTModule *module, AOTMemoryInstance *memory_inst,
AOTMemory *memory, uint32 memory_idx, uint32 heap_size,
char *error_buf, uint32 error_buf_size)
{
void *heap_handle;
uint32 num_bytes_per_page = memory->num_bytes_per_page;
Expand All @@ -396,23 +394,13 @@ memory_instantiate(AOTModuleInstance *module_inst, AOTModule *module,
bool is_shared_memory = memory->memory_flags & 0x02 ? true : false;

/* Shared memory */
if (is_shared_memory) {
if (is_shared_memory && parent != NULL) {
AOTMemoryInstance *shared_memory_instance;
WASMSharedMemNode *node =
wasm_module_get_shared_memory((WASMModuleCommon *)module);
/* If the memory of this module has been instantiated,
return the memory instance directly */
if (node) {
uint32 ref_count;
ref_count = shared_memory_inc_reference((WASMModuleCommon *)module);
bh_assert(ref_count > 0);
shared_memory_instance =
(AOTMemoryInstance *)shared_memory_get_memory_inst(node);
bh_assert(shared_memory_instance);

(void)ref_count;
return shared_memory_instance;
}
bh_assert(memory_idx == 0);
bh_assert(parent->memory_count > memory_idx);
shared_memory_instance = parent->memories[memory_idx];
shared_memory_inc_reference(shared_memory_instance);
return shared_memory_instance;
}
#endif

Expand Down Expand Up @@ -609,23 +597,12 @@ memory_instantiate(AOTModuleInstance *module_inst, AOTModule *module,

#if WASM_ENABLE_SHARED_MEMORY != 0
if (is_shared_memory) {
memory_inst->is_shared = true;
if (!shared_memory_set_memory_inst(
(WASMModuleCommon *)module,
(WASMMemoryInstanceCommon *)memory_inst)) {
set_error_buf(error_buf, error_buf_size, "allocate memory failed");
goto fail3;
}
memory_inst->ref_count = 1;
}
#endif

return memory_inst;

#if WASM_ENABLE_SHARED_MEMORY != 0
fail3:
if (heap_size > 0)
mem_allocator_destroy(memory_inst->heap_handle);
#endif
fail2:
if (heap_size > 0)
wasm_runtime_free(memory_inst->heap_handle);
Expand Down Expand Up @@ -654,8 +631,9 @@ aot_get_default_memory(AOTModuleInstance *module_inst)
}

static bool
memories_instantiate(AOTModuleInstance *module_inst, AOTModule *module,
uint32 heap_size, char *error_buf, uint32 error_buf_size)
memories_instantiate(AOTModuleInstance *module_inst, AOTModuleInstance *parent,
AOTModule *module, uint32 heap_size, char *error_buf,
uint32 error_buf_size)
{
uint32 global_index, global_data_offset, base_offset, length;
uint32 i, memory_count = module->memory_count;
Expand All @@ -672,8 +650,8 @@ memories_instantiate(AOTModuleInstance *module_inst, AOTModule *module,

memories = module_inst->global_table_data.memory_instances;
for (i = 0; i < memory_count; i++, memories++) {
memory_inst = memory_instantiate(module_inst, module, memories,
&module->memories[i], heap_size,
memory_inst = memory_instantiate(module_inst, parent, module, memories,
&module->memories[i], i, heap_size,
error_buf, error_buf_size);
if (!memory_inst) {
return false;
Expand Down Expand Up @@ -1100,9 +1078,9 @@ check_linked_symbol(AOTModule *module, char *error_buf, uint32 error_buf_size)
}

AOTModuleInstance *
aot_instantiate(AOTModule *module, bool is_sub_inst, WASMExecEnv *exec_env_main,
uint32 stack_size, uint32 heap_size, char *error_buf,
uint32 error_buf_size)
aot_instantiate(AOTModule *module, AOTModuleInstance *parent,
WASMExecEnv *exec_env_main, uint32 stack_size, uint32 heap_size,
char *error_buf, uint32 error_buf_size)
{
AOTModuleInstance *module_inst;
const uint32 module_inst_struct_size =
Expand All @@ -1112,6 +1090,7 @@ aot_instantiate(AOTModule *module, bool is_sub_inst, WASMExecEnv *exec_env_main,
uint64 total_size, table_size = 0;
uint8 *p;
uint32 i, extra_info_offset;
const bool is_sub_inst = parent != NULL;

/* Check heap size */
heap_size = align_uint(heap_size, 8);
Expand Down Expand Up @@ -1171,7 +1150,7 @@ aot_instantiate(AOTModule *module, bool is_sub_inst, WASMExecEnv *exec_env_main,
goto fail;

/* Initialize memory space */
if (!memories_instantiate(module_inst, module, heap_size, error_buf,
if (!memories_instantiate(module_inst, parent, module, heap_size, error_buf,
error_buf_size))
goto fail;

Expand Down
8 changes: 4 additions & 4 deletions core/iwasm/aot/aot_runtime.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -406,7 +406,7 @@ aot_unload(AOTModule *module);
* Instantiate a AOT module.
*
* @param module the AOT module to instantiate
* @param is_sub_inst the flag of sub instance
* @param parent the parent module instance
* @param heap_size the default heap size of the module instance, a heap will
* be created besides the app memory space. Both wasm app and native
* function can allocate memory from the heap. If heap_size is 0, the
Expand All @@ -417,9 +417,9 @@ aot_unload(AOTModule *module);
* @return return the instantiated AOT module instance, NULL if failed
*/
AOTModuleInstance *
aot_instantiate(AOTModule *module, bool is_sub_inst, WASMExecEnv *exec_env_main,
uint32 stack_size, uint32 heap_size, char *error_buf,
uint32 error_buf_size);
aot_instantiate(AOTModule *module, AOTModuleInstance *parent,
WASMExecEnv *exec_env_main, uint32 stack_size, uint32 heap_size,
char *error_buf, uint32 error_buf_size);

/**
* Deinstantiate a AOT module instance, destroy the resources.
Expand Down
12 changes: 5 additions & 7 deletions core/iwasm/common/wasm_memory.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -608,7 +608,7 @@ wasm_enlarge_memory_internal(WASMModuleInstance *module, uint32 inc_page_count)
}

#if WASM_ENABLE_SHARED_MEMORY != 0
if (memory->is_shared) {
if (shared_memory_is_shared(memory)) {
memory->num_bytes_per_page = num_bytes_per_page;
memory->cur_page_count = total_page_count;
memory->max_page_count = max_page_count;
Expand Down Expand Up @@ -769,15 +769,13 @@ wasm_enlarge_memory(WASMModuleInstance *module, uint32 inc_page_count)
bool ret = false;

#if WASM_ENABLE_SHARED_MEMORY != 0
WASMSharedMemNode *node =
wasm_module_get_shared_memory((WASMModuleCommon *)module->module);
if (node)
os_mutex_lock(&node->shared_mem_lock);
if (module->memory_count > 0)
shared_memory_lock(module->memories[0]);
#endif
ret = wasm_enlarge_memory_internal(module, inc_page_count);
#if WASM_ENABLE_SHARED_MEMORY != 0
if (node)
os_mutex_unlock(&node->shared_mem_lock);
if (module->memory_count > 0)
shared_memory_unlock(module->memories[0]);
#endif

return ret;
Expand Down
33 changes: 15 additions & 18 deletions core/iwasm/common/wasm_runtime_common.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1196,22 +1196,23 @@ wasm_runtime_unload(WASMModuleCommon *module)
}

WASMModuleInstanceCommon *
wasm_runtime_instantiate_internal(WASMModuleCommon *module, bool is_sub_inst,
wasm_runtime_instantiate_internal(WASMModuleCommon *module,
WASMModuleInstanceCommon *parent,
WASMExecEnv *exec_env_main, uint32 stack_size,
uint32 heap_size, char *error_buf,
uint32 error_buf_size)
{
#if WASM_ENABLE_INTERP != 0
if (module->module_type == Wasm_Module_Bytecode)
return (WASMModuleInstanceCommon *)wasm_instantiate(
(WASMModule *)module, is_sub_inst, exec_env_main, stack_size,
heap_size, error_buf, error_buf_size);
(WASMModule *)module, (WASMModuleInstance *)parent, exec_env_main,
stack_size, heap_size, error_buf, error_buf_size);
#endif
#if WASM_ENABLE_AOT != 0
if (module->module_type == Wasm_Module_AoT)
return (WASMModuleInstanceCommon *)aot_instantiate(
(AOTModule *)module, is_sub_inst, exec_env_main, stack_size,
heap_size, error_buf, error_buf_size);
(AOTModule *)module, (AOTModuleInstance *)parent, exec_env_main,
stack_size, heap_size, error_buf, error_buf_size);
#endif
set_error_buf(error_buf, error_buf_size,
"Instantiate module failed, invalid module type");
Expand All @@ -1224,7 +1225,7 @@ wasm_runtime_instantiate(WASMModuleCommon *module, uint32 stack_size,
uint32 error_buf_size)
{
return wasm_runtime_instantiate_internal(
module, false, NULL, stack_size, heap_size, error_buf, error_buf_size);
module, NULL, NULL, stack_size, heap_size, error_buf, error_buf_size);
}

void
Expand Down Expand Up @@ -2310,10 +2311,8 @@ wasm_set_exception(WASMModuleInstance *module_inst, const char *exception)
WASMExecEnv *exec_env = NULL;

#if WASM_ENABLE_SHARED_MEMORY != 0
WASMSharedMemNode *node =
wasm_module_get_shared_memory((WASMModuleCommon *)module_inst->module);
if (node)
os_mutex_lock(&node->shared_mem_lock);
if (module_inst->memory_count > 0)
shared_memory_lock(module_inst->memories[0]);
Comment on lines +2314 to +2315
Copy link
Collaborator

@xujuntwt95329 xujuntwt95329 Aug 3, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@wenyongh Do you remember why we need to lock the shared memory when setting exception?

Copy link
Contributor Author

@yamt yamt Aug 3, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

see #2407

Copy link
Collaborator

@wenyongh wenyongh Aug 3, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

@eloparco eloparco Aug 3, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

@xujuntwt95329 xujuntwt95329 Aug 3, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks a lot!

Comment on lines +2314 to +2315
Copy link
Collaborator

@xujuntwt95329 xujuntwt95329 Aug 3, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@wenyongh Do you remember why we need to lock the shared memory when setting exception?

#endif
if (exception) {
snprintf(module_inst->cur_exception, sizeof(module_inst->cur_exception),
Expand All @@ -2323,8 +2322,8 @@ wasm_set_exception(WASMModuleInstance *module_inst, const char *exception)
module_inst->cur_exception[0] = '\0';
}
#if WASM_ENABLE_SHARED_MEMORY != 0
if (node)
os_mutex_unlock(&node->shared_mem_lock);
if (module_inst->memory_count > 0)
shared_memory_unlock(module_inst->memories[0]);
#endif

#if WASM_ENABLE_THREAD_MGR != 0
Expand Down Expand Up @@ -2386,10 +2385,8 @@ wasm_copy_exception(WASMModuleInstance *module_inst, char *exception_buf)
bool has_exception = false;

#if WASM_ENABLE_SHARED_MEMORY != 0
WASMSharedMemNode *node =
wasm_module_get_shared_memory((WASMModuleCommon *)module_inst->module);
if (node)
os_mutex_lock(&node->shared_mem_lock);
if (module_inst->memory_count > 0)
shared_memory_lock(module_inst->memories[0]);
#endif
if (module_inst->cur_exception[0] != '\0') {
/* NULL is passed if the caller is not interested in getting the
Expand All @@ -2403,8 +2400,8 @@ wasm_copy_exception(WASMModuleInstance *module_inst, char *exception_buf)
has_exception = true;
}
#if WASM_ENABLE_SHARED_MEMORY != 0
if (node)
os_mutex_unlock(&node->shared_mem_lock);
if (module_inst->memory_count > 0)
shared_memory_unlock(module_inst->memories[0]);
#endif

return has_exception;
Expand Down
3 changes: 2 additions & 1 deletion core/iwasm/common/wasm_runtime_common.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -498,7 +498,8 @@ wasm_runtime_unload(WASMModuleCommon *module);

/* Internal API */
WASMModuleInstanceCommon *
wasm_runtime_instantiate_internal(WASMModuleCommon *module, bool is_sub_inst,
wasm_runtime_instantiate_internal(WASMModuleCommon *module,
WASMModuleInstanceCommon *parent,
WASMExecEnv *exec_env_main, uint32 stack_size,
uint32 heap_size, char *error_buf,
uint32 error_buf_size);
Expand Down
Loading