From owner-freebsd-arch@FreeBSD.ORG Tue Dec 4 01:46:24 2007 Return-Path: Delivered-To: freebsd-arch@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:4f8:fff6::34]) by hub.freebsd.org (Postfix) with ESMTP id 5DDD716A418 for ; Tue, 4 Dec 2007 01:46:24 +0000 (UTC) (envelope-from bright@elvis.mu.org) Received: from elvis.mu.org (elvis.mu.org [192.203.228.196]) by mx1.freebsd.org (Postfix) with ESMTP id 52D9C13C457 for ; Tue, 4 Dec 2007 01:46:24 +0000 (UTC) (envelope-from bright@elvis.mu.org) Received: by elvis.mu.org (Postfix, from userid 1192) id 621751A4D7E; Mon, 3 Dec 2007 17:46:14 -0800 (PST) Date: Mon, 3 Dec 2007 17:46:14 -0800 From: Alfred Perlstein To: Karsten Behrmann Message-ID: <20071204014614.GE76623@elvis.mu.org> References: <20071128.151021.709401576.imp@bsdimp.com> <20071203235929.685d3674@Karsten.Behrmanns.Kasten> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20071203235929.685d3674@Karsten.Behrmanns.Kasten> User-Agent: Mutt/1.4.2.3i Cc: freebsd-arch@freebsd.org Subject: Re: Code review request: small optimization to localtime.c X-BeenThere: freebsd-arch@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: Discussion related to FreeBSD architecture List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 04 Dec 2007 01:46:24 -0000 Karsten, _typically_ (but not always) an "unlock" operation requires that writes prior to the unlock be globally visible. This is why it works almost everywhere. * Karsten Behrmann [071203 15:26] wrote: > Hi there, > > [ we do ] > > > pthread_mutex_lock(); > > if (!is_set) { > > is_set = true; > > // do something > > } > > pthread_mutex_unlock(); > > [couldn't we do] > > > if (!is_set) { > > pthread_mutex_lock(); > > if (!is_set) { > > is_set = true; > > // do something > > } > > pthread_mutex_unlock(); > > } > > Ah, the old "double-checked locking" thing. Take what I say here > with a grain of salt: I'm a plain programmer, not very versed in > different architectures, so I just pick up what I hear. > > essentially, the problem with the scheme is that plain variable > writes may not be ordered as expected, making them unsuitable to > communicate between threads and processors, depending on compiler > and architecture. > > Let's take the example > > pthread_mutex_lock(); > if (!is_set) { > a = 3; > b = 4; > c = a * b; > is_set = true; > } > pthread_mutex_unlock(); > > Now, an optimizing compiler might put a and b into registers, > compute a * b while storing a 1 to is_set, and then store a, b, and > c from registers into memory. As I said, I'm not actually sure > where compilers are allowed to do this. > > Also, on some architectures, the caching structure may cause writes > to is_set to show up earlier on another CPU than writes to other > parts of memory, if you're unlucky. > > So the bottom line is, in some situations, writes to memory don't > neccessarily take effect everywhere in the same order as the code > would suggest, breaking double-checked locking. is_set could end > up getting set before the "something" part has been executed. > You need the synchronization to ensure consistency of data before > being sure your checks do what you think they do. > > In fact, in your patch, you sometimes actually set the variable > getting checked before setting the data it is supposed to protect ;-) > > > - _MUTEX_LOCK(&gmt_mutex); > > if (!gmt_is_set) { > > - gmt_is_set = TRUE; > > + _MUTEX_LOCK(&gmt_mutex); > > + if (!gmt_is_set) { > > + gmt_is_set = TRUE; > > XXX - at this point, gmt_is_set is TRUE but gmtptr is not initialized yet. > > > #ifdef ALL_STATE > > - gmtptr = (struct state *) malloc(sizeof *gmtptr); > > - if (gmtptr != NULL) > > + gmtptr = (struct state *) malloc(sizeof *gmtptr); > > + if (gmtptr != NULL) > > #endif /* defined ALL_STATE */ > > - gmtload(gmtptr); > > + gmtload(gmtptr); > > + } > > + _MUTEX_UNLOCK(&gmt_mutex); > } > - _MUTEX_UNLOCK(&gmt_mutex); > > > So Far, > Karsten Behrmann -- - Alfred Perlstein