Re: [PATCH v13 13/18] x86/tsc: calibrate tsc only once

[Pavel Tatashin]

On Mon, Jul 16, 2018 at 5:33 AM Dou Liyang <douly.fnst@xxxxxxxxxxxxxx> wrote:
>
>
>
> At 07/13/2018 07:30 PM, Pavel Tatashin wrote:
> > On Fri, Jul 13, 2018 at 3:24 AM Dou Liyang <douly.fnst@xxxxxxxxxxxxxx> wrote:
> >>
> >>
> >> At 07/12/2018 08:04 AM, Pavel Tatashin wrote:
> >>> During boot tsc is calibrated twice: once in tsc_early_delay_calibrate(),
> >>> and the second time in tsc_init().
> >>>
> >>> Rename tsc_early_delay_calibrate() to tsc_early_init(), and rework it so
> >>> the calibration is done only early, and make tsc_init() to use the values
> >>> already determined in tsc_early_init().
> >>>
> >>> Sometimes it is not possible to determine tsc early, as the subsystem that
> >>> is required is not yet initialized, in such case try again later in
> >>> tsc_init().
> >>>
> >>> Suggested-by: Thomas Gleixner <tglx@xxxxxxxxxxxxx>
> >>> Signed-off-by: Pavel Tatashin <pasha.tatashin@xxxxxxxxxx>
> >>
> >> Hi Pavel,
> >>
> >> Aha, a complex solution for a simple problem! ;-) And I did find any
> >> benefits of doing that. did I miss something?
> >
> > Hi Dou,
> >
> > I had this in previous version:  init early, and unconditionally
> > re-init later (which required to resync sched clocks for continuity,
> > and check for some other corner cases). Thomas did not like the idea,
> > as it is less deterministic: it leads for code to work by accident,
> > where we might get one tsc frequency early and another later, and so
> > on. The request was to initialize only once, and if that fails do it
> > again later. This way, if early initialization is broken, we will know
> > and fix it.
> >
>
> Hi Pavel,
>
> Yes, right, I have seen the purpose in v12.
>
> >>
> >> As the cpu_khz and tsc_khz are global variables and the tsc_khz may
> >> be reset to cpu_khz. How about the following patch.
> >
> > Could you please explain where you think this patch can be applied,
> > and what it fixes?
> >
>
> This patch is just an simple alternative to realize what you want in
> your patch. your patch is also good but complex, and need some scrub.
> eg:
>    - Is it suitable to using the WARN_ON()
>    - the name of determine_cpu_tsc_frequncies() function,
>     s/frequncies/frequencies/. BTW, How about tsc_calibrate_frequencies()
>    - ...
>
> BTW, before this patch, seems we need make sure xen should work well, I
> can investigate and try to test if we can also move the pagetable_init()
> to the front of tsc_early_init() for you.

v13 has xen patches, so xen timestamps work early as well now.

Thank you,
Pavel