* External thread suspension (again)
@ 2006-05-26 21:43 Keith Seitz
2006-06-04 4:47 ` Hans Boehm
0 siblings, 1 reply; 6+ messages in thread
From: Keith Seitz @ 2006-05-26 21:43 UTC (permalink / raw)
To: gc; +Cc: Java Patch List
[-- Attachment #1: Type: text/plain, Size: 1288 bytes --]
Including GC list
-------- Original Message --------
Subject: Re: [RFC] GC: external thread suspension
Date: Thu, 25 May 2006 12:39:58 -0700
From: Keith Seitz <keiths@redhat.com>
To: Java Patch List <java-patches@gcc.gnu.org>
Okay, since it has been relatively quiet for the past week, I thought I
would turn this into an official RFA.
I'm going to be conservative, and offer up for approval the patch
suggested by Hans Boehm, using GC_start/end_blocking. It is much simpler
than my original patch.
In order to modify gc.h, it looks like I need to ifdef stuff. Since I've
only done this for pthreads, I've made a best guess as to the proper
conditions. Let me know if there are any problems with this.
Keith
ChangeLog
2006-05-25 Keith Seitz <keiths@redhat.com>
* pthread_stop_world.c (GC_suspend_handler): Redirect to
suspension
routine if signal is received and thread is flagged SUSPENDED.
(GC_suspend_self): New function.
(GC_suspend_thread): New function.
(GC_resume_thread): New function.
* include/gc.h (GC_suspend_self): Declare.
(GC_suspend_thread): Declare.
(GC_resumet_thread): Declare.
* include/private/pthread_support.h (SUSPENDED): New GC_thread
flag.
[-- Attachment #2: external-suspend.patch --]
[-- Type: text/x-patch, Size: 3397 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,36 @@
GC_stopping_thread = 0; /* debugging only */
}
+void GC_suspend_self() {
+ GC_thread me = GC_lookup_thread(pthread_self());
+ struct timespec t;
+ t.tv_sec = 0;
+ t.tv_nsec = 1000 * 1000 * 50;
+ me -> flags |= SUSPENDED;
+ GC_start_blocking();
+ while (me -> flags & SUSPENDED)
+ nanosleep(&t, NULL);
+ 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()
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: External thread suspension (again)
2006-05-26 21:43 External thread suspension (again) Keith Seitz
@ 2006-06-04 4:47 ` Hans Boehm
2006-06-05 20:43 ` Keith Seitz
0 siblings, 1 reply; 6+ messages in thread
From: Hans Boehm @ 2006-06-04 4:47 UTC (permalink / raw)
To: Keith Seitz; +Cc: gc, Java Patch List
Sorry about the slow resposne.
In general, this also looks like a better approach to me. Thanks.
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?
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.
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.
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.
Hans
On Fri, 26 May 2006, Keith Seitz wrote:
> Including GC list
>
> -------- Original Message --------
> Subject: Re: [RFC] GC: external thread suspension
> Date: Thu, 25 May 2006 12:39:58 -0700
> From: Keith Seitz <keiths@redhat.com>
> To: Java Patch List <java-patches@gcc.gnu.org>
>
> Okay, since it has been relatively quiet for the past week, I thought I
> would turn this into an official RFA.
>
> I'm going to be conservative, and offer up for approval the patch
> suggested by Hans Boehm, using GC_start/end_blocking. It is much simpler
> than my original patch.
>
> In order to modify gc.h, it looks like I need to ifdef stuff. Since I've
> only done this for pthreads, I've made a best guess as to the proper
> conditions. Let me know if there are any problems with this.
>
> Keith
>
> ChangeLog
> 2006-05-25 Keith Seitz <keiths@redhat.com>
>
> * pthread_stop_world.c (GC_suspend_handler): Redirect to
> suspension
> routine if signal is received and thread is flagged SUSPENDED.
> (GC_suspend_self): New function.
> (GC_suspend_thread): New function.
> (GC_resume_thread): New function.
> * include/gc.h (GC_suspend_self): Declare.
> (GC_suspend_thread): Declare.
> (GC_resumet_thread): Declare.
> * include/private/pthread_support.h (SUSPENDED): New GC_thread
> flag.
>
>
>
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: External thread suspension (again)
2006-06-04 4:47 ` Hans Boehm
@ 2006-06-05 20:43 ` Keith Seitz
2006-06-09 20:13 ` Bryce McKinlay
0 siblings, 1 reply; 6+ messages in thread
From: Keith Seitz @ 2006-06-05 20:43 UTC (permalink / raw)
To: Hans Boehm; +Cc: gc, Java Patch List
[-- 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()
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: External thread suspension (again)
2006-06-05 20:43 ` Keith Seitz
@ 2006-06-09 20:13 ` Bryce McKinlay
2006-06-09 21:10 ` Keith Seitz
0 siblings, 1 reply; 6+ messages in thread
From: Bryce McKinlay @ 2006-06-09 20:13 UTC (permalink / raw)
To: Keith Seitz; +Cc: Hans Boehm, gc, Java Patch List
Keith Seitz wrote:
> 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).
This works in the GC_suspend_self() case, but how does GC_start_blocking
get called in the GC_suspend_thread() case? Doesn't GC_suspend_thread()
need to set SUSPENDED before it sends the suspend signal?
Bryce
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: External thread suspension (again)
2006-06-09 20:13 ` Bryce McKinlay
@ 2006-06-09 21:10 ` Keith Seitz
2006-06-09 21:24 ` Keith Seitz
0 siblings, 1 reply; 6+ messages in thread
From: Keith Seitz @ 2006-06-09 21:10 UTC (permalink / raw)
To: Bryce McKinlay; +Cc: Hans Boehm, gc, Java Patch List
Bryce McKinlay wrote:
> Keith Seitz wrote:
>> 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).
>
> This works in the GC_suspend_self() case, but how does GC_start_blocking
> get called in the GC_suspend_thread() case? Doesn't GC_suspend_thread()
> need to set SUSPENDED before it sends the suspend signal?
Doh! I'm an idiot. Talk about not seeing the forest!
I've added that to the latest revision of the patch (which is attached
for completeness).
[Hmm. Now that I see it, I wonder if it would be prudent to add some
sanity checks to GC_suspend_thread, GC_suspend_self, and
GC_resume_thread to make sure the thread is known to the GC, otherwise
lookup_thread could return null...]
Keith
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: External thread suspension (again)
2006-06-09 21:10 ` Keith Seitz
@ 2006-06-09 21:24 ` Keith Seitz
0 siblings, 0 replies; 6+ messages in thread
From: Keith Seitz @ 2006-06-09 21:24 UTC (permalink / raw)
To: Bryce McKinlay; +Cc: gc, Java Patch List
[-- Attachment #1: Type: text/plain, Size: 380 bytes --]
Keith Seitz wrote:
> [Hmm. Now that I see it, I wonder if it would be prudent to add some
> sanity checks to GC_suspend_thread, GC_suspend_self, and
> GC_resume_thread to make sure the thread is known to the GC, otherwise
> lookup_thread could return null...]
Sheesh. I just added this. It can't hurt.
And this time, I've remembered to attach the freakin' patch. :-)
Keith
[-- Attachment #2: external-suspend.patch --]
[-- Type: text/x-patch, Size: 3582 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,43 @@
GC_stopping_thread = 0; /* debugging only */
}
+void GC_suspend_self() {
+ GC_thread me = GC_lookup_thread(pthread_self());
+ if (me == NULL)
+ ABORT("attempting to suspend unknown thread");
+
+ 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);
+ if (t == NULL)
+ ABORT("attempting to suspend unknown thread");
+
+ t -> flags |= SUSPENDED;
+ 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);
+ if (t == NULL)
+ ABORT("attempting to resume unknown thread");
+
+ t -> flags &= ~SUSPENDED;
+}
+
/* Caller holds allocation lock, and has held it continuously since */
/* the world stopped. */
void GC_start_world()
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2006-06-09 21:24 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2006-05-26 21:43 External thread suspension (again) Keith Seitz
2006-06-04 4:47 ` Hans Boehm
2006-06-05 20:43 ` Keith Seitz
2006-06-09 20:13 ` Bryce McKinlay
2006-06-09 21:10 ` Keith Seitz
2006-06-09 21:24 ` Keith Seitz
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).