[utils] Fix lock-free data structures.

Hazard pointers didn't check for the third pointer.

Lots of memory barriers missing.

Small bug in LLS.

Comments and asserts.

diff --git a/mono/utils/hazard-pointer.c b/mono/utils/hazard-pointer.c
index e808d9e1102246649c568da8d500c96301157c60..90a1779f562dbb686325a333ef88017f6170c9c0 100644 (file)
--- a/mono/utils/hazard-pointer.c
+++ b/mono/utils/hazard-pointer.c
@@ -150,13 +150,15 @@ delayed_free_push (DelayedFreeItem item)
 
        entry->state = DFE_STATE_USED;
 
-       mono_memory_write_barrier ();
+       mono_memory_barrier ();
 
        do {
                num_used = num_used_delayed_free_entries;
                if (num_used > index)
                        break;
        } while (InterlockedCompareExchange (&num_used_delayed_free_entries, index + 1, num_used) != num_used);
+
+       mono_memory_write_barrier ();
 }
 
 static gboolean
@@ -175,6 +177,7 @@ delayed_free_pop (DelayedFreeItem *item)
                entry = get_delayed_free_entry (index - 1);
        } while (InterlockedCompareExchange (&entry->state, DFE_STATE_BUSY, DFE_STATE_USED) != DFE_STATE_USED);
 
+       /* Reading the item must happen before CASing the state. */
        mono_memory_barrier ();
 
        *item = entry->item;
@@ -183,6 +186,8 @@ delayed_free_pop (DelayedFreeItem *item)
 
        entry->state = DFE_STATE_FREE;
 
+       mono_memory_write_barrier ();
+
        return TRUE;
 }
 
