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 04:22 PM, Jan Beulich wrote:
>>>> On 05.04.16 at 17:12, <joao.m.martins@oracle.com> wrote:
>> On 04/05/2016 12:46 PM, Jan Beulich wrote:
>>>>>> On 29.03.16 at 15:44, <joao.m.martins@oracle.com> wrote:
>>>> @@ -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" ?
> 
> Or simply plt_update()?
Sounds better indeed.

> 
>>>> +    {
>>>> +        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
> 
> How is MILLISECS(1000) different from SECONDS(1)?
Sorry, It's not - I meant MILLISECS(1).

> 
>> - or I can remove it entirely if you prefer not logging this info.
> 
> Well, there had been times where this information would have been
> quite useful in diagnosing problems. That's been a while back, but
> knowing we had such issues I can't just say "drop the message",
> even if I hope we won't have any similar problems anymore.
I will keep it then - until further notice.

Joao

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