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