Re: [Xen-devel] [PATCH v4 2/5] x86/time: implement tsc as clocksource

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 19.09.16 at 18:11, <joao.m.martins@xxxxxxxxxx> wrote:
> On 09/19/2016 11:13 AM, Jan Beulich wrote:
>>>>> On 14.09.16 at 19:37, <joao.m.martins@xxxxxxxxxx> wrote:
>>> Since b64438c7c ("x86/time: use correct (local) time stamp in
>>> constant-TSC calibration fast path") updates to cpu time use local
>>> stamps, which means platform timer is only used to seed the initial
>>> cpu time.  With clocksource=tsc there is no need to be in sync with
>>> another clocksource, so we reseed the local/master stamps to be values
>>> of TSC and update the platform time stamps accordingly. Time
>>> calibration is set to 1sec after we switch to TSC, thus these stamps
>>> are reseeded to also ensure monotonic returning values right after the
>>> point we switch to TSC. This is also to avoid the possibility of
>>> having inconsistent readings in this short period (i.e. until
>>> calibration fires).
>> 
>> And within this one second, which may cover some of Dom0's
>> booting up, it is okay to have inconsistencies?
> It's not okay which is why I am removing this possibility when switching to 
> TSC.
> The inconsistencies in those readings (if I wasn't adjusting) would be because
> we would be using (in that 1-sec) those cpu time tuples calculated by the
> previous calibration or platform time initialization (while still was HPET,
> ACPI, etc as clocksource). Would you prefer me removing the "avoid" and 
> instead
> change it to "remove the possibility" in this last sentence?

Let's not do the 2nd step before the 1st, which is the question of
what happens prior to and what actually changes at this first
calibration (after 1 sec).

>>> +static void __init try_platform_timer_tail(void)
>>> +{
>>> +    init_timer(&plt_overflow_timer, plt_overflow, NULL, 0);
>>> +    plt_overflow(NULL);
>>> +
>>> +    platform_timer_stamp = plt_stamp64;
>>> +    stime_platform_stamp = NOW();
>>> +
>>> +    if ( !clocksource_is_tsc() )
>>> +        init_percpu_time();
>> 
>> This isn't really dependent on whether TSC is used as clocksource,
>> but solely on the point in time at which the call gets made, is it? If
>> so, I think an explicit boolean function parameter (named e.g. "late")
>> would be better than abusing the predicate here.
> 
> Correct, I will introduce this boolean parameter. Not that is critical but
> probably add an likely(...) there too, since the late case only happens for
> clocksource=tsc ?

Well, in __init code I prefer to avoid likely()/unlikely(), unless it's
in e.g. a performance critical loop.

>>> @@ -1480,6 +1570,25 @@ static int __init verify_tsc_reliability(void)
>>>              printk("TSC warp detected, disabling TSC_RELIABLE\n");
>>>              setup_clear_cpu_cap(X86_FEATURE_TSC_RELIABLE);
>>>          }
>>> +        else if ( !strcmp(opt_clocksource, "tsc") &&
>>> +                  (try_platform_timer(&plt_tsc) > 0) )
>>> +        {
>>> +            /*
>>> +             * Platform timer has changed and CPU time will only be updated
>>> +             * after we set again the calibration timer, which means we 
>>> need to
>>> +             * seed again each local CPU time. At this stage TSC is known 
>>> to be
>>> +             * reliable i.e. monotonically increasing across all CPUs so 
>>> this
>>> +             * lets us remove the skew between platform timer and TSC, 
>>> since
>>> +             * these are now effectively the same.
>>> +             */
>>> +            on_selected_cpus(&cpu_online_map, reset_percpu_time, NULL, 1);
>>> +
>>> +            /* Finish platform timer switch. */
>>> +            try_platform_timer_tail();
>>> +
>>> +            printk(XENLOG_INFO "Switched to Platform timer %s TSC\n",
>>> +                   freq_string(plt_src.frequency));
>> 
>> This message should have the same log level as the one at the end
>> of init_platform_timer().
> Agreed, but at the end of init_platform_timer there is a plain printk with an
> omitted log level. Or do you mean to remove XENLOG_INFO from this printk above
> or, instead add XENLOG_INFO to one printk at the end of 
> init_platform_timer() ?

Well, info would again be too low a level for my taste. Hence either
remove the level here (slightly preferred from my pov), or make both
warning.

Jan

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