Concurrency Bug in memcached

[Brian Aker]

Hi!

We need an atomic portable library. This only works for gcc and the  
gcc in OSX is tool old. I am keeping it in my tree, but mainly for my  
pedantic needs.

Cheers,
	-Brian

On Feb 18, 2008, at 3:49 PM, dormando wrote:

> Has anyone else tested this patch?
>
> Does it work with ancient gcc? Anyone from the solaris camp want to  
> comment?
>
> I might save this for the next cycle, because:
>
> 1) Failure causes the old value to be read until the CPU cache is  
> synced
> completely. Not a huge deal. We'll get way more benefits changing some
> of the more inane mutexes into atomic CAS.
> 2) Potential compatibility breakage. Once I have buildbot up and  
> running
> we can try pushing code like this around and see what it pisses off :)
> 3) Not entirely sure the while loop is right? Must research...
>
> -Dormando
>
> Brian Aker wrote:
>> Hi!
>>
>> Here is a patch to fix the problem with concurrency in the
>> set_current_time() call.
>>
>> diff -r 1d3a59c1dbd3 memcached.c
>> --- a/memcached.c    Mon Feb 04 15:07:29 2008 -0800
>> +++ b/memcached.c    Mon Feb 04 17:33:21 2008 -0800
>> @@ -2560,7 +2560,9 @@ static struct event clockevent;
>>
>> /* time-sensitive callers can call it by hand with this, outside the
>> normal ever-1-second timer */
>> static void set_current_time(void) {
>> -    current_time = (rel_time_t) (time(0) - stats.started);
>> +    volatile rel_time_t new_time;
>> +    new_time = (rel_time_t) (time(0) - stats.started);
>> +   while(!( __sync_bool_compare_and_swap(&current_time,  
>> current_time,
>> new_time)));
>> }
>>
>> The problem will be experienced when you are on a dual processor  
>> machine
>> (aka not a dual core (for most vendors) just a dual processor). The
>> volatile declaration only helps with caching, it does not solve the
>> problem where one processor will see one thing, while another will  
>> see a
>> different value during the setting of the variable.
>>
>> One note about the above, I did not use a mutex since this variable
>> would then have to be locked in all sections of the code (which  
>> would be
>> slow...). This is a gcc specific fix BTW. If others are interested we
>> can put together an include file for different platforms.
>>
>> Cheers,
>>    -Brian
>>
>> -- 
>> _______________________________________________________
>> Brian "Krow" Aker, brian at tangent.org
>> Seattle, Washington
>> http://krow.net/                     <-- Me
>> http://tangent.org/                <-- Software
>> http://exploitseattle.com/    <-- Fun
>> _______________________________________________________
>> You can't grep a dead tree.
>>
>>

--
_______________________________________________________
Brian "Krow" Aker, brian at tangent.org
Seattle, Washington
http://krow.net/                     <-- Me
http://tangent.org/                <-- Software
http://exploitseattle.com/    <-- Fun
_______________________________________________________
You can't grep a dead tree.