From owner-freebsd-arch@FreeBSD.ORG Tue Dec 4 02:00:18 2007 Return-Path: Delivered-To: freebsd-arch@hub.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:4f8:fff6::34]) by hub.freebsd.org (Postfix) with ESMTP id 8919816A419 for ; Tue, 4 Dec 2007 02:00:18 +0000 (UTC) (envelope-from davidxu@FreeBSD.org) Received: from freefall.freebsd.org (freefall.freebsd.org [IPv6:2001:4f8:fff6::28]) by mx1.freebsd.org (Postfix) with ESMTP id 7BCFB13C447; Tue, 4 Dec 2007 02:00:18 +0000 (UTC) (envelope-from davidxu@FreeBSD.org) Received: from [127.0.0.1] (root@localhost [127.0.0.1]) by freefall.freebsd.org (8.14.2/8.14.2) with ESMTP id lB420F4q016775; Tue, 4 Dec 2007 02:00:16 GMT (envelope-from davidxu@freebsd.org) Message-ID: <4754B4EC.5040005@freebsd.org> Date: Tue, 04 Dec 2007 10:01:16 +0800 From: David Xu User-Agent: Mozilla/5.0 (X11; U; FreeBSD i386; en-US; rv:1.7.13) Gecko/20070516 X-Accept-Language: en-us, en MIME-Version: 1.0 To: Karsten Behrmann References: <20071128.151021.709401576.imp@bsdimp.com> <20071203235929.685d3674@Karsten.Behrmanns.Kasten> In-Reply-To: <20071203235929.685d3674@Karsten.Behrmanns.Kasten> Content-Type: text/plain; charset=us-ascii; format=flowed Content-Transfer-Encoding: 7bit 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 02:00:18 -0000 Karsten Behrmann 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 You are correct here, I think it should either use pthread_once or use some atomic instructions to implicitly set memory barrieries. The pthread_once function in libthr is already using mutex and aware of memory barrieries. Regards, David Xu