Re: [Xen-devel] [PATCH v2 4/6] x86/time: streamline platform time init on plt_init()

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 29.03.16 at 15:44, <joao.m.martins@xxxxxxxxxx> wrote:
> --- a/xen/arch/x86/time.c
> +++ b/xen/arch/x86/time.c
> @@ -434,6 +434,7 @@ uint64_t ns_to_acpi_pm_tick(uint64_t ns)
>  /************************************************************
>   * PLATFORM TIMER 4: TSC
>   */
> +static bool_t clocksource_is_tsc;

__read_mostly, but see below.

> @@ -516,17 +519,31 @@ static s_time_t __read_platform_stime(u64 platform_time)
>      return (stime_platform_stamp + scale_delta(diff, &plt_scale));
>  }
>  
> +static void __plt_init(void)
> +{
> +    u64 count;
> +
> +    ASSERT(spin_is_locked(&platform_timer_lock));
> +    count = plt_src.read_counter();
> +    plt_stamp64 += (count - plt_stamp) & plt_mask;
> +    plt_stamp = count;
> +}

Note that this has nothing to do with "init" - it updates the two time
stamps, as is being made clear by ...

>  static void plt_overflow(void *unused)
>  {
>      int i;
> -    u64 count;
>      s_time_t now, plt_now, plt_wrap;
>  
>      spin_lock_irq(&platform_timer_lock);
>  
> -    count = plt_src.read_counter();
> -    plt_stamp64 += (count - plt_stamp) & plt_mask;
> -    plt_stamp = count;
> +    __plt_init();

... this use.

> @@ -621,11 +638,22 @@ static int __init try_platform_timer(struct 
> platform_timesource *pts)
>  
>      set_time_scale(&plt_scale, pts->frequency);
>  
> -    plt_overflow_period = scale_delta(
> -        1ull << (pts->counter_bits - 1), &plt_scale);
> -    init_timer(&plt_overflow_timer, plt_overflow, NULL, 0);
>      plt_src = *pts;
> -    plt_overflow(NULL);
> +
> +    if ( clocksource_is_tsc )

Why not simply "if ( pts == plt_tsc )", eliminating the need for the
variable?

> +    {
> +        plt_init();
> +    }
> +    else
> +    {
> +        plt_overflow_period = scale_delta(
> +            1ull << (pts->counter_bits - 1), &plt_scale);
> +        init_timer(&plt_overflow_timer, plt_overflow, NULL, 0);
> +        plt_overflow(NULL);
> +
> +        printk("Platform timer overflow period is %lu secs\n",
> +               plt_overflow_period/SECONDS(1));

If we want this logged at all, then please at most as XENLOG_INFO.
Plus - is seconds granularity fine grained enough for all sources, i.e.
wouldn't there for typical HPET just be a single digit, not a lot of
precision that is? And finally: Blanks around / please.

Jan


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