From owner-freebsd-arch@FreeBSD.ORG Mon Dec 3 23:26:16 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 35B0316A421 for ; Mon, 3 Dec 2007 23:26:16 +0000 (UTC) (envelope-from BearPerson@gmx.net) Received: from mail.gmx.net (mail.gmx.net [213.165.64.20]) by mx1.freebsd.org (Postfix) with SMTP id 6C62F13C442 for ; Mon, 3 Dec 2007 23:26:15 +0000 (UTC) (envelope-from BearPerson@gmx.net) Received: (qmail invoked by alias); 03 Dec 2007 22:59:34 -0000 Received: from port-83-236-56-222.dynamic.qsc.de (EHLO gmx.net) [83.236.56.222] by mail.gmx.net (mp003) with SMTP; 03 Dec 2007 23:59:34 +0100 X-Authenticated: #20254835 X-Provags-ID: V01U2FsdGVkX19fs5xarR5ft6qKMWcNaV5HfSQ8za8YJRk/V3ALrj q4EvUqcHYatYOq Date: Mon, 3 Dec 2007 23:59:29 +0100 From: Karsten Behrmann To: freebsd-arch@freebsd.org Message-ID: <20071203235929.685d3674@Karsten.Behrmanns.Kasten> In-Reply-To: <20071128.151021.709401576.imp@bsdimp.com> References: <20071128.151021.709401576.imp@bsdimp.com> X-Mailer: Claws Mail 2.9.2 (GTK+ 2.8.18; i686-pc-linux-gnu) Mime-Version: 1.0 Content-Type: multipart/signed; boundary=Sig_w9H.JONpwy2BbOTeikusyyT; protocol="application/pgp-signature"; micalg=PGP-SHA1 X-Y-GMX-Trusted: 0 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: Mon, 03 Dec 2007 23:26:16 -0000 --Sig_w9H.JONpwy2BbOTeikusyyT Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: quoted-printable Hi there, [ we do ] > pthread_mutex_lock(); > if (!is_set) { > is_set =3D true; > // do something > } > pthread_mutex_unlock(); [couldn't we do] > if (!is_set) { > pthread_mutex_lock(); > if (!is_set) { > is_set =3D 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 =3D 3; b =3D 4; c =3D a * b; is_set =3D 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 =3D TRUE; > + _MUTEX_LOCK(&gmt_mutex); > + if (!gmt_is_set) { > + gmt_is_set =3D TRUE; XXX - at this point, gmt_is_set is TRUE but gmtptr is not initialized yet. > #ifdef ALL_STATE > - gmtptr =3D (struct state *) malloc(sizeof *gmtptr); > - if (gmtptr !=3D NULL) > + gmtptr =3D (struct state *) malloc(sizeof *gmtptr); > + if (gmtptr !=3D NULL) > #endif /* defined ALL_STATE */ > - gmtload(gmtptr); > + gmtload(gmtptr); > + } > + _MUTEX_UNLOCK(&gmt_mutex); } - _MUTEX_UNLOCK(&gmt_mutex); So Far, Karsten Behrmann --Sig_w9H.JONpwy2BbOTeikusyyT Content-Type: application/pgp-signature; name=signature.asc Content-Disposition: attachment; filename=signature.asc -----BEGIN PGP SIGNATURE----- Version: GnuPG v1.4.3 (GNU/Linux) iD8DBQFHVIpVAksKLoO3vywRAj3gAKCTAcmVaXGGUiSa6feF1UveYDbhNACaA8B2 PMVBUfz8oM9Kjv+KHZ/bfxM= =I8Wk -----END PGP SIGNATURE----- --Sig_w9H.JONpwy2BbOTeikusyyT--