Allow interrupting blocking instructions #1930

eloparco · 2023-02-02T01:11:34Z

Implementing #1910

eloparco · 2023-02-02T01:34:16Z

I tested different combinations of blocking from main thread or from spawn thread, interpreter or aot, with or without hw bound check enabled.
So far the combination that doesn't seem to work is aot + hw bound check disabled, where I see that the exit code is not the one expected. I'll keep investigating.
@wenyongh

wenyongh · 2023-02-02T02:30:42Z

I tested different combinations of blocking from main thread or from spawn thread, interpreter or aot, with or without hw bound check enabled. So far the combination that doesn't seem to work is aot + hw bound check disabled, where I see that the exit code is not the one expected. I'll keep investigating. @wenyongh

@eloparco Thanks a lot, the code is much better now, I will look into it soon.

build-scripts/config_common.cmake

core/iwasm/aot/aot_runtime.c

core/iwasm/common/wasm_runtime_common.c

core/shared/platform/android/platform_internal.h

core/shared/platform/windows/platform_internal.h

core/iwasm/aot/aot_runtime.c

build-scripts/config_common.cmake

core/iwasm/aot/aot_runtime.c

core/iwasm/common/wasm_runtime_common.c

core/iwasm/libraries/thread-mgr/thread_manager.c

core/iwasm/common/wasm_runtime_common.c

core/iwasm/aot/aot_runtime.c

yamt · 2023-02-03T06:24:00Z

core/iwasm/aot/aot_runtime.c

