Re: External thread suspension (again)

[Keith Seitz <keiths@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 2115 bytes --]

Hans Boehm wrote:

> Some comments/questions:
> 
> 1) I don't see where the SUSPENDED flag gets set in response to
> GC_suspend_thread.  It gets set in GC_suspend_self(), but I don't
> think you get there unless it has already been set?

That gets set in GC_suspend_self. When GC_stop_world is called, 
GC_suspend_all will not signal the externally suspended thread (because 
GC_start_blocking will set thread_blocked).

> 2) You should probably call GC_brief_async_signal_safe_sleep().
> Last time I looked nanosleep wasn't technically async-signal-safe,
> but I think select() always has been.

I've changed that. Thanks.

> 3) Have you thought about the various possible signal races, and which
> signals are masked at what point?  I haven't yet had a chance to.  It
> may be fine, but if there's an obvious argument that it is, a
> comment would be great.

I've thought about it a bit, and the only problem that I see is in the 
signal handler code. *If* a non-blocked signal were to make it to an 
externally suspended thread (and we ended up in our signal handler), it 
would GC_suspend_self itself again. In which case, we would need to 
differentiate between a thread that is supposed to suspend itself and a 
thread which is already suspended.

But since we only install a signal handler for SUSPEND and THR_RESTART, 
so it shouldn't be an issue. A suspended thread should not get 
THR_RESTART, since blocked_thread is set by GC_start_blocking.

Or at least, that's all I could divine.

> 4) IIRC, GC_start_blocking() has a fundamental issue in that there isn't
> really a good way to guarantee that all callee-saves registers are visible
> onthe stack at the right point.  The result might be very occasional
> failures.  So long as this is used only for debugging, and since this
> is fixed with an interface change in 7.0, which can be easily accomodated
> by your patch, I'd lean towards ignoring this for now.  The more
> ambitious, and more correct, solution would be to backport the gc7
> do_blocking() code.

Okay, I will keep that in mind.

I've attached the latest revision of this patch.

Keith

[-- Attachment #2: external-suspend.patch --]
[-- Type: text/x-patch, Size: 3338 bytes --]

Index: include/gc.h
===================================================================
--- include/gc.h	(revision 114118)
+++ include/gc.h	(working copy)
@@ -1040,4 +1040,15 @@
     }  /* end of extern "C" */
 #endif
 
+/* External thread suspension support. These functions do not implement
+ * suspension counts or any other higher-level abstraction. Threads which
+ * have been suspended numerous times will resume with the very first call
+ * to GC_resume_thread.
+ */
+#if defined(GC_PTHREADS) && !defined(GC_SOLARIS_THREADS) \
+  && !defined(GC_WIN32_THREADS) && !defined(GC_DARWIN_THREADS)
+GC_API void GC_suspend_self GC_PROTO((void));
+GC_API void GC_suspend_thread GC_PROTO((pthread_t));
+GC_API void GC_resume_thread GC_PROTO((pthread_t));
+#endif
 #endif /* _GC_H */
Index: include/private/pthread_support.h
===================================================================
--- include/private/pthread_support.h	(revision 114118)
+++ include/private/pthread_support.h	(working copy)
@@ -33,6 +33,7 @@
 #	define FINISHED 1   	/* Thread has exited.	*/
 #	define DETACHED 2	/* Thread is intended to be detached.	*/
 #	define MAIN_THREAD 4	/* True for the original thread only.	*/
+#       define SUSPENDED 8      /* True if thread was suspended externally */
     short thread_blocked;	/* Protected by GC lock.		*/
     				/* Treated as a boolean value.  If set,	*/
     				/* thread will acquire GC lock before	*/
Index: pthread_stop_world.c
===================================================================
--- pthread_stop_world.c	(revision 114118)
+++ pthread_stop_world.c	(working copy)
@@ -127,9 +127,14 @@
 
 void GC_suspend_handler(int sig)
 {
-  int old_errno = errno;
-  GC_with_callee_saves_pushed(GC_suspend_handler_inner, (ptr_t)(word)sig);
-  errno = old_errno;
+  GC_thread me = GC_lookup_thread (pthread_self());
+  if (me -> flags & SUSPENDED)
+    GC_suspend_self();
+  else {
+    int old_errno = errno;
+    GC_with_callee_saves_pushed(GC_suspend_handler_inner, (ptr_t)(word)sig);
+    errno = old_errno;
+  }
 }
 
 #else
@@ -137,9 +142,14 @@
 /* in the signal handler frame.						*/
 void GC_suspend_handler(int sig)
 {
-  int old_errno = errno;
-  GC_suspend_handler_inner((ptr_t)(word)sig);
-  errno = old_errno;
+  GC_thread me = GC_lookup_thread(pthread_self());
+  if (me -> flags & SUSPENDED)
+    GC_suspend_self();
+  else {
+    int old_errno = errno;
+    GC_suspend_handler_inner((ptr_t)(word)sig);
+    errno = old_errno;
+  }
 }
 #endif
 
@@ -430,6 +440,33 @@
     GC_stopping_thread = 0;  /* debugging only */
 }
 
+void GC_suspend_self() {
+  GC_thread me = GC_lookup_thread(pthread_self());
+  me -> flags |= SUSPENDED;
+  GC_start_blocking();
+  while (me -> flags & SUSPENDED)
+    GC_brief_async_signal_safe_sleep();
+  GC_end_blocking();
+}
+
+void GC_suspend_thread(pthread_t thread) {
+  int result;
+  GC_thread t = GC_lookup_thread(thread);
+  result = pthread_kill (t -> id, SIG_SUSPEND);
+  switch (result) {
+      case ESRCH:
+      case 0:
+          break;
+      default:
+          ABORT("pthread_kill failed");
+  }
+}
+
+void GC_resume_thread(pthread_t thread) {
+  GC_thread t = GC_lookup_thread(thread);
+  t -> flags &= ~SUSPENDED;
+}
+
 /* Caller holds allocation lock, and has held it continuously since	*/
 /* the world stopped.							*/
 void GC_start_world()