Fix correct accumulated time for parallelized jobs in SGEN. (mono/mono#17518)

* Fix correct accumulated time for parallelized jobs in SGEN.

Current implementation incorrectly accumulate time for scan jobs for
the follow metrics when running parallel minor GC:

time_minor_scan_major_blocks,
time_minor_scan_los,
time_major_scan_mod_union_blocks,
time_major_scan_mod_union_los

Commit fix so updates are atomic making sure values are correct in cases
where parallel minor GC is used.

* Implement 64-bit atommic add fallback in SGEN.

Commit migrated from https://github.com/mono/mono/commit/44e6226c31d8ffcae58f81350d71a728edecfe22

diff --git a/src/mono/mono/sgen/sgen-gc.c b/src/mono/mono/sgen/sgen-gc.c
index 9d41931..78069ee 100644 (file)
--- a/src/mono/mono/sgen/sgen-gc.c
+++ b/src/mono/mono/sgen/sgen-gc.c
@@ -434,6 +434,34 @@ SgenMinorCollector sgen_minor_collector;
 
 static SgenRememberedSet remset;
 
+#ifdef MONO_ATOMIC_USES_LOCK
+#include <pthread.h>
+static pthread_mutex_t sgen_atomic_spin_lock G_GNUC_UNUSED = PTHREAD_MUTEX_INITIALIZER;
+
+static gint64
+mono_sgen_atomic_cas_i64(volatile gint64 *dest, gint64 exch, gint64 comp)
+{
+       gint64 old;
+       int ret;
+
+       pthread_cleanup_push ((void(*)(void *))pthread_mutex_unlock, (void *)&sgen_atomic_spin_lock);
+       ret = pthread_mutex_lock(&sgen_atomic_spin_lock);
+       g_assert (ret == 0);
+
+       old= *dest;
+       if(old==comp) {
+               *dest=exch;
+       }
+
+       ret = pthread_mutex_unlock(&sgen_atomic_spin_lock);
+       g_assert (ret == 0);
+
+       pthread_cleanup_pop (0);
+
+       return(old);
+}
+#endif
+
 /*
  * The gray queue a worker job must use.  If we're not parallel or
  * concurrent, we use the main gray queue.
@@ -1446,10 +1474,12 @@ job_scan_major_card_table (void *worker_data_untyped, SgenThreadPoolJob *job)
        SGEN_TV_GETTIME (atv);
        sgen_major_collector.scan_card_table (CARDTABLE_SCAN_GLOBAL, ctx, job_data->job_index, job_data->job_split_count, job_data->data);
        SGEN_TV_GETTIME (btv);
-       time_minor_scan_major_blocks += SGEN_TV_ELAPSED (atv, btv);
+
+       gint64 elapsed_time = SGEN_TV_ELAPSED (atv, btv);
+       SGEN_ATOMIC_ADD_I64 (time_minor_scan_major_blocks, elapsed_time);
 
        if (worker_data_untyped)
-               ((WorkerData*)worker_data_untyped)->major_scan_time += SGEN_TV_ELAPSED (atv, btv);
+               ((WorkerData*)worker_data_untyped)->major_scan_time += elapsed_time;
 }
 
 static void
@@ -1463,10 +1493,12 @@ job_scan_los_card_table (void *worker_data_untyped, SgenThreadPoolJob *job)
        SGEN_TV_GETTIME (atv);
        sgen_los_scan_card_table (CARDTABLE_SCAN_GLOBAL, ctx, job_data->job_index, job_data->job_split_count);
        SGEN_TV_GETTIME (btv);
-       time_minor_scan_los += SGEN_TV_ELAPSED (atv, btv);
+
+       gint64 elapsed_time = SGEN_TV_ELAPSED (atv, btv);
+       SGEN_ATOMIC_ADD_I64 (time_minor_scan_los, elapsed_time);
 
        if (worker_data_untyped)
-               ((WorkerData*)worker_data_untyped)->los_scan_time += SGEN_TV_ELAPSED (atv, btv);
+               ((WorkerData*)worker_data_untyped)->los_scan_time += elapsed_time;
 }
 
 static void
@@ -1481,10 +1513,12 @@ job_scan_major_mod_union_card_table (void *worker_data_untyped, SgenThreadPoolJo
        SGEN_TV_GETTIME (atv);
        sgen_major_collector.scan_card_table (CARDTABLE_SCAN_MOD_UNION, ctx, job_data->job_index, job_data->job_split_count, job_data->data);
        SGEN_TV_GETTIME (btv);
-       time_major_scan_mod_union_blocks += SGEN_TV_ELAPSED (atv, btv);
+
+       gint64 elapsed_time = SGEN_TV_ELAPSED (atv, btv);
+       SGEN_ATOMIC_ADD_I64 (time_minor_scan_los, time_major_scan_mod_union_blocks);
 
        if (worker_data_untyped)
-               ((WorkerData*)worker_data_untyped)->major_scan_time += SGEN_TV_ELAPSED (atv, btv);
+               ((WorkerData*)worker_data_untyped)->major_scan_time += elapsed_time;
 }
 
 static void
@@ -1499,10 +1533,12 @@ job_scan_los_mod_union_card_table (void *worker_data_untyped, SgenThreadPoolJob
        SGEN_TV_GETTIME (atv);
        sgen_los_scan_card_table (CARDTABLE_SCAN_MOD_UNION, ctx, job_data->job_index, job_data->job_split_count);
        SGEN_TV_GETTIME (btv);
-       time_major_scan_mod_union_los += SGEN_TV_ELAPSED (atv, btv);
+
+       gint64 elapsed_time = SGEN_TV_ELAPSED (atv, btv);
+       SGEN_ATOMIC_ADD_I64 (time_minor_scan_los, time_major_scan_mod_union_los);
 
        if (worker_data_untyped)
-               ((WorkerData*)worker_data_untyped)->los_scan_time += SGEN_TV_ELAPSED (atv, btv);
+               ((WorkerData*)worker_data_untyped)->los_scan_time += elapsed_time;
 }
 
 static void

diff --git a/src/mono/mono/sgen/sgen-gc.h b/src/mono/mono/sgen/sgen-gc.h
index 34ee230..5c44bac 100644 (file)
--- a/src/mono/mono/sgen/sgen-gc.h
+++ b/src/mono/mono/sgen/sgen-gc.h
@@ -104,6 +104,19 @@ extern MonoCoopMutex sgen_interruption_mutex;
                } while (mono_atomic_cas_ptr ((void**)&(x), (void*)(__old_x + (i)), (void*)__old_x) != (void*)__old_x); \
        } while (0)
 
+#ifndef MONO_ATOMIC_USES_LOCK
+#define SGEN_ATOMIC_ADD_I64(x,i) do { \
+               mono_atomic_add_i64 ((volatile gint64 *)&x, i); \
+       } while (0)
+#else
+#define SGEN_ATOMIC_ADD_I64(x,i) do { \
+               gint64 __old_x; \
+               do { \
+                       __old_x = (x); \
+               } while (mono_sgen_atomic_cas_i64 ((volatile gint64 *)&(x), __old_x + (i), __old_x) != __old_x); \
+       } while (0)
+#endif /* BROKEN_64BIT_ATOMICS_INTRINSIC */
+
 #ifdef HEAVY_STATISTICS
 extern guint64 stat_objects_alloced_degraded;
 extern guint64 stat_bytes_alloced_degraded;

diff --git a/src/mono/mono/utils/atomic.c b/src/mono/mono/utils/atomic.c
index bd473f6..58c46e2 100644 (file)
--- a/src/mono/mono/utils/atomic.c
+++ b/src/mono/mono/utils/atomic.c
@@ -16,17 +16,20 @@
 #include <mono/utils/mono-compiler.h>
 
 #if defined (WAPI_NO_ATOMIC_ASM) || defined (BROKEN_64BIT_ATOMICS_INTRINSIC)
+#define NEED_64BIT_CMPXCHG_FALLBACK
+#endif
 
+#ifdef MONO_ATOMIC_USES_LOCK
 #include <pthread.h>
-
 static pthread_mutex_t spin G_GNUC_UNUSED = PTHREAD_MUTEX_INITIALIZER;
-
-#define NEED_64BIT_CMPXCHG_FALLBACK
-
 #endif
 
 #ifdef WAPI_NO_ATOMIC_ASM
 
+#ifndef MONO_ATOMIC_USES_LOCK
+#error MONO_ATOMIC_USES_LOCK NOT defined
+#endif
+
 gint32 mono_atomic_cas_i32(volatile gint32 *dest, gint32 exch,
                                  gint32 comp)
 {
@@ -497,6 +500,10 @@ void mono_atomic_store_ptr(volatile gpointer *dst, gpointer val)
 
 #if defined (TARGET_OSX)
 
+#ifdef MONO_ATOMIC_USES_LOCK
+#error MONO_ATOMIC_USES_LOCK defined
+#endif
+
 /* The compiler breaks if this code is in the header... */
 
 gint64
@@ -507,6 +514,10 @@ mono_atomic_cas_i64(volatile gint64 *dest, gint64 exch, gint64 comp)
 
 #elif defined (__arm__) && defined (HAVE_ARMV7) && (defined(TARGET_IOS) || defined(TARGET_WATCHOS) || defined(TARGET_ANDROID))
 
+#ifdef MONO_ATOMIC_USES_LOCK
+#error MONO_ATOMIC_USES_LOCK defined
+#endif
+
 #if defined (TARGET_IOS) || defined (TARGET_WATCHOS)
 
 #ifndef __clang__
@@ -559,6 +570,10 @@ mono_atomic_cas_i64(volatile gint64 *dest, gint64 exch, gint64 comp)
 
 #else
 
+#ifndef MONO_ATOMIC_USES_LOCK
+#error MONO_ATOMIC_USES_LOCK NOT defined
+#endif
+
 gint64
 mono_atomic_cas_i64(volatile gint64 *dest, gint64 exch, gint64 comp)
 {

diff --git a/src/mono/mono/utils/atomic.h b/src/mono/mono/utils/atomic.h
index c26f65b..40574c1 100755 (executable)
--- a/src/mono/mono/utils/atomic.h
+++ b/src/mono/mono/utils/atomic.h
@@ -498,4 +498,12 @@ mono_atomic_store_bool (volatile gboolean *dest, gboolean val)
        mono_atomic_store_i32 ((volatile gint32 *)dest, (gint32)val);
 }
 
+#if defined (WAPI_NO_ATOMIC_ASM)
+#define MONO_ATOMIC_USES_LOCK
+#elif defined(BROKEN_64BIT_ATOMICS_INTRINSIC)
+#if !defined(TARGET_OSX) && !(defined (__arm__) && defined (HAVE_ARMV7) && (defined(TARGET_IOS) || defined(TARGET_WATCHOS) || defined(TARGET_ANDROID)))
+#define MONO_ATOMIC_USES_LOCK
+#endif
+#endif
+
 #endif /* _WAPI_ATOMIC_H_ */