Date: Mon, 3 Dec 2007 23:59:29 +0100 From: Karsten Behrmann <BearPerson@gmx.net> To: freebsd-arch@freebsd.org Subject: Re: Code review request: small optimization to localtime.c Message-ID: <20071203235929.685d3674@Karsten.Behrmanns.Kasten> In-Reply-To: <20071128.151021.709401576.imp@bsdimp.com> References: <20071128.151021.709401576.imp@bsdimp.com>
next in thread | previous in thread | raw e-mail | index | archive | help
--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--
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20071203235929.685d3674>