Modified: branches/safari-613-branch/Source/bmalloc/ChangeLog (291078 => 291079)
--- branches/safari-613-branch/Source/bmalloc/ChangeLog 2022-03-09 22:38:29 UTC (rev 291078)
+++ branches/safari-613-branch/Source/bmalloc/ChangeLog 2022-03-09 22:45:42 UTC (rev 291079)
@@ -1,3 +1,23 @@
+2022-02-17 Yusuke Suzuki <ysuz...@apple.com>
+
+ [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-03-07 Russell Epstein <repst...@apple.com>
Cherry-pick r290195. rdar://problem/88402366
Modified: branches/safari-613-branch/Source/bmalloc/libpas/src/libpas/pas_thread_local_cache.c (291078 => 291079)
--- branches/safari-613-branch/Source/bmalloc/libpas/src/libpas/pas_thread_local_cache.c 2022-03-09 22:38:29 UTC (rev 291078)
+++ branches/safari-613-branch/Source/bmalloc/libpas/src/libpas/pas_thread_local_cache.c 2022-03-09 22:45:42 UTC (rev 291079)
@@ -584,25 +584,45 @@
pas_scavenger_notify_eligibility_if_needed();
}
+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)
+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 (thread_suspend_data->did_suspend)
+ return;
+ thread_suspend_data->did_suspend = true;
if (verbose)
printf("Suspending TLC %p with thread %p.\n", cache, cache->thread);
thread = cache->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
@@ -610,20 +630,21 @@
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)
{
kern_return_t result;
- if (cache->thread == pthread_self())
+ PAS_UNUSED_PARAM(cache);
+ 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);
}
@@ -688,14 +709,14 @@
}
if (allocator_action != pas_allocator_scavenge_no_action) {
- bool did_suspend;
+ scavenger_thread_suspend_data thread_suspend_data;
pas_thread_local_cache_layout_segment* segment;
pas_thread_local_cache_layout_node layout_node;
uintptr_t node_index;
- did_suspend = false;
+ thread_suspend_data = scavenger_thread_suspend_data_create();
- PAS_UNUSED_PARAM(did_suspend);
+ PAS_ASSERT(!thread_suspend_data.did_suspend);
segment = pas_thread_local_cache_layout_first_segment;
node_index = 0;
@@ -769,10 +790,7 @@
if (verbose)
pas_log("Need to suspend for allocator %p\n", scavenger_data);
- if (!did_suspend) {
- suspend(cache);
- did_suspend = true;
- }
+ suspend(cache, &thread_suspend_data);
if (scavenger_data->is_in_use) {
result = true;
@@ -788,8 +806,8 @@
}
#if PAS_OS(DARWIN)
- if (did_suspend)
- resume(cache);
+ if (thread_suspend_data.did_suspend)
+ resume(cache, &thread_suspend_data);
#endif
}
