From mboxrd@z Thu Jan 1 00:00:00 1970 Path: news.gmane.org!not-for-mail From: ludo@gnu.org (Ludovic =?utf-8?Q?Court=C3=A8s?=) Newsgroups: gmane.lisp.guile.devel Subject: Re: [PATCH] Make guardians and (ice-9 popen) thread-safe Date: Sat, 23 Nov 2013 16:06:11 +0100 Message-ID: <877gbzyx7g.fsf@gnu.org> References: <877gc66xw9.fsf@netris.org> NNTP-Posting-Host: plane.gmane.org Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 8bit X-Trace: ger.gmane.org 1385219202 29459 80.91.229.3 (23 Nov 2013 15:06:42 GMT) X-Complaints-To: usenet@ger.gmane.org NNTP-Posting-Date: Sat, 23 Nov 2013 15:06:42 +0000 (UTC) To: guile-devel@gnu.org Original-X-From: guile-devel-bounces+guile-devel=m.gmane.org@gnu.org Sat Nov 23 16:06:47 2013 Return-path: Envelope-to: guile-devel@m.gmane.org Original-Received: from lists.gnu.org ([208.118.235.17]) by plane.gmane.org with esmtp (Exim 4.69) (envelope-from ) id 1VkEnH-0001zQ-4R for guile-devel@m.gmane.org; Sat, 23 Nov 2013 16:06:47 +0100 Original-Received: from localhost ([::1]:44011 helo=lists.gnu.org) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1VkEnG-0000ps-LB for guile-devel@m.gmane.org; Sat, 23 Nov 2013 10:06:46 -0500 Original-Received: from eggs.gnu.org ([2001:4830:134:3::10]:48027) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1VkEn5-0000pk-SA for guile-devel@gnu.org; Sat, 23 Nov 2013 10:06:41 -0500 Original-Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1VkEn0-0000Mx-74 for guile-devel@gnu.org; Sat, 23 Nov 2013 10:06:35 -0500 Original-Received: from plane.gmane.org ([80.91.229.3]:49455) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1VkEmz-0000Mq-Si for guile-devel@gnu.org; Sat, 23 Nov 2013 10:06:30 -0500 Original-Received: from list by plane.gmane.org with local (Exim 4.69) (envelope-from ) id 1VkEmu-0001qS-6D for guile-devel@gnu.org; Sat, 23 Nov 2013 16:06:24 +0100 Original-Received: from reverse-83.fdn.fr ([80.67.176.83]) by main.gmane.org with esmtp (Gmexim 0.1 (Debian)) id 1AlnuQ-0007hv-00 for ; Sat, 23 Nov 2013 16:06:24 +0100 Original-Received: from ludo by reverse-83.fdn.fr with local (Gmexim 0.1 (Debian)) id 1AlnuQ-0007hv-00 for ; Sat, 23 Nov 2013 16:06:24 +0100 X-Injected-Via-Gmane: http://gmane.org/ Original-Lines: 158 Original-X-Complaints-To: usenet@ger.gmane.org X-Gmane-NNTP-Posting-Host: reverse-83.fdn.fr X-URL: http://www.fdn.fr/~lcourtes/ X-Revolutionary-Date: 3 Frimaire an 222 de la =?utf-8?Q?R=C3=A9volution?= X-PGP-Key-ID: 0xEA52ECF4 X-PGP-Key: http://www.fdn.fr/~lcourtes/ludovic.asc X-PGP-Fingerprint: 83C4 F8E5 10A3 3B4C 5BEA D15D 77DD 95E2 EA52 ECF4 X-OS: x86_64-unknown-linux-gnu User-Agent: Gnus/5.130007 (Ma Gnus v0.7) Emacs/24.3 (gnu/linux) Cancel-Lock: sha1:jVayfN2YLubexAEQg7rLgQagLQM= X-detected-operating-system: by eggs.gnu.org: Genre and OS details not recognized. X-Received-From: 80.91.229.3 X-BeenThere: guile-devel@gnu.org X-Mailman-Version: 2.1.14 Precedence: list List-Id: "Developers list for Guile, the GNU extensibility library" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: guile-devel-bounces+guile-devel=m.gmane.org@gnu.org Original-Sender: guile-devel-bounces+guile-devel=m.gmane.org@gnu.org Xref: news.gmane.org gmane.lisp.guile.devel:16760 Archived-At: Hi! (I just realized when I finished my message that I was too late; sorry about that! Anyway, sending the few remaining questions I have.) Mark H Weaver skribis: > Here's a set of patches for stable-2.0 that make (ice-9 popen) and > guardians thread-safe. These patches fix . > (please disregard the preliminary patches I posted in that bug report). > > These patches also lay the groundwork for conveniently blocking asyncs > while mutexes are held, to prevent the deadlocks that can arise when > asyncs are run while a mutex is held, and one of the asyncs runs code > that tries to lock the same mutex, as discussed here: > > http://lists.gnu.org/archive/html/guile-devel/2013-08/msg00025.html > http://lists.gnu.org/archive/html/guile-devel/2013-08/msg00030.html > > In this patch set, I only block asyncs in two places: when the guardian > mutex is held, and when the 'overrides_lock' is held in procprop.scm, > which has caused deadlocks for me in practice. However, in the future > I'd like to make a more comprehensive effort to find other places where > this should be done. As already discussed, this looks like the Right Thing to me. > From e4b90f498c3141ba203a6359a8a0cfe790324203 Mon Sep 17 00:00:00 2001 > From: Mark H Weaver > Date: Sun, 17 Nov 2013 04:00:29 -0500 > Subject: [PATCH 1/6] Add mutex locking functions that also block asyncs. > > * libguile/async.h (scm_i_pthread_mutex_lock_with_asyncs, > scm_i_pthread_mutex_unlock_with_asyncs): New macros. > > * libguile/threads.c (do_unlock_with_asyncs): New static helper. > (scm_i_dynwind_pthread_mutex_lock_with_asyncs): New function. > > * libguile/threads.h (scm_i_dynwind_pthread_mutex_lock_with_asyncs): > Add prototype. OK, but: > +# define scm_i_pthread_mutex_lock_with_asyncs(m) \ > + do \ > + { \ > + SCM_I_CURRENT_THREAD->block_asyncs++; \ > + scm_i_pthread_mutex_lock (m); \ > + } \ > + while (0) I find the name possibly confusing. What about ‘scm_i_pthread_mutex_lock_block_asyncs’ and ‘scm_i_pthread_mutex_unlock_unlock_asyncs’? > +static void > +do_unlock_with_asyncs (void *data) > +{ > + scm_i_pthread_mutex_unlock ((scm_i_pthread_mutex_t *)data); > + SCM_I_CURRENT_THREAD->block_asyncs--; > +} > + > +void > +scm_i_dynwind_pthread_mutex_lock_with_asyncs (scm_i_pthread_mutex_t *mutex) Likewise. > From 13fcd175bc31b5df400dd518836bdce32f32206a Mon Sep 17 00:00:00 2001 > From: Mark H Weaver > Date: Sun, 17 Nov 2013 03:19:32 -0500 > Subject: [PATCH 2/6] Block system asyncs while 'overrides_lock' is held. > > * libguile/procprop.c (scm_set_procedure_property_x): Block system > asyncs while overrides_lock is held. Use dynwind block in case > an exception is thrown. [...] > - scm_i_pthread_mutex_lock (&overrides_lock); > + scm_dynwind_begin (0); > + scm_i_dynwind_pthread_mutex_lock_with_asyncs (&overrides_lock); > props = scm_hashq_ref (overrides, proc, SCM_BOOL_F); Could you recap why asyncs must be blocked here, and add it as a comment? I would think that there’s no way ‘scm_hashq_ref’ can trigger an async run, because it doesn’t call out to Scheme, does it? > From 73d674d1983e7777f22d43335a1834bf26606494 Mon Sep 17 00:00:00 2001 > From: Mark H Weaver > Date: Sun, 17 Nov 2013 03:35:09 -0500 > Subject: [PATCH 3/6] Make guardians thread-safe. > > * libguile/guardians.c (t_guardian): Add mutex. > (finalize_guarded, scm_i_guard, scm_i_get_one_zombie): Lock mutex and > block system asyncs during critical sections. > (scm_make_guardian): Initialize mutex. OK. > From f89b0a7adf9260fee39dd82e756edfabcdf1a668 Mon Sep 17 00:00:00 2001 > From: Mark H Weaver > Date: Sun, 17 Nov 2013 01:11:57 -0500 > Subject: [PATCH 4/6] Make port alists accessible from Scheme. > > * libguile/ports.c (scm_i_port_alist, scm_i_set_port_alist_x): Make > these available from Scheme, as '%port-alist' and '%set-port-alist!'. > Validate port argument. > > * libguile/ports.h (scm_i_set_port_alist_x): Change return type from > 'void' to 'SCM'. OK, but perhaps ‘%port-properties’ and ‘scm_i_port_properties’? (I know the field has been called ‘alist’ from some time, but now that it is somewhat visible from Scheme, names matter more IMO.) Also, I wonder if this should rather be exposed as ‘%port-property KEY’ and ‘%set-port-property! PORT KEY VALUE’. Those two procedures could eventually do any locking required. > From b53342d28a0ec0844373c1469d5f56d4cb6d98fc Mon Sep 17 00:00:00 2001 > From: Mark H Weaver > Date: Sun, 17 Nov 2013 02:46:08 -0500 > Subject: [PATCH 5/6] Stylistic improvements for (ice-9 popen). > > * module/ice-9/popen.scm (close-process, close-process-quietly): Accept > 'port' and 'pid' as separate arguments. Improve style. > (close-pipe, read-pipes): Improve style. OK. > From 3c3e66b85d294fc5ded6f6660a0efe00ed6519b9 Mon Sep 17 00:00:00 2001 > From: Mark H Weaver > Date: Sun, 17 Nov 2013 02:54:31 -0500 > Subject: [PATCH 6/6] Make (ice-9 popen) thread-safe. > > * module/ice-9/popen.scm: Import (ice-9 threads). > (port/pid-table): Mark as deprecated in comment. > (port/pid-table-mutex): New variable. > (open-pipe*): Store the pid in the port's alist. Guard the alist > entry instead of the port. Lock 'port/pid-table-mutex' while mutating > 'port/pid-table'. > (fetch-pid): Removed. > (fetch-alist-entry): New procedure. > (close-process-quietly): Removed. > (close-pipe): Use 'fetch-alist-entry' instead of 'fetch-pid'. Clear > the cdr of the alist entry. Improve error messages. > (reap-pipes): Adapt to the fact that the alist entries are now guarded > instead of the ports. Incorporate the 'waitpid' code that was > previously in 'close-process-quietly', but let the port finalizer > close the port. Clear the cdr of the alist entry. OK. Thanks! Ludo’.