Code review request: small optimization to localtime.c

[Alfred Perlstein]

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 at 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