@@ -280,15 +285,16 @@ mono_thread_small_id_free (int id)
 static gboolean
 is_pointer_hazardous (gpointer p)
 {
-       int i;
+       int i, j;
        int highest = highest_small_id;
 
        g_assert (highest < hazard_table_size);
 
        for (i = 0; i <= highest; ++i) {
-               if (hazard_table [i].hazard_pointers [0] == p
-                               || hazard_table [i].hazard_pointers [1] == p)
-                       return TRUE;
+               for (j = 0; j < HAZARD_POINTER_COUNT; ++j) {
+                       if (hazard_table [i].hazard_pointers [j] == p)
+                               return TRUE;
+               }
        }
 
        return FALSE;

diff --git a/mono/utils/mono-linked-list-set.c b/mono/utils/mono-linked-list-set.c
index d9ab6210c381274670d32a9340ab2e6fb06df3c3..3f34359f2731ae779c1b37a0a52f0068fe0d1db8 100644 (file)
--- a/mono/utils/mono-linked-list-set.c
+++ b/mono/utils/mono-linked-list-set.c
@@ -39,6 +39,9 @@ get_hazardous_pointer_with_mask (gpointer volatile *pp, MonoThreadHazardPointers
                        return p;
                /* Make it hazardous */
                mono_hazard_pointer_set (hp, hazard_index, mono_lls_pointer_unmask (p));
+
+               mono_memory_barrier ();
+
                /* Check that it's still the same.  If not, try
                   again. */
                if (*pp != p) {
@@ -81,9 +84,17 @@ mono_lls_find (MonoLinkedListSet *list, MonoThreadHazardPointers *hp, uintptr_t
 
 try_again:
        prev = &list->head;
+
+       /*
+        * prev is not really a hazardous pointer, but we return prev
+        * in hazard pointer 2, so we set it here.  Note also that
+        * prev is not a pointer to a node.  We use here the fact that
+        * the first element in a node is the next pointer, so it
+        * works, but it's not pretty.
+        */
        mono_hazard_pointer_set (hp, 2, prev);
 
-       cur = mono_lls_pointer_unmask (get_hazardous_pointer ((gpointer*)prev, hp, 1));
+       cur = get_hazardous_pointer_with_mask ((gpointer*)prev, hp, 1);
 
        while (1) {
                if (cur == NULL)
@@ -91,6 +102,13 @@ try_again:
                next = get_hazardous_pointer_with_mask ((gpointer*)&cur->next, hp, 0);
                cur_key = cur->key;
 
+               /*
+                * We need to make sure that we dereference prev below
+                * after reading cur->next above, so we need a read
+                * barrier.
+                */
+               mono_memory_read_barrier ();
+
                if (*prev != cur)
                        goto try_again;
 
@@ -102,7 +120,10 @@ try_again:
                        mono_hazard_pointer_set (hp, 2, cur);
                } else {
                        next = mono_lls_pointer_unmask (next);
-                       if (InterlockedCompareExchangePointer ((volatile gpointer*)prev, next, cur) == next) {
+                       if (InterlockedCompareExchangePointer ((volatile gpointer*)prev, next, cur) == cur) {
+                               /* The hazard pointer must be cleared after the CAS. */
+                               mono_memory_write_barrier ();
+                               mono_hazard_pointer_clear (hp, 1);
                                if (list->free_node_func)
                                        mono_thread_hazardous_free_or_queue (cur, list->free_node_func);
                        } else
@@ -136,6 +157,8 @@ mono_lls_insert (MonoLinkedListSet *list, MonoThreadHazardPointers *hp, MonoLink
 
                value->next = cur;
                mono_hazard_pointer_set (hp, 0, value);
+               /* The CAS must happen after setting the hazard pointer. */
+               mono_memory_write_barrier ();
                if (InterlockedCompareExchangePointer ((volatile gpointer*)prev, value, cur) == cur)
                        return TRUE;
        }
@@ -159,9 +182,16 @@ mono_lls_remove (MonoLinkedListSet *list, MonoThreadHazardPointers *hp, MonoLink
                cur = mono_hazard_pointer_get_val (hp, 1);
                prev = mono_hazard_pointer_get_val (hp, 2);
 
+               g_assert (cur == value);
+
                if (InterlockedCompareExchangePointer ((volatile gpointer*)&cur->next, mask (next, 1), next) != next)
                        continue;
+               /* The second CAS must happen before the first. */
+               mono_memory_write_barrier ();
                if (InterlockedCompareExchangePointer ((volatile gpointer*)prev, next, cur) == cur) {
+                       /* The CAS must happen before the hazard pointer clear. */
+                       mono_memory_write_barrier ();
+                       mono_hazard_pointer_clear (hp, 1);
                        if (list->free_node_func)
                                mono_thread_hazardous_free_or_queue (value, list->free_node_func);
                } else

diff --git a/mono/utils/mono-linked-list-set.h b/mono/utils/mono-linked-list-set.h
index d20cd17e44e05cb5612d52c7a81eeada9e09195a..5009fc8c8c2a06994d71970d3ed3e6dba95e5026 100644 (file)
--- a/mono/utils/mono-linked-list-set.h
+++ b/mono/utils/mono-linked-list-set.h
@@ -16,6 +16,7 @@
 typedef struct _MonoLinkedListSetNode MonoLinkedListSetNode;
 
 struct _MonoLinkedListSetNode {
+       /* next must be the first element in this struct! */
        MonoLinkedListSetNode *next;
        uintptr_t key;
 };
@@ -63,9 +64,9 @@ Requires the world to be stoped
 */
 #define MONO_LLS_FOREACH(list, element) {\
        MonoLinkedListSetNode *__cur;   \
-       for (__cur = list->head; __cur; __cur = mono_lls_pointer_unmask (__cur->next))  \
+       for (__cur = (list)->head; __cur; __cur = mono_lls_pointer_unmask (__cur->next)) \
                if (!mono_lls_pointer_get_mark (__cur->next)) { \
-                       element = (typeof(element))__cur;       \
+                       (element) = (typeof((element)))__cur;                   \
 
 #define MONO_LLS_END_FOREACH }}
 
@@ -83,12 +84,12 @@ Provides snapshot iteration
 #define MONO_LLS_FOREACH_SAFE(list, element) {\
        MonoThreadHazardPointers *__hp = mono_hazard_pointer_get ();    \
        MonoLinkedListSetNode *__cur, *__next;  \
-       for (__cur = mono_lls_pointer_unmask (get_hazardous_pointer ((gpointer volatile*)&list->head, __hp, 1)); \
+       for (__cur = mono_lls_pointer_unmask (get_hazardous_pointer ((gpointer volatile*)&(list)->head, __hp, 1)); \
                __cur;  \
                __cur = mono_lls_info_step (__next, __hp)) {    \
                __next = get_hazardous_pointer_with_mask ((gpointer volatile*)&__cur->next, __hp, 0);   \
                if (!mono_lls_pointer_get_mark (__next)) {      \
-                       element = (typeof(element))__cur;
+                       (element) = (typeof((element)))__cur;
 
 #define MONO_LLS_END_FOREACH_SAFE \
                } \