Concurrency Bug in memcached

[dormando]

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