Skip to content

Implement sync primitives instead of using pthread ones #2028

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 6 commits into from
Mar 26, 2023
Merged

Implement sync primitives instead of using pthread ones #2028

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
c83b8b4
feat: implement sync primitives instead of using pthread ones
eloparco Mar 15, 2023
fcea51c
fix: notify all waiting threads
eloparco Mar 15, 2023
4f5ff0c
fix: reset barrier when threshold is reached
eloparco Mar 16, 2023
1227d42
feat: use preprocessor option to decide pthread primitive implementat…
eloparco Mar 19, 2023
c379e63
fix: remove outdated sample now part of the tests
eloparco Mar 19, 2023
6b2999f
feat: use pthread primitives by default and update ci to use wasi-lib…
eloparco Mar 26, 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
8 changes: 4 additions & 4 deletions .github/workflows/compilation_on_android_ubuntu.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -368,9 +368,9 @@ jobs:
mkdir wasi-libc
cd wasi-libc
git init
# "Rename thread_spawn import" commit on main branch
# "Fix a_store operation in atomic.h" commit on main branch
git fetch https://github.com/WebAssembly/wasi-libc \
8f5275796a82f8ecfd0833a4f3f444fa37ed4546
1dfe5c302d1c5ab621f7abf04620fae92700fd22
Copy link
Collaborator

@wenyongh wenyongh Mar 26, 2023

Choose a reason for hiding this comment

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

Had better also compile wasi-libc in Ubuntu-22.04 instead using the pre-release wasi-sdk-thread version since there is issue in it:
Merge L337 to L346, and remove L366 and

Copy link
Contributor Author

@eloparco eloparco Mar 26, 2023

Choose a reason for hiding this comment

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

I thought of that, but it would mean that now we remove the usage wasi-sdk 20 pre-release from the ci, basically reverting all the changes previously added in #2021. When wasi-sdk 20 final release comes out we will have to add those changes again.

I was leaving the wasi-sdk 20 part as it is since in practice we didn't encounter problems in the main branch, the hanging issues, only reproducible by running each test many times, always passed unnoticed in the CI.

Copy link
Collaborator

@wenyongh wenyongh Mar 26, 2023

Choose a reason for hiding this comment

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

Got it, so let's keep it.

git checkout FETCH_HEAD
make -j \
AR=/opt/wasi-sdk/bin/llvm-ar \
Expand Down Expand Up @@ -532,9 +532,9 @@ jobs:
mkdir wasi-libc
cd wasi-libc
git init
# "Rename thread_spawn import" commit on main branch
# "Fix a_store operation in atomic.h" commit on main branch
git fetch https://github.com/WebAssembly/wasi-libc \
8f5275796a82f8ecfd0833a4f3f444fa37ed4546
1dfe5c302d1c5ab621f7abf04620fae92700fd22
Copy link
Collaborator

@wenyongh wenyongh Mar 26, 2023

Choose a reason for hiding this comment

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

Same as above

git checkout FETCH_HEAD
make -j \
AR=/opt/wasi-sdk/bin/llvm-ar \
Expand Down
4 changes: 2 additions & 2 deletions .github/workflows/compilation_on_sgx.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -265,9 +265,9 @@ jobs:
mkdir wasi-libc
cd wasi-libc
git init
# "Rename thread_spawn import" commit on main branch
# "Fix a_store operation in atomic.h" commit on main branch
git fetch https://github.com/WebAssembly/wasi-libc \
8f5275796a82f8ecfd0833a4f3f444fa37ed4546
1dfe5c302d1c5ab621f7abf04620fae92700fd22
git checkout FETCH_HEAD
make \
AR=/opt/wasi-sdk/bin/llvm-ar \
Expand Down
7 changes: 6 additions & 1 deletion core/iwasm/libraries/lib-wasi-threads/test/common.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,11 +6,16 @@
#include <stdlib.h>
#include <stdio.h>
#include <assert.h>
#include <pthread.h>
#include <stdbool.h>
#include <unistd.h>
#include <limits.h>

#if USE_CUSTOM_SYNC_PRIMITIVES != 0
#include "sync_primitives.h"
Copy link
Contributor

@g0djan g0djan Mar 20, 2023

Choose a reason for hiding this comment

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

@wenyongh @eloparco

I'd like to use this primitives to easier find the other errors, but do we want to use it as default one though?

Pros:

  • stable CI, if people break something they will be sure it's because of their changes

Cons:

  • now pthreads spot the problems that need to be fixed anyway but we would hide it

Copy link
Collaborator

@wenyongh wenyongh Mar 22, 2023

Choose a reason for hiding this comment

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

