Code review request: small optimization to localtime.c

[David Xu]

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