Merge pull request #2810 from kumpera/fix_hazard_free
authormonojenkins <jo.shields+jenkins@xamarin.com>
Thu, 31 Mar 2016 19:00:19 +0000 (20:00 +0100)
committermonojenkins <jo.shields+jenkins@xamarin.com>
Thu, 31 Mar 2016 19:00:19 +0000 (20:00 +0100)
Fix hazard free

Replace usage of mono_thread_hazardous_free_or_queue and remove it.

    mono_thread_hazardous_free_or_queue has a fundamentally broken design.

    It allows arbitrary free code to run in the context of its caller.

    The sync/async split on where it's called is not enough to know whether
    we can run free'ing code or not.

    Since in sync context we called free functions that could take locks,
    it happened that those locks conflicted with the ones already taken.

    In particular, mono_jit_info_table_free did take a domain lock and
    quite a few of the sync context calls would hold locks that must
    not be held when taking a domain lock.

    This, is practice, means that we'd need to partition the free calls
    into 3 groups: async context, reentrant context (no runtime locks held)
    and sync context (maybe some locks held).

    There was no case where reentrant context would be usable meaning,
    in practice, that all calls happens in what effectively is async context
    where free functions can't be called.

    The new design is a lot more straightforward:

    - No implicit free queue pumping
    - A pair of free functions with well defined behavior.
     mono_thread_hazardous_try_free - to be used where the caller expects the free function to be called
     mono_thread_hazardous_queue_free - to be used where the caller don't expect the free function to be called [1]
    - Explicit pumping on places that known to be ok
     Thread detach
     Finalizer thread

    [1] This might sound like a weird condition, but a lot of lock-free code
    have compensation logic that trigger free'ing during lookups.

1  2 
mono/metadata/gc.c
mono/metadata/jit-info.c
mono/sgen/sgen-gc.c
mono/utils/hazard-pointer.c
mono/utils/hazard-pointer.h
mono/utils/lock-free-alloc.c
mono/utils/mono-threads.c

