[webkit-changes] [290110] trunk/Source/bmalloc

Thu, 17 Feb 2022 20:17:03 -0800

Title: [290110] trunk/Source/bmalloc
Revision
290110
Author
[email protected]
Date
2022-02-17 20:16:55 -0800 (Thu, 17 Feb 2022)

Log Message

[libpas] scavenger should not call pthread_mach_thread_np after suspension
https://bugs.webkit.org/show_bug.cgi?id=236798
rdar://89020902

Reviewed by Mark Lam and Keith Miller.

pthread_mach_thread_np can take a lock internally. So we should not call it after suspending a thread.
This patch refactors libpas scavenger so that we call pthread_mach_thread_np before suspending a thread.
We also avoid calling pthread_self() after suspension to make suspending safer (while it does not take
a lock, it also depends on an internal implementation).

* libpas/src/libpas/pas_thread_local_cache.c:
(scavenger_thread_suspend_data_create):
(suspend):
(resume):
(stop_allocator):
(pas_thread_local_cache_for_all):

Modified Paths

Diff

Modified: trunk/Source/bmalloc/ChangeLog (290109 => 290110)


--- trunk/Source/bmalloc/ChangeLog	2022-02-18 03:55:27 UTC (rev 290109)
+++ trunk/Source/bmalloc/ChangeLog	2022-02-18 04:16:55 UTC (rev 290110)
@@ -1,5 +1,25 @@
 2022-02-17  Yusuke Suzuki  <[email protected]>
 
+        [libpas] scavenger should not call pthread_mach_thread_np after suspension
+        https://bugs.webkit.org/show_bug.cgi?id=236798
+        rdar://89020902
+
+        Reviewed by Mark Lam and Keith Miller.
+
+        pthread_mach_thread_np can take a lock internally. So we should not call it after suspending a thread.
+        This patch refactors libpas scavenger so that we call pthread_mach_thread_np before suspending a thread.
+        We also avoid calling pthread_self() after suspension to make suspending safer (while it does not take
+        a lock, it also depends on an internal implementation).
+
+        * libpas/src/libpas/pas_thread_local_cache.c:
+        (scavenger_thread_suspend_data_create):
+        (suspend):
+        (resume):
+        (stop_allocator):
+        (pas_thread_local_cache_for_all):
+
+2022-02-17  Yusuke Suzuki  <[email protected]>
+
         Unreviewed, fix heap enumeration
         https://bugs.webkit.org/show_bug.cgi?id=236662

Modified: trunk/Source/bmalloc/libpas/src/libpas/pas_thread_local_cache.c (290109 => 290110)


--- trunk/Source/bmalloc/libpas/src/libpas/pas_thread_local_cache.c	2022-02-18 03:55:27 UTC (rev 290109)
+++ trunk/Source/bmalloc/libpas/src/libpas/pas_thread_local_cache.c	2022-02-18 04:16:55 UTC (rev 290110)
@@ -774,20 +774,35 @@
         thread_local_cache, heap_lock_hold_mode, pas_segregated_deallocation_direct_mode);
 }
 
+typedef struct scavenger_thread_suspend_data {
+    bool did_suspend;
+    bool is_scavenger_itself;
 #if PAS_OS(DARWIN)
+    mach_port_t mach_thread;
+#endif
+} scavenger_thread_suspend_data;
 
