From mboxrd@z Thu Jan 1 00:00:00 1970 Path: news.gmane.org!not-for-mail From: Andy Wingo Newsgroups: gmane.lisp.guile.devel Subject: Re: thread safe functions Date: Thu, 10 Feb 2011 23:19:17 +0100 Message-ID: References: <20100805112743.GA1671@securactive.net> <8C8EEFE6-77CA-42E7-A2FB-9CEF4E83CDFF@raeburn.org> <5105E811-5A0E-4D7B-9A4A-D3DABB12406F@raeburn.org> NNTP-Posting-Host: lo.gmane.org Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii X-Trace: dough.gmane.org 1297376071 9563 80.91.229.12 (10 Feb 2011 22:14:31 GMT) X-Complaints-To: usenet@dough.gmane.org NNTP-Posting-Date: Thu, 10 Feb 2011 22:14:31 +0000 (UTC) Cc: guile-devel Development To: Ken Raeburn Original-X-From: guile-devel-bounces+guile-devel=m.gmane.org@gnu.org Thu Feb 10 23:14:25 2011 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.69) (envelope-from ) id 1PnemP-0006Uo-0t for guile-devel@m.gmane.org; Thu, 10 Feb 2011 23:14:25 +0100 Original-Received: from localhost ([127.0.0.1]:46960 helo=lists.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1PnemO-0003By-E8 for guile-devel@m.gmane.org; Thu, 10 Feb 2011 17:14:24 -0500 Original-Received: from [140.186.70.92] (port=50443 helo=eggs.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1PnemK-0003Bj-NU for guile-devel@gnu.org; Thu, 10 Feb 2011 17:14:21 -0500 Original-Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1PnemI-0007Cg-Ug for guile-devel@gnu.org; Thu, 10 Feb 2011 17:14:20 -0500 Original-Received: from a-pb-sasl-sd.pobox.com ([64.74.157.62]:52185 helo=sasl.smtp.pobox.com) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1PnemI-0007Cc-QI for guile-devel@gnu.org; Thu, 10 Feb 2011 17:14:18 -0500 Original-Received: from sasl.smtp.pobox.com (unknown [127.0.0.1]) by a-pb-sasl-sd.pobox.com (Postfix) with ESMTP id 32D564D81; Thu, 10 Feb 2011 17:15:18 -0500 (EST) DKIM-Signature: v=1; a=rsa-sha1; c=relaxed; d=pobox.com; h=from:to:cc :subject:references:date:in-reply-to:message-id:mime-version :content-type; s=sasl; bh=jmknvjaVhuVzK2MqTvRxTOOZiiU=; b=HT1FON 28sylL2VgS8mwSu/gTBIhJ+W8U2NV5KZD5xJDRj6KrJN8GQqSMVFwGpbRdp9VlhS zvvy09b086bULRW39Dh3dXeNdCgGtgFvrj1ymY0zvbgnAIqLVZY2iGQIwahUzAt4 Pzu6ogu6bEo4sLb9KA2s2xIu/g7/25rV59+d8= DomainKey-Signature: a=rsa-sha1; c=nofws; d=pobox.com; h=from:to:cc :subject:references:date:in-reply-to:message-id:mime-version :content-type; q=dns; s=sasl; b=qFTfoxblU955hMvroOFuQpx8YG+2RVup ocz1GnbjiVXolgddLJ3SqGoBIAQX79uYbXtVpK7gaAdmEc38jkGbK1E6jKsDdIw5 UG4xrmIDCgPsSF/to1fR85ez0fgPSQ1Yb/CxaIcgMtAhUhluYwfG6CafXtLDmB8i gASSGLjoU9M= Original-Received: from a-pb-sasl-sd.pobox.com (unknown [127.0.0.1]) by a-pb-sasl-sd.pobox.com (Postfix) with ESMTP id 1FBE74D80; Thu, 10 Feb 2011 17:15:17 -0500 (EST) Original-Received: from unquote.localdomain (unknown [90.164.198.39]) (using TLSv1 with cipher DHE-RSA-AES256-SHA (256/256 bits)) (No client certificate requested) by a-pb-sasl-sd.pobox.com (Postfix) with ESMTPSA id 0E4494D7C; Thu, 10 Feb 2011 17:15:14 -0500 (EST) In-Reply-To: <5105E811-5A0E-4D7B-9A4A-D3DABB12406F@raeburn.org> (Ken Raeburn's message of "Sat, 28 Aug 2010 21:33:25 -0400") User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/23.2 (gnu/linux) X-Pobox-Relay-ID: 3D7F56C2-3563-11E0-9C3C-AF401E47CF6F-02397024!a-pb-sasl-sd.pobox.com X-detected-operating-system: by eggs.gnu.org: Solaris 10 (beta) X-Received-From: 64.74.157.62 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:11549 Archived-At: Hey Ken, I got tired of seeing your mail marked as starred in my inbox, so I decided to take a look at it ;-) You were writing about threadsafe access to global variables. On Sun 29 Aug 2010 03:33, Ken Raeburn writes: > looking at deprecation.c, I'd suggest not printing stuff while the > lock is held Fixed, thanks. > I was starting to look through the port code for this, but got > distracted by scm_ptobs, a pointer which appears to be dynamically > reallocated as needed when scm_make_port_type is called. Which means > every function that reads it should be locking the same mutex used when > updating it; in this case, that appears to be the generic "critical > section" stuff, and it looks like we're not doing that. Yeah, and it is exported to users of libguile. Fun, no? I added a comment to the header about its impending deprecation, but that's all I have time for right now. > foreign.c: pointer_weak_refs is a weakly keyed hash table, which we add > things to without locking. Fixed. > instructions.c: Can scm_lookup_instruction_by_name be called the "first" > time from two threads concurrently? Yes; fixed. > modules.c: scm_pre_modules_obarray doesn't look protected. Doesn't need to be; added a comment to that effect. > ports.c: scm_i_port_weak_hash is protected in some places, others I > haven't investigated further. But in scm_c_port_for_each, it looks to > me as though, if ports are being created while the function is setting > up, you could end up raising an exception if scm_internal_hash_fold > iterates over more ports than were in the hash table when the count was > initially taken (and the vector size set). Fixed scm_c_port_for_each, and access to scm_i_port_weak_hash. But the scm_ptobs situation is as unresolved as ever. > procproc.c: There's a mutex to protect overrides, but it looks like > set-procedure-property! doesn't use it correctly; it should look more > like set-object-property! does. I'm going to punt on this one, since it cannot access the hash table in an inconsistent state, and I don't care enough about what happens when multiple threads start changing the same procedure property (a fairly legacy interface) at once. Furthermore there is always (assq-set! (procedure-properties foo) 'bar) to contend with... > properties.c: I'm not familiar with this code at all, but I'd guess > properties_whash should be protected. Though I wonder if the call-out > from primitive-property-ref could invoke other primitive-property > functions, causing a deadlock if it isn't done carefully. I went ahead and deprecated this whole mess, it was only used for object properties, which I reimplemented using weak hash tables and a lock. > symbols.c: I don't think 'symbols' is handled safely. But this code is > all starting to run together in my mind. :-) I think I fixed this one a month ago or so. > smob.c: I don't think tramp_weak_map is adequately protected, but I'm > not certain. > > srcprop.c: scm_source_whash isn't protected; it also appears to be > exposed by the name "source-whash" to Scheme code, which wouldn't be > able to use a mutex defined and used purely in the C code. > > (What's that, four different kinds of property-list mappings, all > implemented separately?) Tell me about it... > struct.c: Even calling struct-vtable-name can cause a hash table entry > to be created, it appears. So that's not thread-safe, never mind the > call to actually change the name. > > weaks.c: Is only making things for Scheme code, where we're making > locking the user's problem. At this point I got tired. But sending this mail to the list will requeue this work, so hey... > It would be a bit easier if hash tables themselves were thread-safe. A > read/write lock might make more sense than a simple mutex though, if > reading is more common than writing. In my experience they sometimes > perform worse than simple mutexes, so they're a win mainly for > concurrent reading. Yeah, that might be a good idea. For later, though. > Of course, hash tables aren't the only places where we can have > problems, and I'm not even touching on hash tables that might be created > by Scheme code (but exposed to users with APIs that don't scream "unsafe > hash table used here!"), but I've already spent my afternoon and evening > on this. Indeed, and I just spent some hours on it too... >>> And don't get me going on memory ordering models.... >> >> I'm interested! Thanks for the notes on this, it was an interesting read. > A real review should check: > * any non-const static-storage variables defined in the library, and > how they're used; > * any functions modifying objects, to determine the effect of > concurrent calls; > * any uses of those functions or objects from our C or Scheme code, to > see if we're presenting the correct semantics in the face of concurrent > calls; > * that we actually specify the semantics, or when results are > undefined, clearly; > * anything dealing with OS interfaces (or other library APIs) that > aren't thread-safe; > And if those aren't going to take long enough already: > * getting *real* answers to the memory ordering issues; > * possibly reviewing every function manipulating memory for ordering > issues; > * probably more. > > Then, I might have some confidence that "things are mostly there." I > don't think it's going to happen at all for 2.0, and I don't expect to > have time myself to do anything more than little bits of work on the > first part of the list, if even that much, in the near future. Indeed, it certainly won't all happen for 2.0. Maybe during 2.0 and changes for 2.2. Keep the reviews coming, though... Cheers, Andy -- http://wingolog.org/