Disable aux stack allocations for threads spawned by wasi_thread_start by loganek · Pull Request #1867 · bytecodealliance/wasm-micro-runtime

loganek · 2023-01-06T12:06:47Z

This syscall doesn't need allocating stack or TLS and it's expected from the application to do that instead. E.g. WASI-libc already does this for pthread_create.

I also fixed some of the examples to allocate memory for stack and not use stack before the stack pointer is set to a correct value.

This syscall doesn't need allocating stack or TLS and it's expected from the application to do that instead.

For pthreads this is configured in wasi-libc, but since we don't use pthreads here, we have to do it explicitly. The wasi_thread_start code is implemented in wasm to make sure there's no stack operations before the stack pointer is set to a correct value.

yamt · 2023-01-06T12:41:07Z

+wasm_exec_env_is_aux_stack_managed_by_runtime(WASMExecEnv *exec_env)
+{
+    return exec_env->aux_stack_boundary.boundary != 0
+           || exec_env->aux_stack_bottom.bottom != 0;


i feel it's simpler to use a dedicated flag as 0 is a valid address in wasm. how do you think?

also, i guess aot needs an equivalent around create_aux_stack_info.

i feel it's simpler to use a dedicated flag as 0 is a valid address in wasm. how do you think?

Yeah, I did think about it. But I noticed that WAMR actually doesn't always respect 0 as a valid address; for example, it considers wasm_runtime_malloc() returning 0 as a failure. Flag was another option but didn't want to increase the size of the object unnecessarily if there was a way to achieve the same without it (if my assumption about WAMR not respecting 0 as a correct value holds).

also, i guess aot needs an equivalent around create_aux_stack_info.

That's correct. At the moment we're not testing AOT though so I was planning to do that (and other necessary things) in the follow-up PR. Does it make sense?

i feel it's simpler to use a dedicated flag as 0 is a valid address in wasm. how do you think?

Yeah, I did think about it. But I noticed that WAMR actually doesn't always respect 0 as a valid address; for example, it considers wasm_runtime_malloc() returning 0 as a failure. Flag was another option but didn't want to increase the size of the object unnecessarily if there was a way to achieve the same without it (if my assumption about WAMR not respecting 0 as a correct value holds).

ok.

also, i guess aot needs an equivalent around create_aux_stack_info.

That's correct. At the moment we're not testing AOT though so I was planning to do that (and other necessary things) in the follow-up PR. Does it make sense?

ok.

wenyongh · 2023-01-09T06:34:03Z

-                    wasm_set_exception(module,
-                                       "wasm auxiliary stack underflow");
-                    goto got_exception;
+                if (wasm_exec_env_is_aux_stack_managed_by_runtime(exec_env)) {


Do we need to apply similar changes for LLVM AOT/JIT and Fast JIT, refer to:

https://github.com/bytecodealliance/wasm-micro-runtime/blob/main/core/iwasm/compilation/aot_emit_variable.c#L192

https://github.com/bytecodealliance/wasm-micro-runtime/blob/main/core/iwasm/fast-jit/fe/jit_emit_variable.c#L264-L279

Yes we do; however, for now we focus on the interpreter and we'll work on JIT/AOT next.

OK, so let's merge this PR first?

Yes, we can merge this one and next we'll continue working on AOT and JIT

@wenyongh @yamt for AOT, should we disable the aux stack globally when WASI threads are enabled? by replacing this

wasm-micro-runtime/core/iwasm/compilation/aot_llvm.c

Line 1614 in 21ba730

comp_ctx->enable_aux_stack_check = true;

with something like

#if WASM_ENABLE_LIB_WASI_THREADS != 0 comp_ctx->enable_aux_stack_check = false; #else comp_ctx->enable_aux_stack_check = true; #endif

i guess it can be a workaround.
but i think it's better to avoid making the compiler depend on build-time configurations like this.

at this point, should we just specify in the documentation to compile with --disable-aux-stack-check when using WASI threads with AOT?
for fast JIT we could instead put an #if WASM_ENABLE_LIB_WASI_THREADS == 0 around here

wasm-micro-runtime/core/iwasm/fast-jit/fe/jit_emit_variable.c

Lines 264 to 280 in 622cdbe

if (is_aux_stack) {

JitReg aux_stack_bound = get_aux_stack_bound_reg(cc->jit_frame);

JitReg aux_stack_bottom =

get_aux_stack_bottom_reg(cc->jit_frame);

GEN_INSN(CMP, cc->cmp_reg, value, aux_stack_bound);

if (!(jit_emit_exception(cc, EXCE_AUX_STACK_OVERFLOW,

JIT_OP_BLEU, cc->cmp_reg, NULL)))

goto fail;

GEN_INSN(CMP, cc->cmp_reg, value, aux_stack_bottom);

if (!(jit_emit_exception(cc, EXCE_AUX_STACK_UNDERFLOW,

JIT_OP_BGTU, cc->cmp_reg, NULL)))

goto fail;

}

GEN_INSN(STI32, value, get_module_inst_reg(cc->jit_frame),

NEW_CONST(I32, data_offset));

break;

}

Describe how to use WASI threads in AOT mode, following the discussion below: #1867 (comment) Make aux stack boundary checks of wasi-threads always successful by setting `exec_env->aux_stack_bottom` to UINT32_MAX and `exec_env->aux_stack_boundary` to 0

bytecodealliance#1867) This syscall doesn't need allocating stack or TLS and it's expected from the application to do that instead. E.g. WASI-libc already does this for `pthread_create`. Also fix some of the examples to allocate memory for stack and not use stack before the stack pointer is set to a correct value.

Describe how to use WASI threads in AOT mode, following the discussion below: bytecodealliance#1867 (comment) Make aux stack boundary checks of wasi-threads always successful by setting `exec_env->aux_stack_bottom` to UINT32_MAX and `exec_env->aux_stack_boundary` to 0

Describe how to use WASI threads in AOT mode, following the discussion below: bytecodealliance/wasm-micro-runtime#1867 (comment) Make aux stack boundary checks of wasi-threads always successful by setting `exec_env->aux_stack_bottom` to UINT32_MAX and `exec_env->aux_stack_boundary` to 0

Do not allocate aux stack for threads spawned by wasi_thread_start

4ec4744

This syscall doesn't need allocating stack or TLS and it's expected from the application to do that instead.

loganek force-pushed the loganek/dont-allocate-aux-stack-for-wasi-threads branch from daf6367 to dfa58f8 Compare January 6, 2023 12:16

yamt reviewed Jan 6, 2023

View reviewed changes

Comment thread samples/wasi-threads/wasm-apps/no_pthread.c

Comment thread samples/wasi-threads/wasm-apps/wasi_thread_start.S

loganek force-pushed the loganek/dont-allocate-aux-stack-for-wasi-threads branch from dfa58f8 to cace517 Compare January 6, 2023 12:27

yamt reviewed Jan 6, 2023

View reviewed changes

yamt approved these changes Jan 6, 2023

View reviewed changes

wenyongh reviewed Jan 9, 2023

View reviewed changes

wenyongh merged commit 0e2382a into bytecodealliance:dev/wasi_threads Jan 9, 2023

eloparco mentioned this pull request Jan 20, 2023

Document how to use WASI threads in AOT mode #1905

Merged

loganek deleted the loganek/dont-allocate-aux-stack-for-wasi-threads branch June 10, 2024 12:48

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Disable aux stack allocations for threads spawned by wasi_thread_start#1867

Disable aux stack allocations for threads spawned by wasi_thread_start#1867
wenyongh merged 2 commits into
bytecodealliance:dev/wasi_threadsfrom
loganek:loganek/dont-allocate-aux-stack-for-wasi-threads

loganek commented Jan 6, 2023 •

edited

Loading

Uh oh!

Uh oh!

Uh oh!

yamt Jan 6, 2023

Uh oh!

loganek Jan 6, 2023

Uh oh!

yamt Jan 6, 2023

Uh oh!

wenyongh Jan 9, 2023

Uh oh!

loganek Jan 9, 2023

Uh oh!

wenyongh Jan 9, 2023

Uh oh!

loganek Jan 9, 2023

Uh oh!

eloparco Jan 13, 2023

Uh oh!

yamt Jan 14, 2023

Uh oh!

eloparco Jan 14, 2023 •

edited

Loading

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

	if (is_aux_stack) {
	JitReg aux_stack_bound = get_aux_stack_bound_reg(cc->jit_frame);
	JitReg aux_stack_bottom =
	get_aux_stack_bottom_reg(cc->jit_frame);
	GEN_INSN(CMP, cc->cmp_reg, value, aux_stack_bound);
	if (!(jit_emit_exception(cc, EXCE_AUX_STACK_OVERFLOW,
	JIT_OP_BLEU, cc->cmp_reg, NULL)))
	goto fail;
	GEN_INSN(CMP, cc->cmp_reg, value, aux_stack_bottom);
	if (!(jit_emit_exception(cc, EXCE_AUX_STACK_UNDERFLOW,
	JIT_OP_BGTU, cc->cmp_reg, NULL)))
	goto fail;
	}
	GEN_INSN(STI32, value, get_module_inst_reg(cc->jit_frame),
	NEW_CONST(I32, data_offset));
	break;
	}

Conversation

loganek commented Jan 6, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

eloparco Jan 14, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

loganek commented Jan 6, 2023 •

edited

Loading

eloparco Jan 14, 2023 •

edited

Loading