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

[Dou Liyang]

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.

Thanks,
	dou

diff --git a/arch/x86/kernel/setup.c b/arch/x86/kernel/setup.c
index da1dbd99cb6e..74cb16d89e25 100644
--- a/arch/x86/kernel/setup.c
+++ b/arch/x86/kernel/setup.c
@@ -1197,12 +1197,12 @@ void __init setup_arch(char **cmdline_p)

        memblock_find_dma_reserve();

+       x86_init.paging.pagetable_init();
+
        tsc_early_delay_calibrate();
        if (!early_xdbc_setup_hardware())
                early_xdbc_register_console();

-       x86_init.paging.pagetable_init();
-
        kasan_init();

        /*

> Thank you,
> Pavel
>
> > Thanks,
> >          dou
> > ------------------------8<-----------------------------------
> >
> > diff --git a/arch/x86/kernel/tsc.c b/arch/x86/kernel/tsc.c
> > index 74392d9d51e0..e54fa1037d45 100644
> > --- a/arch/x86/kernel/tsc.c
> > +++ b/arch/x86/kernel/tsc.c
> > @@ -1370,8 +1370,10 @@ void __init tsc_init(void)
> >                   return;
> >           }
> >
> > -       cpu_khz = x86_platform.calibrate_cpu();
> > -       tsc_khz = x86_platform.calibrate_tsc();
> > +       if (!tsc_khz) {
> > +               cpu_khz = x86_platform.calibrate_cpu();
> > +               tsc_khz = x86_platform.calibrate_tsc();
> > +       }
> >
> >           /*
> >            * Trust non-zero tsc_khz as authorative,