Re: [Xen-devel] [PATCH] x86/hvm/pmtimer: improving scalability of virtual time update on Xen 4.0.0

[Keir Fraser <keir@xxxxxxx>]

On 17/11/2010 14:47, "Tim Deegan" <Tim.Deegan@xxxxxxxxxx> wrote:

>>      /* Update the timer */
>>      curr_gtime = hvm_get_guest_time(s->vcpu);
>> -    s->pm.tmr_val += ((curr_gtime - s->last_gtime) * s->scale) >> 32;
>> +    *(volatile uint32_t *)&s->pm.tmr_val = s->pm.tmr_val + (((curr_gtime -
>> s->last_gtime) * s->scale) >> 32);
>>      s->pm.tmr_val &= TMR_VAL_MASK;
>>      s->last_gtime = curr_gtime;
> 
> That doesn't make it an atomic update!  The variable is still written
> to twice. :)  You need to calculate the new tmr_val in a scratch
> variable, and then write it back once at the end of the function.
> (And no 'volatile' wll be necessary).

Can we be sure the compiler will emit only one write to the shared field
unless we use volatile? Seems to safer and more explicit to keep it, to me
(like atomic_set()). Otherwise compiler could elide the local variable and
continue to do the whole composite operation on the shared field?

Of course I agree about calculating the value to store in a local variabne
and then write the result in one operation.

 -- Keir



_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel