Re: [PATCH v2 5/6] x86/time: refactor read_platform_stime()

[Joao Martins <joao.m.martins@oracle.com>]

On 04/05/2016 12:52 PM, Jan Beulich wrote:
>>>> On 29.03.16 at 15:44, <joao.m.martins@oracle.com> wrote:
>> -static s_time_t read_platform_stime(void)
>> +static s_time_t read_platform_stime(u64 *stamp)
>>  {
>> -    u64 count;
>> +    u64 plt_stamp_counter, count;
> 
> "stamp" and "counter" seem kind of redundant.
> 
A bit, perhaps you prefer the latter? There was a variable named "count", so I
named "stamp" for clearer distinction between the variables and the output arg.

>>      s_time_t stime;
>>  
>>      ASSERT(!local_irq_is_enabled());
>>  
>>      spin_lock(&platform_timer_lock);
>> -    count = plt_stamp64 + ((plt_src.read_counter() - plt_stamp) & plt_mask);
>> +    plt_stamp_counter = plt_src.read_counter();
>> +    count = plt_stamp64 + ((plt_stamp_counter - plt_stamp) & plt_mask);
>>      stime = __read_platform_stime(count);
>> +    if ( stamp )
>> +        *stamp = plt_stamp_counter;
>>      spin_unlock(&platform_timer_lock);
> 
> What reason is there to do that conditional write inside the locked
> region?
> 
None, I should move this conditional write out of this region.

Joao

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel