Code review request: small optimization to localtime.c

[Karsten Behrmann]

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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 189 bytes
Desc: not available
Url : http://lists.freebsd.org/pipermail/freebsd-arch/attachments/20071203/3fb91506/signature.pgp