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

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

On 09/20/2016 02:55 PM, Jan Beulich wrote:
>>>> On 20.09.16 at 12:15, <joao.m.martins@oracle.com> wrote:
>> On 09/20/2016 08:13 AM, Jan Beulich wrote:
>>>>>> On 19.09.16 at 19:54, <joao.m.martins@oracle.com> wrote:
>>>> On 09/19/2016 05:25 PM, Jan Beulich wrote:
>>>>>>>> On 19.09.16 at 18:11, <joao.m.martins@oracle.com> wrote:
>>>>>> On 09/19/2016 11:13 AM, Jan Beulich wrote:
>>>>>>>>>> On 14.09.16 at 19:37, <joao.m.martins@oracle.com> 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).
>>>> The first calibration won't change much - this 1-sec was meant when having
>>>> nop_rendezvous which is the first time platform timer would be used to set local
>>>> cpu_time (will adjust the mention above as it's misleading for the reader as it
>>>> doesn't refer to this patch).
>>>
>>> So what makes it that it actually _is_ nop_rendezvous after that one
>>> second? (And yes, part of this may indeed be just bad placement of
>>> the description, as iirc nop_rendezvous gets introduced only in a later
>>> patch.)
>> Because with nop_rendezvous we will be using the platform timer to get a
>> monotonic time tuple and *set* cpu_time as opposed to just adding up plain TSC
>> delta as it is the case prior to b64438c7c. Thus the reseeding of the cpu times
>> solves both ends of the problem, with nop_rendezvous until it is first
>> calibration fixes it, and without nop_rendezvous to remove the latch adjustment
>> from initial platform timer.
> 
> So am I getting you right (together with the second part of your reply
> further down) that you escape answering the question raised by saying
> that it doesn't really matter which rendezvous function gets used, when
> TSC is the clock source?
Correct and in my defense I wasn't escaping the question, as despite
unfortunate mis-mention in the patch (or bad English) I think the above
explains it. During that time window, we now just need to ensure that we will
get monotonic results solely based on the individual CPU time (i.e. calls to
get_s_time or anything that uses cpu_time). Unless the calibration function is
doing something wrong/fishy, I don't see a reason for this to go wrong.

> I.e. the introduction of nop_rendezvous is
> really just to avoid unnecessary overhead?
Yes, but note that it's only the case since recent commit b64438c7c where
cpu_time stime is now incremented with TSC based deltas with a matching TSC
stamp. Before it wasn't the case. The main difference with nop_rendezvous (other
than the significant overhead) versus std_rendezvous is that we use a single
global tuple propagated to all cpus, whereas with std_rendezvous each tuple is
different and will vary according to when it rendezvous with cpu 0.

> In which case it should
> probably be a separate patch, saying so in its description.
OK, will move that out of Patch 4 into its own while keeping the same logic.

Joao

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