Concurrency Bug in memcached

[Tomash Brechko]

On Mon, Feb 04, 2008 at 17:42:54 -0800, Brian Aker wrote:
>  /* 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).

Could you be more specific on the essence of the problem?  I tried to
think of several scenarios, but don't have any clue.

To my understanding the only bug is that set_current_time() is called
from process_command(), while it should be called only from
clock_handler().  Better remove this function, and embed its body into
clock_handler().

As for your fix, you don't have to make new_time volatile: it's local
to this function, you never update the value, so why would you want to
re-read it from memory every time?

Second, while(! __sync_bool_compare_and_swap()) construct is very
suspicious.  It either may become an infinite loop if second argument
is read only once.  Or, once current_time is volatile, the chance that
&current_time and current_time would be different is very tiny.  But
suppose we hit it.  This would mean current_time is been updated.  But
what the difference would it make then?  The race haven't gone
anywhere, you still may overwrite the future value with the past
value.  And the point is that it's not really important for
current_time to be precise.  Updating it once a second is very
imprecise anyway.


current_time is volatile unsigned int, so you may be pretty much sure
that it will be updated atomically.  Even if the update is not atomic,
__sync_bool_compare_and_swap() may still return true _in the middle_
of concurrent update (for instance, other thread updates the value, it
stored one half of it, but that half happened to be the same as the
old part, so the value looks unmodified).


Perhaps I'm not getting something.


-- 
   Tomash Brechko