From 5ed0703c377c06172f5065ed2d8da7ebb4e03018 Mon Sep 17 00:00:00 2001 From: Vlad Brezae Date: Tue, 13 Jun 2023 10:10:02 +0300 Subject: [PATCH 1/3] [mono][utils] Move all membar code to single file --- src/mono/mono/metadata/domain.c | 1 - src/mono/mono/metadata/icall.c | 1 - src/mono/mono/metadata/jit-info.c | 2 +- src/mono/mono/metadata/loader.c | 2 +- src/mono/mono/metadata/property-bag.c | 2 +- src/mono/mono/metadata/reflection.c | 2 +- src/mono/mono/metadata/string-icalls.c | 2 +- src/mono/mono/metadata/threads-types.h | 2 +- src/mono/mono/metadata/threads.c | 2 +- src/mono/mono/mini/interp/interp.c | 2 +- src/mono/mono/mini/mini-trampolines.c | 2 +- src/mono/mono/profiler/log.c | 2 +- src/mono/mono/sgen/sgen-fin-weak-hash.c | 2 +- src/mono/mono/sgen/sgen-gchandles.c | 2 +- src/mono/mono/sgen/sgen-nursery-allocator.c | 2 +- src/mono/mono/sgen/sgen-protocol.c | 2 +- src/mono/mono/sgen/sgen-workers.c | 2 +- src/mono/mono/utils/CMakeLists.txt | 1 - src/mono/mono/utils/atomic.h | 2 +- src/mono/mono/utils/hazard-pointer.c | 1 - src/mono/mono/utils/hazard-pointer.h | 2 +- src/mono/mono/utils/lock-free-alloc.c | 2 +- src/mono/mono/utils/lock-free-array-queue.c | 2 +- src/mono/mono/utils/lock-free-queue.c | 2 +- src/mono/mono/utils/mono-linked-list-set.h | 2 +- src/mono/mono/utils/mono-membar.h | 83 --------------------- src/mono/mono/utils/mono-memory-model.h | 66 +++++++++++++++- 27 files changed, 86 insertions(+), 109 deletions(-) delete mode 100644 src/mono/mono/utils/mono-membar.h diff --git a/src/mono/mono/metadata/domain.c b/src/mono/mono/metadata/domain.c index 8519cf409c54b..10666a5f2b229 100644 --- a/src/mono/mono/metadata/domain.c +++ b/src/mono/mono/metadata/domain.c @@ -22,7 +22,6 @@ #include #include #include -#include #include #include #include diff --git a/src/mono/mono/metadata/icall.c b/src/mono/mono/metadata/icall.c index a4ec470a2ddb6..11af2c344bc63 100644 --- a/src/mono/mono/metadata/icall.c +++ b/src/mono/mono/metadata/icall.c @@ -38,7 +38,6 @@ #endif #include "mono/metadata/icall-internals.h" -#include "mono/utils/mono-membar.h" #include #include #include diff --git a/src/mono/mono/metadata/jit-info.c b/src/mono/mono/metadata/jit-info.c index 5446a3248acff..9cc5e817deebd 100644 --- a/src/mono/mono/metadata/jit-info.c +++ b/src/mono/mono/metadata/jit-info.c @@ -23,7 +23,7 @@ #include #include #include -#include +#include #include #include #include diff --git a/src/mono/mono/metadata/loader.c b/src/mono/mono/metadata/loader.c index afb16c6351d07..81209134e6444 100644 --- a/src/mono/mono/metadata/loader.c +++ b/src/mono/mono/metadata/loader.c @@ -46,7 +46,7 @@ #include #include #include -#include +#include #include #include #include diff --git a/src/mono/mono/metadata/property-bag.c b/src/mono/mono/metadata/property-bag.c index 42c083f038115..b1b60da8d1971 100644 --- a/src/mono/mono/metadata/property-bag.c +++ b/src/mono/mono/metadata/property-bag.c @@ -9,7 +9,7 @@ */ #include #include -#include +#include /* * mono_property_bag_get: diff --git a/src/mono/mono/metadata/reflection.c b/src/mono/mono/metadata/reflection.c index 37898f954ff34..431e764393298 100644 --- a/src/mono/mono/metadata/reflection.c +++ b/src/mono/mono/metadata/reflection.c @@ -13,7 +13,7 @@ * Licensed under the MIT license. See LICENSE file in the project root for full license information. */ #include -#include "mono/utils/mono-membar.h" +#include "mono/utils/mono-memory-model.h" #include "mono/metadata/assembly-internals.h" #include "mono/metadata/reflection-internals.h" #include "mono/metadata/tabledefs.h" diff --git a/src/mono/mono/metadata/string-icalls.c b/src/mono/mono/metadata/string-icalls.c index a8e3cb14dd14d..c73f197a120bc 100644 --- a/src/mono/mono/metadata/string-icalls.c +++ b/src/mono/mono/metadata/string-icalls.c @@ -14,7 +14,7 @@ #include #include #include -#include "mono/utils/mono-membar.h" +#include "mono/utils/mono-memory-model.h" #include #include #include diff --git a/src/mono/mono/metadata/threads-types.h b/src/mono/mono/metadata/threads-types.h index c1436094559c8..c92e61497dba6 100644 --- a/src/mono/mono/metadata/threads-types.h +++ b/src/mono/mono/metadata/threads-types.h @@ -19,7 +19,7 @@ #include #include "mono/metadata/handle.h" #include "mono/utils/mono-compiler.h" -#include "mono/utils/mono-membar.h" +#include "mono/utils/mono-memory-model.h" #include "mono/utils/mono-threads.h" #include "mono/metadata/class-internals.h" #include diff --git a/src/mono/mono/metadata/threads.c b/src/mono/mono/metadata/threads.c index 7cde95a9dc0b1..6f97688797209 100644 --- a/src/mono/mono/metadata/threads.c +++ b/src/mono/mono/metadata/threads.c @@ -36,7 +36,7 @@ #include #include #include -#include +#include #include #include #include diff --git a/src/mono/mono/mini/interp/interp.c b/src/mono/mono/mini/interp/interp.c index a6ee9e1f3eeaf..ab864bbd28a9c 100644 --- a/src/mono/mono/mini/interp/interp.c +++ b/src/mono/mono/mini/interp/interp.c @@ -27,7 +27,7 @@ #include #include #include -#include +#include #ifdef HAVE_ALLOCA_H # include diff --git a/src/mono/mono/mini/mini-trampolines.c b/src/mono/mono/mini/mini-trampolines.c index 663120fd54030..5eb7be5c46c08 100644 --- a/src/mono/mono/mini/mini-trampolines.c +++ b/src/mono/mono/mini/mini-trampolines.c @@ -14,7 +14,7 @@ #include #include #include -#include +#include #include #include #include diff --git a/src/mono/mono/profiler/log.c b/src/mono/mono/profiler/log.c index 110253833a97b..1aa33e9c40ee8 100644 --- a/src/mono/mono/profiler/log.c +++ b/src/mono/mono/profiler/log.c @@ -38,7 +38,7 @@ #include #include #include -#include +#include #include #include #include diff --git a/src/mono/mono/sgen/sgen-fin-weak-hash.c b/src/mono/mono/sgen/sgen-fin-weak-hash.c index 7b5f3c908aeed..615982c3d6611 100644 --- a/src/mono/mono/sgen/sgen-fin-weak-hash.c +++ b/src/mono/mono/sgen/sgen-fin-weak-hash.c @@ -23,7 +23,7 @@ #include "mono/sgen/sgen-pointer-queue.h" #include "mono/sgen/sgen-client.h" #include "mono/sgen/gc-internal-agnostic.h" -#include "mono/utils/mono-membar.h" +#include "mono/utils/mono-memory-model.h" #include "mono/utils/atomic.h" #include "mono/utils/unlocked.h" diff --git a/src/mono/mono/sgen/sgen-gchandles.c b/src/mono/mono/sgen/sgen-gchandles.c index 7d4f3344f030f..fa9742b93c0cc 100644 --- a/src/mono/mono/sgen/sgen-gchandles.c +++ b/src/mono/mono/sgen/sgen-gchandles.c @@ -13,7 +13,7 @@ #include "mono/sgen/sgen-gc.h" #include "mono/sgen/sgen-client.h" #include "mono/sgen/sgen-array-list.h" -#include "mono/utils/mono-membar.h" +#include "mono/utils/mono-memory-model.h" #ifdef HEAVY_STATISTICS static volatile guint32 stat_gc_handles_allocated = 0; diff --git a/src/mono/mono/sgen/sgen-nursery-allocator.c b/src/mono/mono/sgen/sgen-nursery-allocator.c index 731dfcccbd1d4..9f9548474cf47 100644 --- a/src/mono/mono/sgen/sgen-nursery-allocator.c +++ b/src/mono/mono/sgen/sgen-nursery-allocator.c @@ -55,7 +55,7 @@ #include "mono/sgen/sgen-memory-governor.h" #include "mono/sgen/sgen-pinning.h" #include "mono/sgen/sgen-client.h" -#include "mono/utils/mono-membar.h" +#include "mono/utils/mono-memory-model.h" /* Enable it so nursery allocation diagnostic data is collected */ //#define NALLOC_DEBUG 1 diff --git a/src/mono/mono/sgen/sgen-protocol.c b/src/mono/mono/sgen/sgen-protocol.c index 1e76136dc8281..7c7543cd4292c 100644 --- a/src/mono/mono/sgen/sgen-protocol.c +++ b/src/mono/mono/sgen/sgen-protocol.c @@ -18,7 +18,7 @@ #include "sgen-memory-governor.h" #include "sgen-workers.h" #include "sgen-client.h" -#include "mono/utils/mono-membar.h" +#include "mono/utils/mono-memory-model.h" #include "mono/utils/mono-proclib.h" #include diff --git a/src/mono/mono/sgen/sgen-workers.c b/src/mono/mono/sgen/sgen-workers.c index f68911581e0d2..adb4b8f3e8a48 100644 --- a/src/mono/mono/sgen/sgen-workers.c +++ b/src/mono/mono/sgen/sgen-workers.c @@ -18,7 +18,7 @@ #include "mono/sgen/sgen-gc.h" #include "mono/sgen/sgen-workers.h" #include "mono/sgen/sgen-thread-pool.h" -#include "mono/utils/mono-membar.h" +#include "mono/utils/mono-memory-model.h" #include "mono/sgen/sgen-client.h" #ifndef DISABLE_SGEN_MAJOR_MARKSWEEP_CONC diff --git a/src/mono/mono/utils/CMakeLists.txt b/src/mono/mono/utils/CMakeLists.txt index efbfa3c119cff..fdcece6573e35 100644 --- a/src/mono/mono/utils/CMakeLists.txt +++ b/src/mono/mono/utils/CMakeLists.txt @@ -101,7 +101,6 @@ set(utils_common_sources mono-forward-internal.h mono-machine.h mono-math.h - mono-membar.h mono-path.h mono-poll.h mono-uri.h diff --git a/src/mono/mono/utils/atomic.h b/src/mono/mono/utils/atomic.h index 1bf7fea44b74c..0a69b80bb4f51 100644 --- a/src/mono/mono/utils/atomic.h +++ b/src/mono/mono/utils/atomic.h @@ -15,7 +15,7 @@ #include "config.h" #include -#include +#include #include /* diff --git a/src/mono/mono/utils/hazard-pointer.c b/src/mono/mono/utils/hazard-pointer.c index 9769c4b38ad02..bf01d58627c8d 100644 --- a/src/mono/mono/utils/hazard-pointer.c +++ b/src/mono/mono/utils/hazard-pointer.c @@ -11,7 +11,6 @@ #include #include -#include #include #include #include diff --git a/src/mono/mono/utils/hazard-pointer.h b/src/mono/mono/utils/hazard-pointer.h index a1f453e88cae3..1093b09b475dd 100644 --- a/src/mono/mono/utils/hazard-pointer.h +++ b/src/mono/mono/utils/hazard-pointer.h @@ -10,7 +10,7 @@ #include #include -#include +#include #include #define HAZARD_POINTER_COUNT 3 diff --git a/src/mono/mono/utils/lock-free-alloc.c b/src/mono/mono/utils/lock-free-alloc.c index e90731ca86fa2..7826c27017848 100644 --- a/src/mono/mono/utils/lock-free-alloc.c +++ b/src/mono/mono/utils/lock-free-alloc.c @@ -73,7 +73,7 @@ #else #include #endif -#include +#include #include #include diff --git a/src/mono/mono/utils/lock-free-array-queue.c b/src/mono/mono/utils/lock-free-array-queue.c index 82ef5649cd4e6..3672e3306cc76 100644 --- a/src/mono/mono/utils/lock-free-array-queue.c +++ b/src/mono/mono/utils/lock-free-array-queue.c @@ -21,7 +21,7 @@ #include #include -#include +#include #ifdef SGEN_WITHOUT_MONO #include #include diff --git a/src/mono/mono/utils/lock-free-queue.c b/src/mono/mono/utils/lock-free-queue.c index 02ce5b1745a77..a8152efee5e9d 100644 --- a/src/mono/mono/utils/lock-free-queue.c +++ b/src/mono/mono/utils/lock-free-queue.c @@ -52,7 +52,7 @@ #include #endif -#include +#include #include #include diff --git a/src/mono/mono/utils/mono-linked-list-set.h b/src/mono/mono/utils/mono-linked-list-set.h index 04e1537f314c2..50024f486bb4b 100644 --- a/src/mono/mono/utils/mono-linked-list-set.h +++ b/src/mono/mono/utils/mono-linked-list-set.h @@ -12,7 +12,7 @@ #define __MONO_SPLIT_ORDERED_LIST_H__ #include -#include +#include typedef struct _MonoLinkedListSetNode MonoLinkedListSetNode; diff --git a/src/mono/mono/utils/mono-membar.h b/src/mono/mono/utils/mono-membar.h deleted file mode 100644 index 1092ca634dd6e..0000000000000 --- a/src/mono/mono/utils/mono-membar.h +++ /dev/null @@ -1,83 +0,0 @@ -/** - * \file - * Memory barrier inline functions - * - * Author: - * Mark Probst (mark.probst@gmail.com) - * - * (C) 2007 Novell, Inc - */ - -#ifndef _MONO_UTILS_MONO_MEMBAR_H_ -#define _MONO_UTILS_MONO_MEMBAR_H_ - -#include - -#include - -/* - * Memory barrier which only affects the compiler. - * mono_memory_barrier_process_wide () should be uses to synchronize with code which uses this. - */ -//#define mono_compiler_barrier() asm volatile("": : :"memory") - -#if _MSC_VER -#ifndef WIN32_LEAN_AND_MEAN -#define WIN32_LEAN_AND_MEAN -#endif -#include -#include - -static inline void mono_memory_barrier (void) -{ - /* NOTE: _ReadWriteBarrier and friends only prevent the - compiler from reordering loads and stores. To prevent - the CPU from doing the same, we have to use the - MemoryBarrier macro which expands to e.g. a serializing - XCHG instruction on x86. Also note that the MemoryBarrier - macro does *not* imply _ReadWriteBarrier, so that call - cannot be eliminated. */ - _ReadWriteBarrier (); - MemoryBarrier (); -} - -static inline void mono_memory_read_barrier (void) -{ - _ReadBarrier (); - MemoryBarrier (); -} - -static inline void mono_memory_write_barrier (void) -{ - _WriteBarrier (); - MemoryBarrier (); -} - -#define mono_compiler_barrier() _ReadWriteBarrier () - -#elif defined(USE_GCC_ATOMIC_OPS) || defined(HOST_WASM) - -static inline void mono_memory_barrier (void) -{ - __sync_synchronize (); -} - -static inline void mono_memory_read_barrier (void) -{ - mono_memory_barrier (); -} - -static inline void mono_memory_write_barrier (void) -{ - mono_memory_barrier (); -} - -#define mono_compiler_barrier() asm volatile("": : :"memory") - -#else -#error "Don't know how to do memory barriers!" -#endif - -void mono_memory_barrier_process_wide (void); - -#endif /* _MONO_UTILS_MONO_MEMBAR_H_ */ diff --git a/src/mono/mono/utils/mono-memory-model.h b/src/mono/mono/utils/mono-memory-model.h index 863ee77d61563..addaf028b5cec 100644 --- a/src/mono/mono/utils/mono-memory-model.h +++ b/src/mono/mono/utils/mono-memory-model.h @@ -12,7 +12,71 @@ #define _MONO_UTILS_MONO_MEMMODEL_H_ #include -#include + +#if _MSC_VER +#ifndef WIN32_LEAN_AND_MEAN +#define WIN32_LEAN_AND_MEAN +#endif +#include +#include + +static inline void +mono_memory_barrier (void) +{ + /* NOTE: _ReadWriteBarrier and friends only prevent the + compiler from reordering loads and stores. To prevent + the CPU from doing the same, we have to use the + MemoryBarrier macro which expands to e.g. a serializing + XCHG instruction on x86. Also note that the MemoryBarrier + macro does *not* imply _ReadWriteBarrier, so that call + cannot be eliminated. */ + _ReadWriteBarrier (); + MemoryBarrier (); +} + +static inline void +mono_memory_read_barrier (void) +{ + _ReadBarrier (); + MemoryBarrier (); +} + +static inline void +mono_memory_write_barrier (void) +{ + _WriteBarrier (); + MemoryBarrier (); +} + +#define mono_compiler_barrier() _ReadWriteBarrier () + +#elif defined(USE_GCC_ATOMIC_OPS) || defined(HOST_WASM) + +static inline void +mono_memory_barrier (void) +{ + __sync_synchronize (); +} + +static inline void +mono_memory_read_barrier (void) +{ + mono_memory_barrier (); +} + +static inline void +mono_memory_write_barrier (void) +{ + mono_memory_barrier (); +} + +#define mono_compiler_barrier() asm volatile("": : :"memory") + +#else +#error "Don't know how to do memory barriers!" +#endif + +void mono_memory_barrier_process_wide (void); /* In order to allow for fast concurrent code, we must use fencing to properly order From 6ad0764f251659bf53f659fca8d4c1af74ca4d3a Mon Sep 17 00:00:00 2001 From: Vlad Brezae Date: Tue, 13 Jun 2023 10:27:20 +0300 Subject: [PATCH 2/3] [mono][utils] Redefine write and read memory barriers Before this change they were doing a full memory barrier, regardless of architecture. We have a beginning of more precise implementation via the *_FENCE defines. Implement mono_memory_write_barrier and mono_memory_read_barrier reusing these defines instead. The only consequence of this change is that, on x86 and amd64, `mono_memory_write_barrier` and `mono_memory_read_barrier` become a compiler barrier instead of a full mfence. --- src/mono/mono/utils/mono-memory-model.h | 49 +++++++++---------------- 1 file changed, 17 insertions(+), 32 deletions(-) diff --git a/src/mono/mono/utils/mono-memory-model.h b/src/mono/mono/utils/mono-memory-model.h index addaf028b5cec..aef59eab69ca9 100644 --- a/src/mono/mono/utils/mono-memory-model.h +++ b/src/mono/mono/utils/mono-memory-model.h @@ -34,20 +34,6 @@ mono_memory_barrier (void) MemoryBarrier (); } -static inline void -mono_memory_read_barrier (void) -{ - _ReadBarrier (); - MemoryBarrier (); -} - -static inline void -mono_memory_write_barrier (void) -{ - _WriteBarrier (); - MemoryBarrier (); -} - #define mono_compiler_barrier() _ReadWriteBarrier () #elif defined(USE_GCC_ATOMIC_OPS) || defined(HOST_WASM) @@ -58,18 +44,6 @@ mono_memory_barrier (void) __sync_synchronize (); } -static inline void -mono_memory_read_barrier (void) -{ - mono_memory_barrier (); -} - -static inline void -mono_memory_write_barrier (void) -{ - mono_memory_barrier (); -} - #define mono_compiler_barrier() asm volatile("": : :"memory") #else @@ -110,8 +84,6 @@ enum { }; #define MEMORY_BARRIER mono_memory_barrier () -#define LOAD_BARRIER mono_memory_read_barrier () -#define STORE_BARRIER mono_memory_write_barrier () #if defined(__i386__) || defined(_M_IX86) || defined(__x86_64__) || defined(_M_X64) /* @@ -141,8 +113,8 @@ AND R0, R0, #0 LDR R3, [R4, R0] */ -#define STORE_STORE_FENCE STORE_BARRIER -#define LOAD_LOAD_FENCE LOAD_BARRIER +#define STORE_STORE_FENCE MEMORY_BARRIER +#define LOAD_LOAD_FENCE MEMORY_BARRIER #define STORE_LOAD_FENCE MEMORY_BARRIER #define STORE_ACQUIRE_FENCE MEMORY_BARRIER #define STORE_RELEASE_FENCE MEMORY_BARRIER @@ -160,8 +132,8 @@ LDR R3, [R4, R0] #else /*default implementation with the weakest possible memory model */ -#define STORE_STORE_FENCE STORE_BARRIER -#define LOAD_LOAD_FENCE LOAD_BARRIER +#define STORE_STORE_FENCE MEMORY_BARRIER +#define LOAD_LOAD_FENCE MEMORY_BARRIER #define STORE_LOAD_FENCE MEMORY_BARRIER #define LOAD_STORE_FENCE MEMORY_BARRIER #define STORE_ACQUIRE_FENCE MEMORY_BARRIER @@ -236,4 +208,17 @@ Acquire/release semantics macros. STORE_ACQUIRE_FENCE; \ } + +static inline void +mono_memory_read_barrier (void) +{ + LOAD_LOAD_FENCE; +} + +static inline void +mono_memory_write_barrier (void) +{ + STORE_STORE_FENCE; +} + #endif /* _MONO_UTILS_MONO_MEMMODEL_H_ */ From 160301b2b28539d71ffc4e54446b1408b9cd0ddc Mon Sep 17 00:00:00 2001 From: Vlad Brezae Date: Tue, 13 Jun 2023 10:32:41 +0300 Subject: [PATCH 3/3] [mono][interp] Fix concurrency issues with publishing transfomed imethod When publishing a transformed InterpMethod*, we first set all relevant fields (like `code`, `alloca_size` etc), we execute a write barrier and finally we set the `transformed` flag. On relaxed memory arches we need to have a read barrier on the consumer, since there is no data dependency between `transformed` and the other fields of `InterpMethod`. On arm this change does a full barrier (we could get away with just a load acquire but we haven't yet added support for emitting this in the runtime). Still, this doesn't seem to introduce a heavy perf penalty (on my arm64 M1) but we can revisit if necessary. On x86/amd64 this is a compiler barrier so it should have no impact. WASM is single threaded for now. --- src/mono/mono/mini/interp/interp.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/mono/mono/mini/interp/interp.c b/src/mono/mono/mini/interp/interp.c index ab864bbd28a9c..5e5dcd05844fe 100644 --- a/src/mono/mono/mini/interp/interp.c +++ b/src/mono/mono/mini/interp/interp.c @@ -3606,6 +3606,8 @@ method_entry (ThreadContext *context, InterpFrame *frame, frame->stack = (stackval*)context->stack_pointer; return slow; } + } else { + mono_memory_read_barrier (); } return slow;