| Message ID | 1459259051-4943-5-git-send-email-joao.m.martins@oracle.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
>>> On 29.03.16 at 15:44, <joao.m.martins@oracle.com> 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
On 04/05/2016 12:46 PM, Jan Beulich wrote: >>>> On 29.03.16 at 15:44, <joao.m.martins@oracle.com> 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. > Would you prefer changing the name to e.g "set_plt_stamp" ? >> @@ -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? Yeah, good point. I will fix that. > >> + { >> + 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. OK. > 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? Could be, my HPET was around 2 minutes overflow period, but PIT was a single digit as you mention. I will change that to MILLISECS(1000) for higher precision - or I can remove it entirely if you prefer not logging this info. > And finally: Blanks around / please. OK.
>>> On 05.04.16 at 17:12, <joao.m.martins@oracle.com> wrote: > On 04/05/2016 12:46 PM, Jan Beulich wrote: >>>>> On 29.03.16 at 15:44, <joao.m.martins@oracle.com> wrote: >>> @@ -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. >> > Would you prefer changing the name to e.g "set_plt_stamp" ? Or simply plt_update()? >>> + { >>> + 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. > OK. > >> 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? > Could be, my HPET was around 2 minutes overflow period, but PIT was a single > digit as you mention. I will change that to MILLISECS(1000) for higher > precision How is MILLISECS(1000) different from SECONDS(1)? > - or I can remove it entirely if you prefer not logging this info. Well, there had been times where this information would have been quite useful in diagnosing problems. That's been a while back, but knowing we had such issues I can't just say "drop the message", even if I hope we won't have any similar problems anymore. Jan
On 04/05/2016 04:22 PM, Jan Beulich wrote: >>>> On 05.04.16 at 17:12, <joao.m.martins@oracle.com> wrote: >> On 04/05/2016 12:46 PM, Jan Beulich wrote: >>>>>> On 29.03.16 at 15:44, <joao.m.martins@oracle.com> wrote: >>>> @@ -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. >>> >> Would you prefer changing the name to e.g "set_plt_stamp" ? > > Or simply plt_update()? Sounds better indeed. > >>>> + { >>>> + 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. >> OK. >> >>> 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? >> Could be, my HPET was around 2 minutes overflow period, but PIT was a single >> digit as you mention. I will change that to MILLISECS(1000) for higher >> precision > > How is MILLISECS(1000) different from SECONDS(1)? Sorry, It's not - I meant MILLISECS(1). > >> - or I can remove it entirely if you prefer not logging this info. > > Well, there had been times where this information would have been > quite useful in diagnosing problems. That's been a while back, but > knowing we had such issues I can't just say "drop the message", > even if I hope we won't have any similar problems anymore. I will keep it then - until further notice. Joao
diff --git a/xen/arch/x86/time.c b/xen/arch/x86/time.c index 2602dda..9cadfcb 100644 --- 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; static u64 tsc_freq; static unsigned long tsc_max_warp; static void tsc_check_reliability(void); @@ -466,6 +467,8 @@ static int __init init_tsctimer(struct platform_timesource *pts) } pts->frequency = tsc_freq; + clocksource_is_tsc = tsc_reliable; + return tsc_reliable; } @@ -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; +} + +static void plt_init(void) +{ + spin_lock_irq(&platform_timer_lock); + __plt_init(); + spin_unlock_irq(&platform_timer_lock); +} + 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(); now = NOW(); plt_wrap = __read_platform_stime(plt_stamp64); @@ -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 ) + { + 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)); + } platform_timer_stamp = plt_stamp64; stime_platform_stamp = NOW();