Re: [PATCH v2 4/6] x86/time: streamline platform time init on plt_init()

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

On 04/05/2016 12:46 PM, Jan Beulich wrote:
>>>> On 29.03.16 at 15:44, <joao.m.martins@oracle.com> wrote:
>> --- a/xen/arch/x86/time.c
>> +++ b/xen/arch/x86/time.c
>> @@ -434,6 +434,7 @@ uint64_t ns_to_acpi_pm_tick(uint64_t ns)
>>  /************************************************************
>>   * PLATFORM TIMER 4: TSC
>>   */
>> +static bool_t clocksource_is_tsc;
> 
> __read_mostly, but see below.
> 
>> @@ -516,17 +519,31 @@ static s_time_t __read_platform_stime(u64 platform_time)
>>      return (stime_platform_stamp + scale_delta(diff, &plt_scale));
>>  }
>>  
>> +static void __plt_init(void)
>> +{
>> +    u64 count;
>> +
>> +    ASSERT(spin_is_locked(&platform_timer_lock));
>> +    count = plt_src.read_counter();
>> +    plt_stamp64 += (count - plt_stamp) & plt_mask;
>> +    plt_stamp = count;
>> +}
> 
> Note that this has nothing to do with "init" - it updates the two time
> stamps, as is being made clear by ...
> 
>>  static void plt_overflow(void *unused)
>>  {
>>      int i;
>> -    u64 count;
>>      s_time_t now, plt_now, plt_wrap;
>>  
>>      spin_lock_irq(&platform_timer_lock);
>>  
>> -    count = plt_src.read_counter();
>> -    plt_stamp64 += (count - plt_stamp) & plt_mask;
>> -    plt_stamp = count;
>> +    __plt_init();
> 
> ... this use.
> 
Would you prefer changing the name to e.g "set_plt_stamp" ?

>> @@ -621,11 +638,22 @@ static int __init try_platform_timer(struct platform_timesource *pts)
>>  
>>      set_time_scale(&plt_scale, pts->frequency);
>>  
>> -    plt_overflow_period = scale_delta(
>> -        1ull << (pts->counter_bits - 1), &plt_scale);
>> -    init_timer(&plt_overflow_timer, plt_overflow, NULL, 0);
>>      plt_src = *pts;
>> -    plt_overflow(NULL);
>> +
>> +    if ( clocksource_is_tsc )
> 
> Why not simply "if ( pts == plt_tsc )", eliminating the need for the
> variable?
Yeah, good point. I will fix that.

> 
>> +    {
>> +        plt_init();
>> +    }
>> +    else
>> +    {
>> +        plt_overflow_period = scale_delta(
>> +            1ull << (pts->counter_bits - 1), &plt_scale);
>> +        init_timer(&plt_overflow_timer, plt_overflow, NULL, 0);
>> +        plt_overflow(NULL);
>> +
>> +        printk("Platform timer overflow period is %lu secs\n",
>> +               plt_overflow_period/SECONDS(1));
> 
> If we want this logged at all, then please at most as XENLOG_INFO.
OK.

> Plus - is seconds granularity fine grained enough for all sources, i.e.
> wouldn't there for typical HPET just be a single digit, not a lot of
> precision that is?
Could be, my HPET was around 2 minutes overflow period, but PIT was a single
digit as you mention. I will change that to MILLISECS(1000) for higher precision
- or I can remove it entirely if you prefer not logging this info.

> And finally: Blanks around / please.
OK.

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