Yes, had better use the pthread_barrier_wait after the hang issue is resolved. But note that there will be some normal load/store data races in wasi-libc implementation, e.g. a_swap:
https://github.com/WebAssembly/wasi-libc/blob/main/libc-top-half/musl/src/internal/atomic.h#L106-L115

	int old;
	do old = *p;
	while (a_cas(p, old, v) != old);
	return old;

Here old = *p is not atomic, and a_cas(p, old, v) is atomic, if two threads operate on the same address p, data races may be reported by sanitizer.

Copy link
Contributor Author

@eloparco eloparco Mar 22, 2023

Choose a reason for hiding this comment

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

Yes, had better use the pthread_barrier_wait after the hang issue is resolved.

So should we merge this one as it is then? Until we get it fixed in wasi-libc.

Here old = *p is not atomic, and a_cas(p, old, v) is atomic, if two threads operate on the same address p, data races may be reported by sanitizer.

Nice catch, that's probably what the thread sanitizer is complaining about for the load/store data races. It may be the same for other operations as well, I see the a_fetch_add a few lines later doing something similar.

#else
#include <pthread.h>
#endif

#include "wasi_thread_start.h"

typedef enum {
Expand Down
5 changes: 5 additions & 0 deletions core/iwasm/libraries/lib-wasi-threads/test/global_lock.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,12 @@
#include <stdio.h>
#include <assert.h>
#include <stdbool.h>

#if USE_CUSTOM_SYNC_PRIMITIVES != 0
#include "sync_primitives.h"
#else
#include <pthread.h>
#endif

#include "wasi_thread_start.h"

Expand Down
91 changes: 91 additions & 0 deletions core/iwasm/libraries/lib-wasi-threads/test/sync_primitives.h
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,91 @@
/*
* Copyright (C) 2023 Amazon.com Inc. or its affiliates. All rights reserved.
* SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
*/

#include <stdbool.h>

/* Mutex */

typedef int pthread_mutex_t;

int
pthread_mutex_init(pthread_mutex_t *mutex, void *unused)
{
*mutex = 0;
return 0;
}

int
pthread_mutex_destroy(pthread_mutex_t *mutex)
{
return 0;
}

static bool
try_pthread_mutex_lock(pthread_mutex_t *mutex)
{
int expected = 0;
return __atomic_compare_exchange_n(mutex, &expected, 1, false,
__ATOMIC_SEQ_CST, __ATOMIC_SEQ_CST);
}

int
pthread_mutex_lock(pthread_mutex_t *mutex)
{
while (!try_pthread_mutex_lock(mutex))
__builtin_wasm_memory_atomic_wait32(mutex, 1, -1);
return 0;
}

int
pthread_mutex_unlock(pthread_mutex_t *mutex)
{
__atomic_store_n(mutex, 0, __ATOMIC_SEQ_CST);
__builtin_wasm_memory_atomic_notify(mutex, 1);
return 0;
}

/* Barrier */

typedef struct {
int count;
int num_threads;
int mutex;
int ready;
} pthread_barrier_t;

int
pthread_barrier_init(pthread_barrier_t *barrier, void *unused, int num_threads)
{
barrier->count = 0;
barrier->num_threads = num_threads;
barrier->ready = 0;
pthread_mutex_init(&barrier->mutex, NULL);

return 0;
}

int
pthread_barrier_wait(pthread_barrier_t *barrier)
{
bool no_wait = false;
int count;

pthread_mutex_lock(&barrier->mutex);
count = barrier->count++;
if (barrier->count >= barrier->num_threads) {
no_wait = true;
barrier->count = 0;
}
pthread_mutex_unlock(&barrier->mutex);

if (no_wait) {
__atomic_store_n(&barrier->ready, 1, __ATOMIC_SEQ_CST);
__builtin_wasm_memory_atomic_notify(&barrier->ready, count);
return 0;
}

__builtin_wasm_memory_atomic_wait32(&barrier->ready, 0, -1);
return 0;
}
2 changes: 0 additions & 2 deletions samples/wasi-threads/README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,8 +11,6 @@ $ cmake ..
$ make
...
$ ./iwasm wasm-apps/no_pthread.wasm
...
$ ./iwasm wasm-apps/exception_propagation.wasm
```

## Run samples in AOT mode
Expand Down
3 changes: 1 addition & 2 deletions samples/wasi-threads/wasm-apps/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,5 +43,4 @@ function (compile_sample SOURCE_FILE)
)
endfunction ()

compile_sample(no_pthread.c wasi_thread_start.S)
compile_sample(thread_termination.c wasi_thread_start.S)
compile_sample(no_pthread.c wasi_thread_start.S)
142 changes: 0 additions & 142 deletions samples/wasi-threads/wasm-apps/thread_termination.c
View file
Open in desktop

This file was deleted.