From mboxrd@z Thu Jan 1 00:00:00 1970 Path: news.gmane.org!not-for-mail From: Neil Jerram Newsgroups: gmane.lisp.guile.devel Subject: Re: Locks and threads Date: Wed, 04 Mar 2009 23:49:20 +0000 Message-ID: <87vdqovofz.fsf@arudy.ossau.uklinux.net> References: <87mycsd2rj.fsf@arudy.ossau.uklinux.net> NNTP-Posting-Host: lo.gmane.org Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="=-=-=" X-Trace: ger.gmane.org 1236210586 27057 80.91.229.12 (4 Mar 2009 23:49:46 GMT) X-Complaints-To: usenet@ger.gmane.org NNTP-Posting-Date: Wed, 4 Mar 2009 23:49:46 +0000 (UTC) Cc: Linas Vepstas To: Guile Development Original-X-From: guile-devel-bounces+guile-devel=m.gmane.org@gnu.org Thu Mar 05 00:51:02 2009 Return-path: Envelope-to: guile-devel@m.gmane.org Original-Received: from lists.gnu.org ([199.232.76.165]) by lo.gmane.org with esmtp (Exim 4.50) id 1Lf0rW-00050U-Uv for guile-devel@m.gmane.org; Thu, 05 Mar 2009 00:50:55 +0100 Original-Received: from localhost ([127.0.0.1]:40824 helo=lists.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1Lf0qB-0001ku-Ji for guile-devel@m.gmane.org; Wed, 04 Mar 2009 18:49:31 -0500 Original-Received: from mailman by lists.gnu.org with tmda-scanned (Exim 4.43) id 1Lf0q5-0001kp-JN for guile-devel@gnu.org; Wed, 04 Mar 2009 18:49:25 -0500 Original-Received: from exim by lists.gnu.org with spam-scanned (Exim 4.43) id 1Lf0q3-0001jL-CB for guile-devel@gnu.org; Wed, 04 Mar 2009 18:49:24 -0500 Original-Received: from [199.232.76.173] (port=37551 helo=monty-python.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1Lf0q3-0001jB-7c for guile-devel@gnu.org; Wed, 04 Mar 2009 18:49:23 -0500 Original-Received: from mail3.uklinux.net ([80.84.72.33]:46549) by monty-python.gnu.org with esmtp (Exim 4.60) (envelope-from ) id 1Lf0q2-0002UP-Go for guile-devel@gnu.org; Wed, 04 Mar 2009 18:49:22 -0500 Original-Received: from arudy (host86-157-180-39.range86-157.btcentralplus.com [86.157.180.39]) by mail3.uklinux.net (Postfix) with ESMTP id 5DE6C1F6E90; Wed, 4 Mar 2009 23:49:21 +0000 (GMT) Original-Received: from arudy.ossau.uklinux.net (arudy [127.0.0.1]) by arudy (Postfix) with ESMTP id 62CAB3801E; Wed, 4 Mar 2009 23:49:20 +0000 (GMT) In-Reply-To: <87mycsd2rj.fsf@arudy.ossau.uklinux.net> (Neil Jerram's message of "Wed\, 11 Feb 2009 22\:31\:28 +0000") User-Agent: Gnus/5.11 (Gnus v5.11) Emacs/22.2 (gnu/linux) X-detected-operating-system: by monty-python.gnu.org: GNU/Linux 2.4-2.6 X-BeenThere: guile-devel@gnu.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: "Developers list for Guile, the GNU extensibility library" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Original-Sender: guile-devel-bounces+guile-devel=m.gmane.org@gnu.org Errors-To: guile-devel-bounces+guile-devel=m.gmane.org@gnu.org Xref: news.gmane.org gmane.lisp.guile.devel:8222 Archived-At: --=-=-= Neil Jerram writes: > - first to address problems reported by helgrind (since I think we > should take advantage of external tools before adding debug code to > Guile internally) Here's another lock ordering fix. (For 1.8.x at least, I haven't checked if it's relevant to master yet.) 1.8.x is then helgrind-clean (apart from one unimportant-looking termination issue), so next it's on to the real problem, threadsafe define. Neil --=-=-= Content-Type: text/x-diff Content-Disposition: inline; filename=0001-Lock-ordering-don-t-allocate-when-in-critical-secti.patch >From 588edc4cd1bfea7d51c9aed463e8119482e7a3f0 Mon Sep 17 00:00:00 2001 From: Neil Jerram Date: Wed, 4 Mar 2009 23:45:11 +0000 Subject: [PATCH] Lock ordering: don't allocate when in critical section (scm_sigaction_for_thread) This fixes the following helgrind report. Thread #1: lock order "0x4114748 before 0x4331084" violated at 0x40234F7: pthread_mutex_lock (hg_intercepts.c:408) by 0x407A55F: deval (eval.c:4077) by 0x407A071: scm_dapply (eval.c:5012) by 0x407888A: scm_apply (eval.c:4811) by 0x407E20C: scm_call_0 (eval.c:4666) by 0x406BDF7: scm_dynamic_wind (dynwind.c:111) by 0x407620C: ceval (eval.c:4571) by 0x407E8D9: scm_primitive_eval_x (eval.c:5921) by 0x407E934: scm_eval_x (eval.c:5956) by 0x40B9140: scm_shell (script.c:737) by 0x4095AC5: invoke_main_func (init.c:367) by 0x4065A81: c_body (continuations.c:349) Required order was established by acquisition of lock at 0x4114748 at 0x40234F7: pthread_mutex_lock (hg_intercepts.c:408) by 0x40B7BE1: scm_sigaction_for_thread (scmsigs.c:339) by 0x40911DE: scm_gsubr_apply (gsubr.c:223) by 0x4079E36: scm_dapply (eval.c:4930) by 0x407AB68: deval (eval.c:4378) by 0x407A071: scm_dapply (eval.c:5012) by 0x407888A: scm_apply (eval.c:4811) by 0x407E1D0: scm_call_1 (eval.c:4672) by 0x407FEEE: scm_map (eval.c:5489) by 0x407BDCC: deval (eval.c:4367) by 0x407D197: deval (eval.c:3698) by 0x407A071: scm_dapply (eval.c:5012) followed by a later acquisition of lock at 0x4331084 at 0x40234F7: pthread_mutex_lock (hg_intercepts.c:408) by 0x40DC397: scm_enter_guile (threads.c:377) by 0x40DC8F4: scm_pthread_mutex_lock (threads.c:1485) by 0x4083FAA: scm_gc_for_newcell (gc.c:484) by 0x40A9781: scm_cons (inline.h:115) by 0x4079867: scm_dapply (eval.c:4850) by 0x407888A: scm_apply (eval.c:4811) by 0x40796D6: scm_call_3 (eval.c:4684) by 0x409A7C2: module_variable (modules.c:302) by 0x409A7EE: module_variable (modules.c:312) by 0x409A962: scm_sym2var (modules.c:466) by 0x40738F4: scm_lookupcar1 (eval.c:2874) * libguile/scmsigs.c (close_1): Renamed `handler_to_async'; also handle #f case and wrapping the async in a cons, if necessary. (install_handler): Pass in async instead of constructing it; combine two branches into one. (scm_sigaction_for_thread): Allocate async upfront instead of inside the critical section, and pass it to install_handler calls. Leave critical section before signaling out-of-range error. --- libguile/scmsigs.c | 53 ++++++++++++++++++++++++++++----------------------- 1 files changed, 29 insertions(+), 24 deletions(-) diff --git a/libguile/scmsigs.c b/libguile/scmsigs.c index 9433273..e15bbf3 100644 --- a/libguile/scmsigs.c +++ b/libguile/scmsigs.c @@ -108,10 +108,20 @@ static SIGRETTYPE (*orig_handlers[NSIG])(int); #endif static SCM -close_1 (SCM proc, SCM arg) +handler_to_async (SCM handler, int signum) { - return scm_primitive_eval_x (scm_list_3 (scm_sym_lambda, SCM_EOL, - scm_list_2 (proc, arg))); + if (scm_is_false (handler)) + return SCM_BOOL_F; + else + { + SCM async = scm_primitive_eval_x (scm_list_3 (scm_sym_lambda, SCM_EOL, + scm_list_2 (handler, + scm_from_int (signum)))); +#if !SCM_USE_PTHREAD_THREADS + async = scm_cons (async, SCM_BOOL_F); +#endif + return async; + } } #if SCM_USE_PTHREAD_THREADS @@ -239,23 +249,10 @@ ensure_signal_delivery_thread () #endif /* !SCM_USE_PTHREAD_THREADS */ static void -install_handler (int signum, SCM thread, SCM handler) +install_handler (int signum, SCM thread, SCM handler, SCM async) { - if (scm_is_false (handler)) - { - SCM_SIMPLE_VECTOR_SET (*signal_handlers, signum, SCM_BOOL_F); - SCM_SIMPLE_VECTOR_SET (signal_handler_asyncs, signum, SCM_BOOL_F); - } - else - { - SCM async = close_1 (handler, scm_from_int (signum)); -#if !SCM_USE_PTHREAD_THREADS - async = scm_cons (async, SCM_BOOL_F); -#endif - SCM_SIMPLE_VECTOR_SET (*signal_handlers, signum, handler); - SCM_SIMPLE_VECTOR_SET (signal_handler_asyncs, signum, async); - } - + SCM_SIMPLE_VECTOR_SET (*signal_handlers, signum, handler); + SCM_SIMPLE_VECTOR_SET (signal_handler_asyncs, signum, async); SCM_SIMPLE_VECTOR_SET (signal_handler_threads, signum, thread); } @@ -308,6 +305,7 @@ SCM_DEFINE (scm_sigaction_for_thread, "sigaction", 1, 3, 0, int save_handler = 0; SCM old_handler; + SCM async; csig = scm_to_signed_integer (signum, 0, NSIG-1); @@ -334,6 +332,10 @@ SCM_DEFINE (scm_sigaction_for_thread, "sigaction", 1, 3, 0, SCM_MISC_ERROR ("thread has already exited", SCM_EOL); } + /* Allocate upfront, as we can't do it inside the critical + section. */ + async = handler_to_async (handler, csig); + ensure_signal_delivery_thread (); SCM_CRITICAL_SECTION_START; @@ -351,10 +353,13 @@ SCM_DEFINE (scm_sigaction_for_thread, "sigaction", 1, 3, 0, #else chandler = (SIGRETTYPE (*) (int)) handler_int; #endif - install_handler (csig, SCM_BOOL_F, SCM_BOOL_F); + install_handler (csig, SCM_BOOL_F, SCM_BOOL_F, async); } else - SCM_OUT_OF_RANGE (2, handler); + { + SCM_CRITICAL_SECTION_END; + SCM_OUT_OF_RANGE (2, handler); + } } else if (scm_is_false (handler)) { @@ -366,7 +371,7 @@ SCM_DEFINE (scm_sigaction_for_thread, "sigaction", 1, 3, 0, { action = orig_handlers[csig]; orig_handlers[csig].sa_handler = SIG_ERR; - install_handler (csig, SCM_BOOL_F, SCM_BOOL_F); + install_handler (csig, SCM_BOOL_F, SCM_BOOL_F, async); } #else if (orig_handlers[csig] == SIG_ERR) @@ -375,7 +380,7 @@ SCM_DEFINE (scm_sigaction_for_thread, "sigaction", 1, 3, 0, { chandler = orig_handlers[csig]; orig_handlers[csig] = SIG_ERR; - install_handler (csig, SCM_BOOL_F, SCM_BOOL_F); + install_handler (csig, SCM_BOOL_F, SCM_BOOL_F, async); } #endif } @@ -391,7 +396,7 @@ SCM_DEFINE (scm_sigaction_for_thread, "sigaction", 1, 3, 0, if (orig_handlers[csig] == SIG_ERR) save_handler = 1; #endif - install_handler (csig, thread, handler); + install_handler (csig, thread, handler, async); } /* XXX - Silently ignore setting handlers for `program error signals' -- 1.5.6.5 --=-=-=--