Re: [PATCH v3 2/6] x86/time: implement tsc as clocksource

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

On 08/30/2016 01:30 PM, Jan Beulich wrote:
>>>> On 30.08.16 at 14:08, <joao.m.martins@oracle.com> wrote:
>> On 08/29/2016 10:36 AM, Jan Beulich wrote:
>>>>>> On 26.08.16 at 17:11, <joao.m.martins@oracle.com> wrote:
>>>> On 08/25/2016 11:06 AM, Jan Beulich wrote:
>>>>>>>> On 24.08.16 at 14:43, <joao.m.martins@oracle.com> wrote:
>>>>>>  - Change init_tsctimer to skip all the tests and assume it's called
>>>>>>  only on reliable TSC conditions and no warps observed. Tidy
>>>>>>  initialization on verify_tsc_reliability as suggested by Konrad.
>>>>>
>>>>> Which means that contrary to what you say in the cover letter
>>>>> there's nothing to prevent it from being used when CPU hotplug
>>>>> is possible.
>>>> Probably the cover letter wasn't completely clear on this. I mention that I split it
>>>> between Patch 2 and 5 (intent was for easier review), and you can see that I add
>>>> check in patch 5, plus preventing online of CPUs too.
>>>>
>>>>> I don't think you should skip basic sanity checks, and
>>>>> place entirely on the admin the burden of knowing whether the
>>>>> option is safe to use.
>>>> Neither do I think it should be skipped. We mainly don't duplicate these checks. In
>>>> the end I am just running init_tsctimer, in the TSC_RELIABLE block and if no warps
>>>> are observed. So putting that in init_tsctimer would just duplicate the previously
>>>> done checks. Or would you still prefer as done in previous version i.e. all checks in
>>>> both init_tsctimer and verify_tsc_reliability?
>>>
>>> I'm not sure they're needed in both places; what you need to make
>>> certain is that they're reliably gone through, and that this happens
>>> early enough.
>> They are reliably gone through and we get to avoid duplication of checks. Unless
>> there's a preference to re-add these checks in init_tsctimer, I'll keep these as is.
>> verify_tsc_reliability(...) needs to perform this checks and init_tsctimer is only
>> called these reliable TSC conditions.
> 
> But please make sure there's a comment in (or ahead of)
> init_tsctimer() pointing out where the apparently missing checks
> are. This will help both review and future readers.
Okay, I'll add a comment in init_tsctimer().

>>>>>> @@ -1528,6 +1607,7 @@ void __init early_time_init(void)
>>>>>>  
>>>>>>      preinit_pit();
>>>>>>      tmp = init_platform_timer();
>>>>>> +    plt_tsc.frequency = tmp;
>>>>>
>>>>> How can this be the TSC frequency? init_platform_timer()
>>>>> specifically skips the TSC. And I can see no other place where
>>>>> plt_tsc.frequency gets initialized. Am I overlooking something?
>>>>>
>>>> That's the calibrated TSC frequency. Before I was setting pts->frequency in
>>>> init_tsctimer through a temporary variable called tsc_freq. So I thought I could just
>>>> drop the variable and set plt_tsc directly. The difference though from previous
>>>> versions is that since commit 9334029 this value is returned from platform time
>>>> source init() and calibrated against platform timer, instead of always against PIT.
>>>
>>> This doesn't seem to answer my primary question: Where does
>>> plt_tsc.frequency get initialized now? try_platform_timer() and
>>> init_platform_timer() don't - PIT and ACPI timer use static
>>> initializers, and HEPT gets taken care of in init_hpet(), and hence so
>>> should init_tsctimer() do (instead of just returning this apparently
>>> never initialized value). And that's unrelated to what ->init() returns.
>>
>> plt_tsc.frequency is certainly initialized in early_time_init(). And then on
>> try_platform_timer we have plt_src = *pts (pts would be a pointer to plt_tsc 
>> when
>> called from verify_tsc_reliability()).
>>
>> IOW, effectively I changed from this:
>>
>> #v2
>>
>> static u64 tsc_freq;
>>
>> static s64 __init init_tsctimer(struct platform_timesource *pts)
>> {
>>    ...
>>    pts->frequency = tsc_freq;
>>    return 1;
>> }
>>
>> ...
>>
>> void __init early_time_init(void)
>> {
>>    u64 tmp = init_pit_and_calibrate_tsc();
>>
>>    tsc_freq = tmp;
>> }
>>
>> *To:*
>>
>> #v3
>>
>> static s64 __init init_tsctimer(struct platform_timesource *pts)
>> {
>>     return pts->frequency;
>> }
>>
>>
>> void __init early_time_init(void)
>> {
>>     ...
>>     tmp = init_platform_timer();
>>     plt_tsc.frequency = tmp;
>> }
>>
>> Does this answer your question? Note that my purpose with the change, was to remove
>> the tsc_freq temporary variable. If it makes things less clear (as in doing things
>> differently from other platform timers) I can go back to v2 in this aspect.
> 
> Ah, I see now how I got confused. This once again depends on TSC
> to possible become the platform timer only much later than when
> early_time_init() runs.
>
True, but here we're only initializing frequency at this point, yet it's only used if
reliable tsc conditions do verify.

Joao

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