-static void suspend(pas_thread_local_cache* cache, bool* did_suspend)
+static scavenger_thread_suspend_data scavenger_thread_suspend_data_create()
 {
+    scavenger_thread_suspend_data thread_suspend_data;
+    thread_suspend_data.did_suspend = false;
+    thread_suspend_data.is_scavenger_itself = false;
+    return thread_suspend_data;
+}
+
+#if PAS_OS(DARWIN)
+
+static void suspend(pas_thread_local_cache* cache, scavenger_thread_suspend_data* thread_suspend_data)
+{
     static const bool verbose = false;
 
     pthread_t thread;
-    mach_port_t mach_thread;
     kern_return_t result;
 
-    if (*did_suspend)
+    if (thread_suspend_data->did_suspend)
         return;
 
-    *did_suspend = true;
+    thread_suspend_data->did_suspend = true;
     
     if (verbose)
         printf("Suspending TLC %p with thread %p.\n", cache, cache->thread);
@@ -795,11 +810,12 @@
     thread = cache->thread;
     PAS_ASSERT(thread);
     
-    if (thread == pthread_self())
+    thread_suspend_data->is_scavenger_itself = thread == pthread_self();
+    if (thread_suspend_data->is_scavenger_itself)
         return;
 
-    mach_thread = pthread_mach_thread_np(thread);
-    result = thread_suspend(mach_thread);
+    thread_suspend_data->mach_thread = pthread_mach_thread_np(thread);
+    result = thread_suspend(thread_suspend_data->mach_thread);
     
     /* Fun fact: it's impossible for us to try to suspend a thread that has exited, since
        thread exit for any thread with a TLC needs to grab the heap lock and we hold the
@@ -807,13 +823,13 @@
 
     if (result != KERN_SUCCESS) {
         pas_log("[%d] Failed to suspend pthread %p (mach thread %d) associated with TLC %p: %d\n",
-                getpid(), thread, mach_thread, cache, result);
+                getpid(), thread, thread_suspend_data->mach_thread, cache, result);
         dump_thread_diagnostics(thread);
         PAS_ASSERT(result == KERN_SUCCESS);
     }
 }
 
-static void resume(pas_thread_local_cache* cache)
+static void resume(pas_thread_local_cache* cache, scavenger_thread_suspend_data* thread_suspend_data)
 {
     static const bool verbose = false;
     
@@ -822,10 +838,10 @@
     if (verbose)
         pas_log("Resuming TLC %p with thread %p.\n", cache, cache->thread);
     
-    if (cache->thread == pthread_self())
+    if (thread_suspend_data->is_scavenger_itself)
         return;
 
-    result = thread_resume(pthread_mach_thread_np(cache->thread));
+    result = thread_resume(thread_suspend_data->mach_thread);
     
     PAS_ASSERT(result == KERN_SUCCESS);
 }
@@ -908,7 +924,7 @@
                            pas_allocator_index allocator_index,
                            pas_local_allocator_scavenger_data* scavenger_data,
                            bool* result,
-                           bool* did_suspend)
+                           scavenger_thread_suspend_data* thread_suspend_data)
 {
     static const bool verbose = false;
     
@@ -970,9 +986,9 @@
     if (verbose)
         pas_log("Need to suspend for allocator %p\n", scavenger_data);
 
-    suspend(cache, did_suspend);
+    suspend(cache, thread_suspend_data);
 
-    PAS_ASSERT(*did_suspend);
+    PAS_ASSERT(thread_suspend_data->did_suspend);
                 
     if (scavenger_data->is_in_use) {
         *result = true;
@@ -989,7 +1005,7 @@
             scavenger_data, pas_lock_lock_mode_try_lock, pas_lock_is_held))
         *result = true;
 #else
-    PAS_UNUSED_PARAM(did_suspend);
+    PAS_UNUSED_PARAM(thread_suspend_data);
 #endif
 }
 
@@ -1054,7 +1070,7 @@
         }
 
         if (allocator_action != pas_allocator_scavenge_no_action) {
-            bool did_suspend;
+            scavenger_thread_suspend_data thread_suspend_data;
             pas_thread_local_cache_layout_node layout_node;
             pas_thread_local_cache_layout_segment* segment;
             pas_thread_local_cache_layout_segment* begin_segment;
@@ -1065,12 +1081,12 @@
             uintptr_t node_index;
             uintptr_t begin_node_index;
             
-            did_suspend = false;
+            thread_suspend_data = scavenger_thread_suspend_data_create();
 
             start_of_possible_decommit =
                 pas_thread_local_cache_offset_of_allocator(PAS_LOCAL_ALLOCATOR_UNSELECTED_NUM_INDICES);
 
-            PAS_ASSERT(!did_suspend);
+            PAS_ASSERT(!thread_suspend_data.did_suspend);
 
             begin_segment = NULL;
             begin_node_index = 0;
@@ -1102,7 +1118,7 @@
                            <= decommit_exclusion_range.end_of_possible_decommit);
                 if (pas_decommit_exclusion_range_is_contiguous(decommit_exclusion_range)) {
                     stop_allocator(
-                        cache, allocator_action, allocator_index, scavenger_data, &result, &did_suspend);
+                        cache, allocator_action, allocator_index, scavenger_data, &result, &thread_suspend_data);
 
                     if (pas_local_allocator_scavenger_data_is_stopped(scavenger_data)) {
                         if (!begin_segment) {
@@ -1130,8 +1146,8 @@
             }
             
 #if PAS_OS(DARWIN)
-            if (did_suspend)
-                resume(cache);
+            if (thread_suspend_data.did_suspend)
+                resume(cache, &thread_suspend_data);
 #endif
 
             begin_page_index =

_______________________________________________
webkit-changes mailing list
[email protected]
https://lists.webkit.org/mailman/listinfo/webkit-changes

Reply via email to