+#ifdef OS_ENABLE_BLOCK_INSN_INTERRUPT
+        }
+        else {
+            ret = false;


who frees eg. "argv1" in wasm_runtime_invoke_native on a termination?

while OS_ENABLE_HW_BOUND_CHECK seems to have the same problem,
it seems much more complex to fix for OS_ENABLE_BLOCK_INSN_INTERRUPT because it's an async event.

Yes, this is a potential issue, maybe there are two ways to resolve it: (1) avoid allocating memory in wasm_runtime_invoke_native, just define a large enough buffer for argv, (2) add a list in exec_env, add the allocating the memory to the list, and free it later.

(2) needs to block the signal for OS_ENABLE_BLOCK_INSN_INTERRUPT.
maybe we should invent a convenient api for common cases like memory allocation.

Are you referring to the call at L1396? Isn't it freed at L1458 as before my changes?

I made a try with valgrind using interpreter and AOT, with or without HW bound check and no memory leakage is detected. Unfortunately, in case of AOT I see many warnings about invalid memory accesses that may explain the weird behavior that I reported in another comment.

I will merge it to main branch as soon as possible when (2) is done and well tested.

At this point, we can probably just have the feature disabled by default until it's well tested.

the only simple solution i can think of right now involves block/unblock of the signal.

The tricky part is that it's not always possible to just block signals before the malloc and unblock after the free, because the blocking instruction can be in the middle. Apart from argv1 that we could avoid allocating dynamically, the problem is in the native function calls.

Not sure what's the best solution there, maybe the approach of keeping a list in exec_env is not that bad. The memory that is not freed stays in the list and gets released only at the end.

Yes, suggest to add a list of allocated memories after setjmp in the exec_env.
@eloparco I have created branch dev/interrupt_block_insn, could you upload another patch to it? Or do you mind I refine some code and upload the PR by myself?

Thanks @wenyongh, I pushed the changes I made so far in #1946. Not removing this one so that I can keep trying the changes on the wasi thread examples.

@eloparco I submitted PR #1947 to refactor the feature, please help review, thanks.

Sorry, the source branch and dest branch are invalid, closed it and submitted #1948 instead

…instruction interrupt" wrapper

…mple

core/iwasm/aot/aot_runtime.c

yamt · 2023-02-06T08:46:19Z

core/iwasm/common/wasm_runtime_common.c

+static void
+interrupt_block_insn_sig_handler()
+{
+    WASMJmpBuf *jmpbuf_node = exec_env_tls->jmpbuf_stack_top;


jmpbuf_stack_top here can be for another handler.

Yes, if HW bound check is enabled, it will get a "HW bound" jmpbuf #1930 (comment), but that should not be a problem unless I'm missing something

you can even get a signal between wasm_exec_env_push_jmpbuf and os_setjmp.

you can even get a signal between wasm_exec_env_push_jmpbuf and os_setjmp.

In that case, the jmpbuf would be invalid, as it is in general before being set up by os_setjmp. Isn't that the same for the HW bound check before my PR?

I wonder if we have to mask/un-mask signals to avoid the signal handler being called in the middle. We want to avoid signal handlers being called between wasm_exec_env_push_jmpbuf and os_setjmp as they would use a non-initialized jmpbuf iiuc.

it isn't a problem for hw bound check because the signal for it is basically synchronous to the thread execution. ie. the thread itself can control where it can happen.

Can we maybe invert the order of wasm_exec_env_push_jmpbuf and os_setjmp? So that the jmpbuf is only pushed after being initialized by os_setjmp.

But there are probably other cases that are not covered, like an exception before os_setjmp. [EDIT] These cases shouldn't be a problem since they're already handled by the normal exception spreading mechanism.

Can we maybe invert the order of wasm_exec_env_push_jmpbuf and os_setjmp? So that the jmpbuf is only pushed after being initialized by os_setjmp.

I tried that in the latest commit, but maybe it's safer to use something like static volatile sig_atomic_t canjump; (but with tls) as explained here https://notes.shichao.io/apue/ch10/?

you can even get a signal between wasm_exec_env_push_jmpbuf and os_setjmp.

Apart from this, do you think there are other cases that are not covered? If we get the signal before os_setjmp, the signal handler (for instruction interruption) will return and the thread will stop when starting to execute instructions, using the normal exception propagation mechanism.

yamt · 2023-02-06T08:55:59Z

core/iwasm/aot/aot_runtime.c

+#ifdef OS_ENABLE_BLOCK_INSN_INTERRUPT
+        }
+        else {
+            ret = false;


Are you referring to the call at L1396? Isn't it freed at L1458 as before my changes?

i'm not sure which file and version your line numbers are for.

i meant eg.

wasm-micro-runtime/core/iwasm/common/wasm_runtime_common.c

Line 3506 in 2c8375e

if (!(argv1 = runtime_malloc((uint32)size, exec_env->module_inst, NULL,

wasm-micro-runtime/core/iwasm/common/wasm_runtime_common.c

Line 3790 in 2c8375e

wasm_runtime_free(argv1);

when it escapes with a long jump, it might not be executed.

Move "blocking instruction interruption" implementation from #1930

wenyongh · 2023-02-13T07:12:44Z

Test this feature with branch dev/interrupt_block_insn, and PR #1946, #1948 were merged.

eloparco mentioned this pull request Feb 2, 2023

Interrupt blocking instructions #1921

Closed

fix(wasi-threads): interrupt blocking instructions using signals

3c2f060

eloparco force-pushed the eloparco/allow-interrupting-blocking-instructions branch from ad7582f to 3c2f060 Compare February 2, 2023 01:21

loganek reviewed Feb 2, 2023

View reviewed changes

build-scripts/config_common.cmake Show resolved Hide resolved

wenyongh reviewed Feb 2, 2023

View reviewed changes

xujuntwt95329 reviewed Feb 2, 2023

View reviewed changes

core/iwasm/aot/aot_runtime.c Outdated Show resolved Hide resolved

eloparco force-pushed the eloparco/allow-interrupting-blocking-instructions branch 3 times, most recently from dea3508 to f6ef4e7 Compare February 3, 2023 01:39

fix(wasi-threads): fix platform includes

185cd4d

eloparco force-pushed the eloparco/allow-interrupting-blocking-instructions branch from f6ef4e7 to 185cd4d Compare February 3, 2023 02:25

wenyongh reviewed Feb 3, 2023

View reviewed changes

yamt reviewed Feb 3, 2023

View reviewed changes

eloparco added 2 commits February 4, 2023 01:43

refactor(wasi-threads): move platform-related code and remove "block …

f9c0bd1

…instruction interrupt" wrapper

feat(wasi-threads): use blocking operations in thread termination exa…

6eecad7

…mple

hritikgupta reviewed Feb 5, 2023

View reviewed changes

core/iwasm/aot/aot_runtime.c Show resolved Hide resolved

yamt reviewed Feb 6, 2023

View reviewed changes

wenyongh mentioned this pull request Feb 7, 2023

fixed terminating stale threads on trap/proc_exit #1929

Merged

fix(wasi-threads): handle signal handler being called before os_setjmp

9d6dc29

eloparco mentioned this pull request Feb 9, 2023

feat: interrupt blocking instructions using signals #1946

Merged

wenyongh pushed a commit that referenced this pull request Feb 9, 2023

feat: Interrupt blocking instructions using signals (#1946)

753712f

Move "blocking instruction interruption" implementation from #1930

wenyongh closed this Feb 13, 2023

Allow interrupting blocking instructions #1930

Allow interrupting blocking instructions #1930

Uh oh!

Conversation

eloparco commented Feb 2, 2023

Uh oh!

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

Uh oh!

Uh oh!

wenyongh commented Feb 2, 2023

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

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!

eloparco Feb 8, 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!

Choose a reason for hiding this comment

Uh oh!

eloparco Feb 9, 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

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!

eloparco Feb 6, 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!

Choose a reason for hiding this comment

Uh oh!

eloparco Feb 7, 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!

eloparco Feb 7, 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!

Choose a reason for hiding this comment

Uh oh!

wenyongh commented Feb 13, 2023

Uh oh!

Uh oh!

eloparco commented Feb 2, 2023 •

edited

Loading

eloparco Feb 8, 2023 •

edited

Loading

eloparco Feb 9, 2023 •

edited

Loading

eloparco Feb 6, 2023 •

edited

Loading

eloparco Feb 7, 2023 •

edited

Loading

eloparco Feb 7, 2023 •

edited

Loading