Re: [PATCH v2 3/6] x86/time: implement tsc as clocksource

[Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>]

On Fri, Apr 01, 2016 at 07:38:18PM +0100, Joao Martins wrote:
> [snip]
> 
> >> diff --git a/xen/arch/x86/time.c b/xen/arch/x86/time.c
> >> index ed4ed24..2602dda 100644
> >> --- a/xen/arch/x86/time.c
> >> +++ b/xen/arch/x86/time.c
> >> @@ -432,6 +432,63 @@ uint64_t ns_to_acpi_pm_tick(uint64_t ns)
> >>  }
> >>  
> >>  /************************************************************
> >> + * PLATFORM TIMER 4: TSC
> >> + */
> >> +static u64 tsc_freq;
> >> +static unsigned long tsc_max_warp;
> >> +static void tsc_check_reliability(void);
> >> +
> >> +static int __init init_tsctimer(struct platform_timesource *pts)
> >> +{
> >> +    bool_t tsc_reliable = 0;
> > 
> > No need to set it to zero.
> OK.
> 
> >> +
> >> +    tsc_check_reliability();
> > 
> > This has been already called by verify_tsc_reliability which calls this
> > function. Should we set tsc_max_warp to zero before calling it?
> Ah, correct. But may be it's not necessary to run the tsc_check_reliability here
> at all as what I am doing is ineficient. See my other comment below.
> 
> > 
> >> +
> >> +    if ( tsc_max_warp > 0 )
> >> +    {
> >> +        tsc_reliable = 0;
> > 
> > Ditto. It is by default zero.
> OK.
> 
> > 
> >> +        printk(XENLOG_INFO "TSC: didn't passed warp test\n");
> > 
> > So the earlier test by verify_tsc_reliability did already this check and
> > printed this out - and also cleared the X86_FEATURE_TSC_RELIABLE.
> > 
> > So can this check above be removed then?
> > 
> > Or are you thinking to ditch what verify_tsc_reliability does?
> > 
> I had the tsc_check_reliability here because TSC could still be deemed reliable
> for max_cstate <= 2 or with CONSTANT_TSC + NONSTOP_TSC. The way I have it, the
> most likely scenario (i.e. having TSC_RELIABLE) would run twice. Perhaps a
> better way of doing this would be to run the warp test solely on
> verify_tsc_reliability() in all possible conditions to be deemed reliable? And
> then I could even remove almost the whole init_tsctimer if it was to be called
> when no warps are observed.

So..
> 
> >> +    }
> >> +    else if ( boot_cpu_has(X86_FEATURE_TSC_RELIABLE) ||
> >> +              (boot_cpu_has(X86_FEATURE_CONSTANT_TSC) &&
> >> +               boot_cpu_has(X86_FEATURE_NONSTOP_TSC)) )
> >> +    {
> >> +        tsc_reliable = 1;
> >> +    }
> >> +    else if ( boot_cpu_has(X86_FEATURE_CONSTANT_TSC) )
> >> +    {
> >> +        tsc_reliable = (max_cstate <= 2);
> >> +
> >> +        if ( tsc_reliable )
> >> +            printk(XENLOG_INFO "TSC: no deep Cstates, deemed reliable\n");
> >> +        else
> >> +            printk(XENLOG_INFO "TSC: deep Cstates possible, so not reliable\n");

.. is that always true? As in if this is indeed the case should we clear
X86_FEATURE_CONSTANT_TSC bit? And make check be part of tsc_check_reliability?

Then init_tsctimer() would just need to check for the boot_cpu_has bits being
set.

As in:

 if ( boot_cpu_has(X86_FEATURE_TSC_RELIABLE) ||
      (boot_cpu_has(X86_FEATURE_CONSTANT_TSC) &&
       boot_cpu_has(X86_FEATURE_NONSTOP_TSC)) )
  {
	pts->frequency = tsc_freq;
	return 1;
   }

   return 0;

And tsc_check_reliability would be in charge of clearing the CPU bits if something
is off.

But maybe that is not good? As in, could we mess up and clear those bits
even if they are suppose to be set?

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