[PATCH v2 0/5] MR741: Draft: ntdll: Implement NtFlushProcessWriteBuffers.
Based on [a patch](https://www.winehq.org/mailman3/hyperkitty/list/wine-devel(a)winehq.org/mess...) by Jinoh Kang (@iamahuman) from February 2022. I removed the need for the event object and implemented fast paths for Linux. On Linux 4.14+ `membarrier(MEMBARRIER_CMD_GLOBAL_EXPEDITED, ...)` is used. On x86 Linux <= 4.13 `madvise(..., MADV_DONTNEED)` is used, which sends IPIs to all cores causing them to do a memory barrier. On non-x86 Linux 4.3-4.13 `membarrier(MEMBARRIER_CMD_SHARED, ...)` is used. On non-x86 Linux <= 4.2 and on other platforms the fallback path using APCs is used. -- v2: ntdll: Add thread_get_register_pointer_values-based fast path for NtFlushProcessWriteBuffers. ntdll: Add sys_membarrier-based fast path to NtFlushProcessWriteBuffers. ntdll: Add MADV_DONTNEED-based fast path for NtFlushProcessWriteBuffers. ntdll: Make server_select a memory barrier. ntdll: Implement NtFlushProcessWriteBuffers. https://gitlab.winehq.org/wine/wine/-/merge_requests/741
From: Torge Matthies <tmatthies(a)codeweavers.com> Based on a patch by Jinoh Kang from February 2022 [1]. The following description is copied from said patch: NtFlushProcessWriteBuffers is the NT equivalent of Linux membarrier() system call. The .NET Framework garbage collector uses it to synchronize with other threads, and thus is required to avoid silent memory corruption. [1] https://www.winehq.org/mailman3/hyperkitty/list/wine-devel(a)winehq.org/mess... --- dlls/ntdll/unix/server.c | 6 ++ dlls/ntdll/unix/virtual.c | 22 ++++++- server/protocol.def | 19 +++++- server/thread.c | 135 ++++++++++++++++++++++++++++++++++++++ server/thread.h | 2 + 5 files changed, 180 insertions(+), 4 deletions(-) diff --git a/dlls/ntdll/unix/server.c b/dlls/ntdll/unix/server.c index 77e8d5c7566..51a83f472e1 100644 --- a/dlls/ntdll/unix/server.c +++ b/dlls/ntdll/unix/server.c @@ -574,6 +574,12 @@ static void invoke_system_apc( const apc_call_t *call, apc_result_t *result, BOO if (!self) NtClose( wine_server_ptr_handle(call->dup_handle.dst_process) ); break; } + case APC_MEMORY_BARRIER: + { + MemoryBarrier(); + result->type = call->type; + break; + } default: server_protocol_error( "get_apc_request: bad type %d\n", call->type ); break; diff --git a/dlls/ntdll/unix/virtual.c b/dlls/ntdll/unix/virtual.c index 96a5e095d16..2b1581d767a 100644 --- a/dlls/ntdll/unix/virtual.c +++ b/dlls/ntdll/unix/virtual.c @@ -5020,8 +5020,26 @@ NTSTATUS WINAPI NtFlushInstructionCache( HANDLE handle, const void *addr, SIZE_T */ void WINAPI NtFlushProcessWriteBuffers(void) { - static int once = 0; - if (!once++) FIXME( "stub\n" ); + NTSTATUS status; + + MemoryBarrier(); + + do + { + select_op_t select_op; + + SERVER_START_REQ( flush_process_write_buffers ) + { + status = wine_server_call( req ); + } + SERVER_END_REQ; + if (status) continue; + + select_op.membarrier.op = SELECT_MEMBARRIER; + status = server_select( &select_op, sizeof(select_op.membarrier), SELECT_INTERRUPTIBLE, + TIMEOUT_INFINITE, NULL, NULL ); + } + while (status); } diff --git a/server/protocol.def b/server/protocol.def index d828d41d1f7..eb062d3c7a3 100644 --- a/server/protocol.def +++ b/server/protocol.def @@ -462,7 +462,8 @@ enum select_op SELECT_WAIT_ALL, SELECT_SIGNAL_AND_WAIT, SELECT_KEYED_EVENT_WAIT, - SELECT_KEYED_EVENT_RELEASE + SELECT_KEYED_EVENT_RELEASE, + SELECT_MEMBARRIER }; typedef union @@ -485,6 +486,10 @@ typedef union obj_handle_t handle; client_ptr_t key; } keyed_event; + struct + { + enum select_op op; /* SELECT_MEMBARRIER */ + } membarrier; } select_op_t; enum apc_type @@ -502,7 +507,8 @@ enum apc_type APC_MAP_VIEW, APC_UNMAP_VIEW, APC_CREATE_THREAD, - APC_DUP_HANDLE + APC_DUP_HANDLE, + APC_MEMORY_BARRIER }; typedef struct @@ -611,6 +617,10 @@ typedef union unsigned int attributes; /* object attributes */ unsigned int options; /* duplicate options */ } dup_handle; + struct + { + enum apc_type type; /* APC_MEMORY_BARRIER */ + } memory_barrier; } apc_call_t; typedef union @@ -1610,6 +1620,11 @@ enum server_fd_type @END +/* Issue a memory barrier on other threads in the same process */ +(a)REQ(flush_process_write_buffers) +(a)END + + struct thread_info { timeout_t start_time; diff --git a/server/thread.c b/server/thread.c index f49fbf40b78..4f159da357b 100644 --- a/server/thread.c +++ b/server/thread.c @@ -112,6 +112,42 @@ static const struct object_ops thread_apc_ops = thread_apc_destroy /* destroy */ }; +/* process-wide memory barriers */ + +struct thread_membarrier +{ + struct object obj; /* object header */ + int pending; +}; + +static void dump_thread_membarrier( struct object *obj, int verbose ); +static int thread_membarrier_signaled( struct object *obj, struct wait_queue_entry *entry ); +static struct thread_membarrier *create_membarrier( void ); + +static const struct object_ops thread_membarrier_ops = +{ + sizeof(struct thread_membarrier), /* size */ + &no_type, /* type */ + dump_thread_membarrier, /* dump */ + add_queue, /* add_queue */ + remove_queue, /* remove_queue */ + thread_membarrier_signaled, /* signaled */ + no_satisfied, /* satisfied */ + no_signal, /* signal */ + no_get_fd, /* get_fd */ + default_map_access, /* map_access */ + default_get_sd, /* get_sd */ + default_set_sd, /* set_sd */ + no_get_full_name, /* get_full_name */ + no_lookup_name, /* lookup_name */ + no_link_name, /* link_name */ + NULL, /* unlink_name */ + no_open_file, /* open_file */ + no_kernel_obj_list, /* get_kernel_obj_list */ + no_close_handle, /* close_handle */ + no_destroy /* destroy */ +}; + /* thread CPU context */ @@ -232,6 +268,7 @@ static inline void init_thread_structure( struct thread *thread ) thread->system_regs = 0; thread->queue = NULL; thread->wait = NULL; + thread->membarrier = NULL; thread->error = 0; thread->req_data = NULL; thread->req_toread = 0; @@ -360,6 +397,12 @@ struct thread *create_thread( int fd, struct process *process, const struct secu release_object( thread ); return NULL; } + if (!(thread->membarrier = create_membarrier())) + { + close( fd ); + release_object( thread ); + return NULL; + } if (!(thread->request_fd = create_anonymous_fd( &thread_fd_ops, fd, &thread->obj, 0 ))) { release_object( thread ); @@ -415,6 +458,7 @@ static void cleanup_thread( struct thread *thread ) } clear_apc_queue( &thread->system_apc ); clear_apc_queue( &thread->user_apc ); + if (thread->membarrier) release_object( thread->membarrier ); free( thread->req_data ); free( thread->reply_data ); if (thread->request_fd) release_object( thread->request_fd ); @@ -433,6 +477,7 @@ static void cleanup_thread( struct thread *thread ) } } free( thread->desc ); + thread->membarrier = NULL; thread->req_data = NULL; thread->reply_data = NULL; thread->request_fd = NULL; @@ -526,6 +571,30 @@ static struct thread_apc *create_apc( struct object *owner, const apc_call_t *ca return apc; } +static void dump_thread_membarrier( struct object *obj, int verbose ) +{ + struct thread_membarrier *mb = (struct thread_membarrier *)obj; + assert( obj->ops == &thread_membarrier_ops ); + + fprintf( stderr, "MB pending=%u\n", mb->pending ); +} + +static int thread_membarrier_signaled( struct object *obj, struct wait_queue_entry *entry ) +{ + struct thread_membarrier *mb = (struct thread_membarrier *)obj; + return !mb->pending; +} + +/* create a thread_membarrier object */ +static struct thread_membarrier *create_membarrier( void ) +{ + struct thread_membarrier *mb; + + if ((mb = alloc_object( &thread_membarrier_ops ))) + mb->pending = 0; + return mb; +} + /* get a thread pointer from a thread id (and increment the refcount) */ struct thread *get_thread_from_id( thread_id_t id ) { @@ -1029,6 +1098,13 @@ static int select_on( const select_op_t *select_op, data_size_t op_size, client_ current->wait->key = select_op->keyed_event.key; break; + case SELECT_MEMBARRIER: + object = ¤t->membarrier->obj; + if (!object) return 1; + ret = wait_on( select_op, 1, &object, flags, when ); + if (!ret) return 1; + break; + default: set_error( STATUS_INVALID_PARAMETER ); return 1; @@ -1165,6 +1241,20 @@ int thread_queue_apc( struct process *process, struct thread *thread, struct obj return ret; } +static void finish_membarrier_apc( struct thread_apc *apc ) +{ + struct thread *caller = apc->caller; + + assert( caller ); + + if (caller->membarrier) + { + assert( caller->membarrier->pending > 0 ); + if (!--caller->membarrier->pending) + wake_up( &caller->membarrier->obj, 0 ); + } +} + /* cancel the async procedure call owned by a specific object */ void thread_cancel_apc( struct thread *thread, struct object *owner, enum apc_type type ) { @@ -1176,6 +1266,8 @@ void thread_cancel_apc( struct thread *thread, struct object *owner, enum apc_ty if (apc->owner != owner) continue; list_remove( &apc->entry ); apc->executed = 1; + if (apc->call.type == APC_MEMORY_BARRIER) + finish_membarrier_apc( apc ); wake_up( &apc->obj, 0 ); release_object( apc ); return; @@ -1206,6 +1298,8 @@ static void clear_apc_queue( struct list *queue ) struct thread_apc *apc = LIST_ENTRY( ptr, struct thread_apc, entry ); list_remove( &apc->entry ); apc->executed = 1; + if (apc->call.type == APC_MEMORY_BARRIER) + finish_membarrier_apc( apc ); wake_up( &apc->obj, 0 ); release_object( apc ); } @@ -1650,6 +1744,8 @@ DECL_HANDLER(select) apc->result.create_thread.handle = handle; clear_error(); /* ignore errors from the above calls */ } + if (apc->call.type == APC_MEMORY_BARRIER) /* wake up caller if membarriers done */ + finish_membarrier_apc( apc ); wake_up( &apc->obj, 0 ); close_handle( current->process, req->prev_apc ); release_object( apc ); @@ -1671,6 +1767,8 @@ DECL_HANDLER(select) else { apc->executed = 1; + if (apc->call.type == APC_MEMORY_BARRIER) + finish_membarrier_apc( apc ); wake_up( &apc->obj, 0 ); } release_object( apc ); @@ -2012,3 +2110,40 @@ DECL_HANDLER(get_next_thread) set_error( STATUS_NO_MORE_ENTRIES ); release_object( process ); } + +DECL_HANDLER(flush_process_write_buffers) +{ + struct process *process = current->process; + struct thread_membarrier *membarrier = current->membarrier; + struct thread *thread; + apc_call_t call; + + assert( membarrier ); + + memset( &call, 0, sizeof(call) ); + call.memory_barrier.type = APC_MEMORY_BARRIER; + + LIST_FOR_EACH_ENTRY( thread, &process->thread_list, struct thread, proc_entry ) + { + struct thread_apc *apc; + int success; + + if (thread == current || thread->state == TERMINATED) continue; + + if (!(apc = create_apc( &membarrier->obj, &call ))) break; + + if ((success = queue_apc( NULL, thread, apc ))) + { + apc->caller = (struct thread *)grab_object( current ); + membarrier->pending++; + } + + release_object( apc ); + + if (!success) + { + set_error( STATUS_UNSUCCESSFUL ); + break; + } + } +} diff --git a/server/thread.h b/server/thread.h index 8dcf966a90a..d7369134f42 100644 --- a/server/thread.h +++ b/server/thread.h @@ -28,6 +28,7 @@ struct process; struct thread_wait; struct thread_apc; +struct thread_membarrier; struct debug_obj; struct debug_event; struct msg_queue; @@ -59,6 +60,7 @@ struct thread struct thread_wait *wait; /* current wait condition if sleeping */ struct list system_apc; /* queue of system async procedure calls */ struct list user_apc; /* queue of user async procedure calls */ + struct thread_membarrier *membarrier; /* membarrier object for this thread */ struct inflight_fd inflight[MAX_INFLIGHT_FDS]; /* fds currently in flight */ unsigned int error; /* current error code */ union generic_request req; /* current request */ -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/741
From: Torge Matthies <tmatthies(a)codeweavers.com> --- dlls/ntdll/unix/server.c | 3 +++ dlls/ntdll/unix/virtual.c | 4 ++-- 2 files changed, 5 insertions(+), 2 deletions(-) diff --git a/dlls/ntdll/unix/server.c b/dlls/ntdll/unix/server.c index 51a83f472e1..362c7793cbc 100644 --- a/dlls/ntdll/unix/server.c +++ b/dlls/ntdll/unix/server.c @@ -602,6 +602,9 @@ unsigned int server_select( const select_op_t *select_op, data_size_t size, UINT sigset_t old_set; int signaled; + /* ensure writes so far are visible to other threads */ + MemoryBarrier(); + memset( &result, 0, sizeof(result) ); do diff --git a/dlls/ntdll/unix/virtual.c b/dlls/ntdll/unix/virtual.c index 2b1581d767a..09fb8eaf4a6 100644 --- a/dlls/ntdll/unix/virtual.c +++ b/dlls/ntdll/unix/virtual.c @@ -5022,8 +5022,6 @@ void WINAPI NtFlushProcessWriteBuffers(void) { NTSTATUS status; - MemoryBarrier(); - do { select_op_t select_op; @@ -5040,6 +5038,8 @@ void WINAPI NtFlushProcessWriteBuffers(void) TIMEOUT_INFINITE, NULL, NULL ); } while (status); + + /* server_select is a memory barrier, so no need to call MemoryBarrier() */ } -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/741
From: Torge Matthies <tmatthies(a)codeweavers.com> Credits to Avi Kivity (scylladb) and Aliaksei Kandratsenka (gperftools) for this trick, see [1]. [1] https://github.com/scylladb/seastar/commit/77a58e4dc020233f66fccb8d9e8f7a8b7... --- dlls/ntdll/unix/virtual.c | 54 ++++++++++++++++++++++++++++++++++++--- 1 file changed, 50 insertions(+), 4 deletions(-) diff --git a/dlls/ntdll/unix/virtual.c b/dlls/ntdll/unix/virtual.c index 09fb8eaf4a6..eba6be62337 100644 --- a/dlls/ntdll/unix/virtual.c +++ b/dlls/ntdll/unix/virtual.c @@ -214,6 +214,11 @@ struct range_entry static struct range_entry *free_ranges; static struct range_entry *free_ranges_end; +#if defined(__linux__) && (defined(__i386__) || defined(__x86_64__)) +static void *dontneed_page; +static pthread_mutex_t dontneed_page_mutex = PTHREAD_MUTEX_INITIALIZER; +#endif + static inline BOOL is_beyond_limit( const void *addr, size_t size, const void *limit ) { @@ -5015,10 +5020,40 @@ NTSTATUS WINAPI NtFlushInstructionCache( HANDLE handle, const void *addr, SIZE_T } -/********************************************************************** - * NtFlushProcessWriteBuffers (NTDLL.@) - */ -void WINAPI NtFlushProcessWriteBuffers(void) +#if defined(__linux__) && (defined(__i386__) || defined(__x86_64__)) +static int try_madvise( void ) +{ + int ret = 0; + char *mem; + + pthread_mutex_lock(&dontneed_page_mutex); + /* Credits to Avi Kivity (scylladb) and Aliaksei Kandratsenka (gperftools) for this trick, + * see https://github.com/scylladb/seastar/commit/77a58e4dc020233f66fccb8d9e8f7a8b7... */ + mem = dontneed_page; + if (!mem) + { + mem = anon_mmap_alloc( page_size, PROT_READ | PROT_WRITE ); + if (mem == MAP_FAILED) + goto failed; + if (mlock( mem, page_size )) + { + munmap( mem, page_size ); + goto failed; + } + dontneed_page = mem; + } + *mem = 3; + ret = !madvise( mem, page_size, MADV_DONTNEED ); +failed: + pthread_mutex_unlock(&dontneed_page_mutex); + return ret; +} +#else +static int try_madvise( void ) { return 0; } +#endif + + +static void do_apc_memorybarrier( void ) { NTSTATUS status; @@ -5043,6 +5078,17 @@ void WINAPI NtFlushProcessWriteBuffers(void) } +/********************************************************************** + * NtFlushProcessWriteBuffers (NTDLL.@) + */ +void WINAPI NtFlushProcessWriteBuffers(void) +{ + if (try_madvise()) + return; + do_apc_memorybarrier(); +} + + /********************************************************************** * NtCreatePagingFile (NTDLL.@) */ -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/741
From: Torge Matthies <tmatthies(a)codeweavers.com> Uses the MEMBARRIER_CMD_PRIVATE_EXPEDITED membarrier command introduced in Linux 4.14. --- dlls/ntdll/unix/virtual.c | 49 ++++++++++++++++++++++++++++++++++++++- 1 file changed, 48 insertions(+), 1 deletion(-) diff --git a/dlls/ntdll/unix/virtual.c b/dlls/ntdll/unix/virtual.c index eba6be62337..fa4216e7f9d 100644 --- a/dlls/ntdll/unix/virtual.c +++ b/dlls/ntdll/unix/virtual.c @@ -39,6 +39,9 @@ #ifdef HAVE_SYS_SYSINFO_H # include <sys/sysinfo.h> #endif +#ifdef HAVE_SYS_SYSCALL_H +# include <sys/syscall.h> +#endif #ifdef HAVE_SYS_SYSCTL_H # include <sys/sysctl.h> #endif @@ -214,10 +217,16 @@ struct range_entry static struct range_entry *free_ranges; static struct range_entry *free_ranges_end; -#if defined(__linux__) && (defined(__i386__) || defined(__x86_64__)) +#ifdef __linux__ +#ifdef __NR_membarrier +static BOOL membarrier_exp_available; +static pthread_once_t membarrier_init_once = PTHREAD_ONCE_INIT; +#endif +#if defined(__i386__) || defined(__x86_64__) static void *dontneed_page; static pthread_mutex_t dontneed_page_mutex = PTHREAD_MUTEX_INITIALIZER; #endif +#endif static inline BOOL is_beyond_limit( const void *addr, size_t size, const void *limit ) @@ -5020,6 +5029,42 @@ NTSTATUS WINAPI NtFlushInstructionCache( HANDLE handle, const void *addr, SIZE_T } +#if defined(__linux__) && defined(__NR_membarrier) +#define MEMBARRIER_CMD_QUERY 0x00 +#define MEMBARRIER_CMD_PRIVATE_EXPEDITED 0x08 +#define MEMBARRIER_CMD_REGISTER_PRIVATE_EXPEDITED 0x10 + + +static int membarrier( int cmd, unsigned int flags, int cpu_id ) +{ + return syscall( __NR_membarrier, cmd, flags, cpu_id ); +} + + +static void membarrier_init( void ) +{ + static const int exp_required_cmds = + MEMBARRIER_CMD_PRIVATE_EXPEDITED | MEMBARRIER_CMD_REGISTER_PRIVATE_EXPEDITED; + int available_cmds = membarrier( MEMBARRIER_CMD_QUERY, 0, 0 ); + if (available_cmds == -1) + return; + if ((available_cmds & exp_required_cmds) == exp_required_cmds) + membarrier_exp_available = !membarrier( MEMBARRIER_CMD_REGISTER_PRIVATE_EXPEDITED, 0, 0 ); +} + + +static int try_exp_membarrier( void ) +{ + pthread_once(&membarrier_init_once, membarrier_init); + if (!membarrier_exp_available) + return 0; + return !membarrier( MEMBARRIER_CMD_PRIVATE_EXPEDITED, 0, 0 ); +} +#else +static int try_exp_membarrier( void ) { return 0; } +#endif + + #if defined(__linux__) && (defined(__i386__) || defined(__x86_64__)) static int try_madvise( void ) { @@ -5083,6 +5128,8 @@ static void do_apc_memorybarrier( void ) */ void WINAPI NtFlushProcessWriteBuffers(void) { + if (try_exp_membarrier()) + return; if (try_madvise()) return; do_apc_memorybarrier(); -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/741
From: Torge Matthies <tmatthies(a)codeweavers.com> --- dlls/ntdll/unix/virtual.c | 58 +++++++++++++++++++++++++++++++++++++++ 1 file changed, 58 insertions(+) diff --git a/dlls/ntdll/unix/virtual.c b/dlls/ntdll/unix/virtual.c index fa4216e7f9d..e920fea23a0 100644 --- a/dlls/ntdll/unix/virtual.c +++ b/dlls/ntdll/unix/virtual.c @@ -65,6 +65,9 @@ #if defined(__APPLE__) # include <mach/mach_init.h> # include <mach/mach_vm.h> +# include <mach/task.h> +# include <mach/thread_state.h> +# include <mach/vm_map.h> #endif #include "ntstatus.h" @@ -217,6 +220,11 @@ struct range_entry static struct range_entry *free_ranges; static struct range_entry *free_ranges_end; +#ifdef __APPLE__ +static kern_return_t (*pthread_get_register_pointer_values)( thread_t, uintptr_t*, size_t*, uintptr_t* ); +static pthread_once_t tgrpvs_init_once = PTHREAD_ONCE_INIT; +#endif + #ifdef __linux__ #ifdef __NR_membarrier static BOOL membarrier_exp_available; @@ -5029,6 +5037,54 @@ NTSTATUS WINAPI NtFlushInstructionCache( HANDLE handle, const void *addr, SIZE_T } +#ifdef __APPLE__ + +static void tgrpvs_init( void ) +{ + pthread_get_register_pointer_values = dlsym( RTLD_DEFAULT, "thread_get_register_pointer_values" ); +} + +static int try_mach_tgrpvs( void ) +{ + /* Taken from https://github.com/dotnet/runtime/blob/7be37908e5a1cbb83b1062768c1649827eeac... */ + mach_msg_type_number_t count, i; + thread_act_array_t threads; + kern_return_t kret; + int ret = 0; + + pthread_once(&tgrpvs_init_once, tgrpvs_init); + if (!pthread_get_register_pointer_values) + return 0; + + kret = task_threads( mach_task_self(), &threads, &count ); + if (kret) + return 0; + + for (i = 0; i < count; i++) + { + uintptr_t reg_values[128]; + size_t reg_count = ARRAY_SIZE( reg_values ); + uintptr_t sp; + + kret = pthread_get_register_pointer_values( threads[i], &sp, ®_count, reg_values ); + if (kret) + goto fail; + + mach_port_deallocate( mach_task_self(), threads[i] ); + } + ret = 1; +fail: + for (; i < count; i++) + mach_port_deallocate( mach_task_self(), threads[i] ); + vm_deallocate( mach_task_self(), (vm_address_t)threads, count * sizeof(threads[0]) ); + return ret; +} + +#else +static int try_mach_tgrpvs( void ) { return 0; } +#endif + + #if defined(__linux__) && defined(__NR_membarrier) #define MEMBARRIER_CMD_QUERY 0x00 #define MEMBARRIER_CMD_PRIVATE_EXPEDITED 0x08 @@ -5128,6 +5184,8 @@ static void do_apc_memorybarrier( void ) */ void WINAPI NtFlushProcessWriteBuffers(void) { + if (try_mach_tgrpvs()) + return; if (try_exp_membarrier()) return; if (try_madvise()) -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/741
On Fri Sep 2 20:20:25 2022 +0000, Tatsuyuki Ishi wrote:
Don���t use MEMBARRIER_CMD_SHARED under any circumstance: its performance hit is potentially catastrophic: https://lttng.org/blog/2018/01/15/membarrier-system-call-performance-and-use... Also, dotnet core has code paths for a few more platforms so you might want to pick some of that too. https://github.com/dotnet/runtime/blob/7be37908e5a1cbb83b1062768c1649827eeac... I have removed the `MEMBARRIER_CMD_SHARED` path and added the `thread_get_register_pointer_values` path from dotnet. The latter needs testing from someone else because I have no device with macOS.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/741#note_7579
On Thu Sep 1 15:38:33 2022 +0000, Jinoh Kang wrote:
Do you remember why you added a memory barrier in `server_select`? It was accidental. That change should be a distinct patch on its own. Afaict it's not needed No, it's not needed for this MR. The idea was that `server_select` should act as a strong memory barrier itself, so that the thread calling `FlushProcessWriteBuffers` did not need to call `MemoryBarrier()` explicitly This is based on my reading of https://docs.microsoft.com/en-us/windows/win32/sync/synchronization-and-mult...: it seems to imply that wait functions shall act as barriers themselves. It makes sense since the waiter would expect to read latest values from shared variables when it wakes up. but I copied it into this mr for now.
Also if you want me to add a Co-Authored-By for you please let me know. How about splitting the patch instead? The first commit would introduce the fallback path, the second membarrier(), and so on. We're not introducing regression by rolling out an inefficient `FlushProcessWriteBuffers` implementation, since it had never worked in the first place. done
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/741#note_7580
I need someone with a macOS device to test the `thread_get_register_pointer_values` code path because I have no such device. Here is a diff that adds a test to the ntdll sync test that just calls `NtFlushProcessWriteBuffers`: [NtFlushProcessWriteBuffers-test.diff](/uploads/b84a0504ebd3d5f7b56813072d40122f/NtFlushProcessWriteBuffers-test.diff) -- https://gitlab.winehq.org/wine/wine/-/merge_requests/741#note_7581
Are you sure that all Unices will issue a memory barrier on all running threads of the process though?
At least .NET seems to prefer that method on both Linux and macOS. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/741#note_7598
On Sat Sep 3 04:06:25 2022 +0000, Jinoh Kang wrote:
Are you sure that all Unices will issue a memory barrier on all running threads of the process though? At least .NET seems to prefer that method on both Linux and macOS. .NET core doesn't use that code path on macOS - `s_helperPage` is never initialized if `TARGET_OSX` is defined.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/741#note_7600
.NET core doesn't use that code path on macOS - `s_helperPage` is never initialized if `TARGET_OSX` is defined.
Well, ok, then. I assumed eager page table manipulation would trigger IPI on most operating systems, though. For example, Dave et al.<sup>[1]</sup> mentions mprotect as a cross-platform global memory barrier technique. But I think it's better to stick to whatever existing projects are doing anyway. [1] Dave Dice, Maurice Herlihy, and Alex Kogan. 2016. Fast non-intrusive memory reclamation for highly-concurrent data structures. SIGPLAN Not. 51, 11 (November 2016), 36���45. https://doi.org/10.1145/3241624.2926699 -- https://gitlab.winehq.org/wine/wine/-/merge_requests/741#note_7626
I don't think I can/should do that, since then if thread 2 issues a memory barrier while thread 1 is already waiting on a memory barrier, then thread 1 will also wait for the second memory barrier to complete instead of just its own.
We can serialize the barrier calls with mutex here, too. It will avoid excessive APCs in case multiple threads call NtFlushProcessWriteBuffers (e.g. RCU with concurrent writers, GC in multiple arena/isolates). If we don't serialize the barrier or coalesce APCs, the total number of simultaneous APCs will be `NM` where N = number of threads in the process, and M = concurrent calls to NtFlushProcessWriteBuffers. Since not all applications use membarrier in the first place, we can also avoid extra object allocation for threads that will never end up using the global memory barrier. (Yet another approach to solve this problem would be keeping track of generations. It will let us coalesce APCs, but this sounds like an overkill.) In general, we want to minimize the complexity and overhead of the fallback path since its use will not be very common: newest operating systems will just use mprotect/membarrier/mach calls, and the fallback is only used when all else fails. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/741#note_7628
On Sun Sep 4 13:48:53 2022 +0000, Torge Matthies wrote:
I need someone with a macOS device to test the `thread_get_register_pointer_values` code path because I have no such device. Here is a diff that adds a test to the ntdll sync test that just calls `NtFlushProcessWriteBuffers`: [NtFlushProcessWriteBuffers-test.diff](/uploads/b84a0504ebd3d5f7b56813072d40122f/NtFlushProcessWriteBuffers-test.diff) It would be best if we could introduce the litmus test into the test suite, but it would count as flaky since what we're testing is a data race by nature.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/741#note_7629
participants (3)
-
Jinoh Kang (＠iamahuman) -
Torge Matthies
-
Torge Matthies (＠tmatthies)