diff --combined mono/metadata/gc.c
index 172660d1b55e93efbf291e6612ab9877883782ff,36f896ce929b4bb80b9a79c740a63c08438481a7..148bbedee6444a74666934c4a37d4b5377e69b2e
@@@ -6,7 -6,6 +6,7 @@@
   * Copyright 2002-2003 Ximian, Inc (http://www.ximian.com)
   * Copyright 2004-2009 Novell, Inc (http://www.novell.com)
   * Copyright 2012 Xamarin Inc (http://www.xamarin.com)
 + * Licensed under the MIT license. See LICENSE file in the project root for full license information.
   */
  
  #include <config.h>
@@@ -40,6 -39,7 +40,7 @@@
  #include <mono/utils/mono-threads.h>
  #include <mono/utils/atomic.h>
  #include <mono/utils/mono-coop-semaphore.h>
+ #include <mono/utils/hazard-pointer.h>
  
  #ifndef HOST_WIN32
  #include <pthread.h>
@@@ -639,6 -639,41 +640,41 @@@ mono_gc_finalize_notify (void
        mono_coop_sem_post (&finalizer_sem);
  }
  
+ /*
+ This is the number of entries allowed in the hazard free queue before
+ we explicitly cycle the finalizer thread to trigger pumping the queue.
+ It was picked empirically by running the corlib test suite in a stress
+ scenario where all hazard entries are queued.
+ In this extreme scenario we double the number of times we cycle the finalizer
+ thread compared to just GC calls.
+ Entries are usually in the order of 100's of bytes each, so we're limiting
+ floating garbage to be in the order of a dozen kb.
+ */
+ static gboolean finalizer_thread_pulsed;
+ #define HAZARD_QUEUE_OVERFLOW_SIZE 20
+ static void
+ hazard_free_queue_is_too_big (size_t size)
+ {
+       if (size < HAZARD_QUEUE_OVERFLOW_SIZE)
+               return;
+       if (finalizer_thread_pulsed || InterlockedCompareExchange (&finalizer_thread_pulsed, TRUE, FALSE))
+               return;
+       mono_gc_finalize_notify ();
+ }
+ static void
+ hazard_free_queue_pump (void)
+ {
+       mono_thread_hazardous_try_free_all ();
+       finalizer_thread_pulsed = FALSE;
+ }
  #ifdef HAVE_BOEHM_GC
  
  static void
@@@ -714,6 -749,9 +750,9 @@@ finalizer_thread (gpointer unused
  {
        gboolean wait = TRUE;
  
+       /* Register a hazard free queue pump callback */
+       mono_hazard_pointer_install_free_queue_size_callback (hazard_free_queue_is_too_big);
        while (!finished) {
                /* Wait to be notified that there's at least one
                 * finaliser to run
  
                reference_queue_proccess_all ();
  
+               hazard_free_queue_pump ();
                /* Avoid posting the pending done event until there are pending finalizers */
                if (mono_coop_sem_timedwait (&finalizer_sem, 0, MONO_SEM_FLAGS_NONE) == 0) {
                        /* Don't wait again at the start of the loop */
diff --combined mono/metadata/jit-info.c
index c8d5f588322040c152b1d18ec78c60ad5aa10cef,65df2246a7ffebe83efe6372ae84948c3d8a339a..e94b73d590cfe85fd055d00b341fc8d84198c924
@@@ -8,7 -8,6 +8,7 @@@
   * Copyright 2001-2003 Ximian, Inc (http://www.ximian.com)
   * Copyright 2004-2009 Novell, Inc (http://www.novell.com)
   * Copyright 2011-2012 Xamarin, Inc (http://www.xamarin.com)
 + * Licensed under the MIT license. See LICENSE file in the project root for full license information.
   */
  
  #include <config.h>
@@@ -614,7 -613,7 +614,7 @@@ jit_info_table_add (MonoDomain *domain
                *table_ptr = new_table;
                mono_memory_barrier ();
                domain->num_jit_info_tables++;
-               mono_thread_hazardous_free_or_queue (table, (MonoHazardousFreeFunc)mono_jit_info_table_free, HAZARD_FREE_MAY_LOCK, HAZARD_FREE_SAFE_CTX);
+               mono_thread_hazardous_try_free (table, (MonoHazardousFreeFunc)mono_jit_info_table_free);
                table = new_table;
  
                goto restart;
@@@ -692,7 -691,7 +692,7 @@@ mono_jit_info_free_or_queue (MonoDomai
        if (domain->num_jit_info_tables <= 1) {
                /* Can it actually happen that we only have one table
                   but ji is still hazardous? */
-               mono_thread_hazardous_free_or_queue (ji, g_free, HAZARD_FREE_MAY_LOCK, HAZARD_FREE_SAFE_CTX);
+               mono_thread_hazardous_try_free (ji, g_free);
        } else {
                domain->jit_info_free_queue = g_slist_prepend (domain->jit_info_free_queue, ji);
        }
diff --combined mono/sgen/sgen-gc.c
index f3077a6759c9b59f006c71c62f30cd028d211bcd,dcce323e982bb2d735db7c3d67ec4ddd57ddcfca..57e6f6931fc85ae980bee580694ee17c045b6d6f
   * Copyright 2011 Xamarin, Inc.
   * Copyright (C) 2012 Xamarin Inc
   *
 - * This library is free software; you can redistribute it and/or
 - * modify it under the terms of the GNU Library General Public
 - * License 2.0 as published by the Free Software Foundation;
 - *
 - * This library is distributed in the hope that it will be useful,
 - * but WITHOUT ANY WARRANTY; without even the implied warranty of
 - * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
 - * Library General Public License for more details.
 - *
 - * You should have received a copy of the GNU Library General Public
 - * License 2.0 along with this library; if not, write to the Free
 - * Software Foundation, Inc., 675 Mass Ave, Cambridge, MA 02139, USA.
 + * Licensed under the MIT license. See LICENSE file in the project root for full license information.
   *
   * Important: allocation provides always zeroed memory, having to do
   * a memset after allocation is deadly for performance.
@@@ -332,7 -343,6 +332,6 @@@ nursery_canaries_enabled (void
   * ######################################################################
   */
  MonoCoopMutex gc_mutex;
- gboolean sgen_try_free_some_memory;
  
  #define SCAN_START_SIZE       SGEN_SCAN_START_SIZE
  
@@@ -1702,7 -1712,7 +1701,7 @@@ major_copy_or_mark_from_roots (size_t *
  
        sgen_client_pre_collection_checks ();
  
 -      if (!concurrent) {
 +      if (mode != COPY_OR_MARK_FROM_ROOTS_START_CONCURRENT) {
                /* Remsets are not useful for a major collection */
                remset.clear_cards ();
        }
        sgen_init_pinning ();
        SGEN_LOG (6, "Collecting pinned addresses");
        pin_from_roots ((void*)lowest_heap_address, (void*)highest_heap_address, ctx);
 -
 +      if (mode == COPY_OR_MARK_FROM_ROOTS_FINISH_CONCURRENT) {
 +              /* Pin cemented objects that were forced */
 +              sgen_pin_cemented_objects ();
 +      }
        sgen_optimize_pin_queue ();
 +      if (mode == COPY_OR_MARK_FROM_ROOTS_START_CONCURRENT) {
 +              /*
 +               * Cemented objects that are in the pinned list will be marked. When
 +               * marking concurrently we won't mark mod-union cards for these objects.
 +               * Instead they will remain cemented until the next major collection,
 +               * when we will recheck if they are still pinned in the roots.
 +               */
 +              sgen_cement_force_pinned ();
 +      }
  
        sgen_client_collecting_major_1 ();
  
        major_collector.init_to_space ();
  
        SGEN_ASSERT (0, sgen_workers_all_done (), "Why are the workers not done when we start or finish a major collection?");
 -      /*
 -       * The concurrent collector doesn't move objects, neither on
 -       * the major heap nor in the nursery, so we can mark even
 -       * before pinning has finished.  For the non-concurrent
 -       * collector we start the workers after pinning.
 -       */
 -      if (mode == COPY_OR_MARK_FROM_ROOTS_START_CONCURRENT) {
 -              if (precleaning_enabled) {
 -                      ScanJob *sj;
 -                      /* Mod union preclean job */
 -                      sj = (ScanJob*)sgen_thread_pool_job_alloc ("preclean mod union cardtable", job_mod_union_preclean, sizeof (ScanJob));
 -                      sj->ops = object_ops;
 -                      sgen_workers_start_all_workers (object_ops, &sj->job);
 -              } else {
 -                      sgen_workers_start_all_workers (object_ops, NULL);
 -              }
 -              gray_queue_enable_redirect (WORKERS_DISTRIBUTE_GRAY_QUEUE);
 -      } else if (mode == COPY_OR_MARK_FROM_ROOTS_FINISH_CONCURRENT) {
 +      if (mode == COPY_OR_MARK_FROM_ROOTS_FINISH_CONCURRENT) {
                if (sgen_workers_have_idle_work ()) {
 +                      /*
 +                       * We force the finish of the worker with the new object ops context
 +                       * which can also do copying. We need to have finished pinning.
 +                       */
                        sgen_workers_start_all_workers (object_ops, NULL);
                        sgen_workers_join ();
                }
  
        sgen_client_collecting_major_3 (&fin_ready_queue, &critical_fin_queue);
  
 -      /*
 -       * FIXME: is this the right context?  It doesn't seem to contain a copy function
 -       * unless we're concurrent.
 -       */
 -      enqueue_scan_from_roots_jobs (heap_start, heap_end, object_ops, mode == COPY_OR_MARK_FROM_ROOTS_START_CONCURRENT);
 +      enqueue_scan_from_roots_jobs (heap_start, heap_end, object_ops, FALSE);
  
        TV_GETTIME (btv);
        time_major_scan_roots += TV_ELAPSED (atv, btv);
  
 +      /*
 +       * We start the concurrent worker after pinning and after we scanned the roots
 +       * in order to make sure that the worker does not finish before handling all
 +       * the roots.
 +       */
 +      if (mode == COPY_OR_MARK_FROM_ROOTS_START_CONCURRENT) {
 +              if (precleaning_enabled) {
 +                      ScanJob *sj;
 +                      /* Mod union preclean job */
 +                      sj = (ScanJob*)sgen_thread_pool_job_alloc ("preclean mod union cardtable", job_mod_union_preclean, sizeof (ScanJob));
 +                      sj->ops = object_ops;
 +                      sgen_workers_start_all_workers (object_ops, &sj->job);
 +              } else {
 +                      sgen_workers_start_all_workers (object_ops, NULL);
 +              }
 +              gray_queue_enable_redirect (WORKERS_DISTRIBUTE_GRAY_QUEUE);
 +      }
 +
        if (mode == COPY_OR_MARK_FROM_ROOTS_FINISH_CONCURRENT) {
                ScanJob *sj;
  
@@@ -1852,6 -1849,11 +1851,6 @@@ static voi
  major_finish_copy_or_mark (CopyOrMarkFromRootsMode mode)
  {
        if (mode == COPY_OR_MARK_FROM_ROOTS_START_CONCURRENT) {
 -              /*
 -               * Prepare the pin queue for the next collection.  Since pinning runs on the worker
 -               * threads we must wait for the jobs to finish before we can reset it.
 -               */
 -              sgen_workers_wait_for_jobs_finished ();
                sgen_finish_pinning ();
  
                sgen_pin_stats_reset ();
@@@ -3176,11 -3178,7 +3175,7 @@@ sgen_gc_lock (void
  void
  sgen_gc_unlock (void)
  {
-       gboolean try_free = sgen_try_free_some_memory;
-       sgen_try_free_some_memory = FALSE;
        mono_coop_mutex_unlock (&gc_mutex);
-       if (try_free)
-               mono_thread_hazardous_try_free_some ();
  }
  
  void
@@@ -3247,8 -3245,6 +3242,6 @@@ sgen_restart_world (int generation, GGT
  
        binary_protocol_world_restarted (generation, sgen_timestamp ());
  
-       sgen_try_free_some_memory = TRUE;
        if (sgen_client_bridge_need_processing ())
                sgen_client_bridge_processing_finish (generation);
  
index f5e9f73ed3595d4efe48a41bf78f1884f6735238,de3dc113fb16170a056369d1254d82ea84c1e5b9..efe6b649b3039450cbdc176f0373cd55754a69be
@@@ -2,7 -2,6 +2,7 @@@
   * hazard-pointer.c: Hazard pointer related code.
   *
   * (C) Copyright 2011 Novell, Inc
 + * Licensed under the MIT license. See LICENSE file in the project root for full license information.
   */
  
  #include <config.h>
@@@ -43,6 -42,7 +43,7 @@@ typedef struct 
  
  static volatile int hazard_table_size = 0;
  static MonoThreadHazardPointers * volatile hazard_table = NULL;
+ static MonoHazardFreeQueueSizeCallback queue_size_cb;
  
  /*
   * Each entry is either 0 or 1, indicating whether that overflow small
@@@ -306,29 -306,63 +307,63 @@@ try_free_delayed_free_item (HazardFreeC
        return TRUE;
  }
  
+ /**
+  * mono_thread_hazardous_try_free:
+  * @p: the pointer to free
+  * @free_func: the function that can free the pointer
+  *
+  * If @p is not a hazardous pointer it will be immediately freed by calling @free_func.
+  * Otherwise it will be queued for later.
+  *
+  * Use this function if @free_func can ALWAYS be called in the context where this function is being called.
+  *
+  * This function doesn't pump the free queue so try to accommodate a call at an appropriate time.
+  * See mono_thread_hazardous_try_free_some for when it's appropriate.
+  *
+  * Return: TRUE if @p was free or FALSE if it was queued.
+  */
+ gboolean
+ mono_thread_hazardous_try_free (gpointer p, MonoHazardousFreeFunc free_func)
+ {
+       if (!is_pointer_hazardous (p)) {
+               free_func (p);
+               return TRUE;
+       } else {
+               mono_thread_hazardous_queue_free (p, free_func);
+               return FALSE;
+       }
+ }
+ /**
+  * mono_thread_hazardous_queue_free:
+  * @p: the pointer to free
+  * @free_func: the function that can free the pointer
+  *
+  * Queue @p to be freed later. @p will be freed once the hazard free queue is pumped.
+  *
+  * This function doesn't pump the free queue so try to accommodate a call at an appropriate time.
+  * See mono_thread_hazardous_try_free_some for when it's appropriate.
+  *
+  */
  void
- mono_thread_hazardous_free_or_queue (gpointer p, MonoHazardousFreeFunc free_func,
-                                      HazardFreeLocking locking, HazardFreeContext context)
+ mono_thread_hazardous_queue_free (gpointer p, MonoHazardousFreeFunc free_func)
  {
-       int i;
+       DelayedFreeItem item = { p, free_func, HAZARD_FREE_MAY_LOCK };
  
-       /* First try to free a few entries in the delayed free
-          table. */
-       for (i = 0; i < 3; ++i)
-               try_free_delayed_free_item (context);
+       InterlockedIncrement (&hazardous_pointer_count);
  
-       /* Now see if the pointer we're freeing is hazardous.  If it
-          isn't, free it.  Otherwise put it in the delay list. */
-       if ((context == HAZARD_FREE_ASYNC_CTX && locking == HAZARD_FREE_MAY_LOCK) ||
-           is_pointer_hazardous (p)) {
-               DelayedFreeItem item = { p, free_func, locking };
+       mono_lock_free_array_queue_push (&delayed_free_queue, &item);
  
-               ++hazardous_pointer_count;
+       guint32 queue_size = delayed_free_queue.num_used_entries;
+       if (queue_size && queue_size_cb)
+               queue_size_cb (queue_size);
+ }
  
-               mono_lock_free_array_queue_push (&delayed_free_queue, &item);
-       } else {
-               free_func (p);
-       }
+ void
+ mono_hazard_pointer_install_free_queue_size_callback (MonoHazardFreeQueueSizeCallback cb)
+ {
+       queue_size_cb = cb;
  }
  
  void
index ae3a1a1d8873968dd3f5101e66b99eccb4406f04,4805eede2bf3135475602bca4b45f14aea56ebc5..3cb2c0aeba33817f0aad3b33bdb9c906913e6fb9
@@@ -2,7 -2,6 +2,7 @@@
   * hazard-pointer.h: Hazard pointer related code.
   *
   * (C) Copyright 2011 Novell, Inc
 + * Licensed under the MIT license. See LICENSE file in the project root for full license information.
   */
  #ifndef __MONO_HAZARD_POINTER_H__
  #define __MONO_HAZARD_POINTER_H__
@@@ -29,8 -28,9 +29,9 @@@ typedef enum 
        HAZARD_FREE_ASYNC_CTX,
  } HazardFreeContext;
  
- void mono_thread_hazardous_free_or_queue (gpointer p, MonoHazardousFreeFunc free_func,
-                                           HazardFreeLocking locking, HazardFreeContext context);
+ gboolean mono_thread_hazardous_try_free (gpointer p, MonoHazardousFreeFunc free_func);
+ void mono_thread_hazardous_queue_free (gpointer p, MonoHazardousFreeFunc free_func);
  void mono_thread_hazardous_try_free_all (void);
  void mono_thread_hazardous_try_free_some (void);
  MonoThreadHazardPointers* mono_hazard_pointer_get (void);
@@@ -58,6 -58,9 +59,9 @@@ int mono_thread_small_id_alloc (void)
  int mono_hazard_pointer_save_for_signal_handler (void);
  void mono_hazard_pointer_restore_for_signal_handler (int small_id);
  
+ typedef void (*MonoHazardFreeQueueSizeCallback)(size_t size);
+ void mono_hazard_pointer_install_free_queue_size_callback (MonoHazardFreeQueueSizeCallback cb);
  void mono_thread_smr_init (void);
  void mono_thread_smr_cleanup (void);
  #endif /*__MONO_HAZARD_POINTER_H__*/
index b32f592617806d65e7804123016c82d59933a73f,42d403f80d0a73645aa7f728af8bce5a2d5433e5..e879d910f1c2e60fe32934c079f98eb840b24169
@@@ -3,7 -3,24 +3,7 @@@
   *
   * (C) Copyright 2011 Novell, Inc
   *
 - * Permission is hereby granted, free of charge, to any person obtaining
 - * a copy of this software and associated documentation files (the
 - * "Software"), to deal in the Software without restriction, including
 - * without limitation the rights to use, copy, modify, merge, publish,
 - * distribute, sublicense, and/or sell copies of the Software, and to
 - * permit persons to whom the Software is furnished to do so, subject to
 - * the following conditions:
 - * 
 - * The above copyright notice and this permission notice shall be
 - * included in all copies or substantial portions of the Software.
 - * 
 - * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND,
 - * EXPRESS OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF
 - * MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND
 - * NONINFRINGEMENT. IN NO EVENT SHALL THE AUTHORS OR COPYRIGHT HOLDERS BE
 - * LIABLE FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY, WHETHER IN AN ACTION
 - * OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION
 - * WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE SOFTWARE.
 + * Licensed under the MIT license. See LICENSE file in the project root for full license information.
   */
  
  /*
@@@ -233,7 -250,7 +233,7 @@@ desc_retire (Descriptor *desc
        g_assert (desc->in_use);
        desc->in_use = FALSE;
        free_sb (desc->sb, desc->block_size);
-       mono_thread_hazardous_free_or_queue (desc, desc_enqueue_avail, HAZARD_FREE_NO_LOCK, HAZARD_FREE_ASYNC_CTX);
+       mono_thread_hazardous_try_free (desc, desc_enqueue_avail);
  }
  #else
  MonoLockFreeQueue available_descs;
@@@ -285,7 -302,7 +285,7 @@@ static voi
  list_put_partial (Descriptor *desc)
  {
        g_assert (desc->anchor.data.state != STATE_FULL);
-       mono_thread_hazardous_free_or_queue (desc, desc_put_partial, HAZARD_FREE_NO_LOCK, HAZARD_FREE_ASYNC_CTX);
+       mono_thread_hazardous_try_free (desc, desc_put_partial);
  }
  
  static void
@@@ -304,7 -321,7 +304,7 @@@ list_remove_empty_desc (MonoLockFreeAll
                        desc_retire (desc);
                } else {
                        g_assert (desc->heap->sc == sc);
-                       mono_thread_hazardous_free_or_queue (desc, desc_put_partial, HAZARD_FREE_NO_LOCK, HAZARD_FREE_ASYNC_CTX);
+                       mono_thread_hazardous_try_free (desc, desc_put_partial);
                        if (++num_non_empty >= 2)
                                return;
                }
index 21fae2f60784525c7d4592470a95c0532d4a898b,5f77452f23a87a9040145a083aa5bd251c7fa0d5..0b142ee1c3f0024fedbce186aa41c65df83e484e
@@@ -6,7 -6,6 +6,7 @@@
   *
   * Copyright 2011 Novell, Inc (http://www.novell.com)
   * Copyright 2011 Xamarin, Inc (http://www.xamarin.com)
 + * Licensed under the MIT license. See LICENSE file in the project root for full license information.
   */
  
  #include <config.h>
@@@ -425,7 -424,10 +425,10 @@@ unregister_thread (void *arg
        g_byte_array_free (info->stackdata, /*free_segment=*/TRUE);
  
        /*now it's safe to free the thread info.*/
-       mono_thread_hazardous_free_or_queue (info, free_thread_info, HAZARD_FREE_MAY_LOCK, HAZARD_FREE_SAFE_CTX);
+       mono_thread_hazardous_try_free (info, free_thread_info);
+       /* Pump the HP queue */
+       mono_thread_hazardous_try_free_some ();
        mono_thread_small_id_free (small_id);
  }