| Message ID | 1459259051-4943-3-git-send-email-joao.m.martins@oracle.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, Mar 29, 2016 at 02:44:07PM +0100, Joao Martins wrote: > And accomodate platform time source initialization in > try_platform_time(). This is a preparatory patch for deferring > TSC clocksource initialization to the stage where all CPUS are > up (verify_tsc_reliability init call). > > Signed-off-by: Joao Martins <joao.m.martins@oracle.com> Reviewed-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
On 04/01/2016 05:10 PM, Konrad Rzeszutek Wilk wrote: > On Tue, Mar 29, 2016 at 02:44:07PM +0100, Joao Martins wrote: >> And accomodate platform time source initialization in >> try_platform_time(). This is a preparatory patch for deferring >> TSC clocksource initialization to the stage where all CPUS are >> up (verify_tsc_reliability init call). >> >> Signed-off-by: Joao Martins <joao.m.martins@oracle.com> > > Reviewed-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> > Thanks!
>>> 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 > @@ -533,6 +533,30 @@ static void resume_platform_timer(void) > plt_stamp = plt_src.read_counter(); > } > > +static int __init try_platform_timer(struct platform_timesource *pts) > +{ > + int rc = -1; Pointless initializer. In fact ... > + rc = pts->init(pts); ... this could be the initializer. > + if ( rc <= 0 ) > + return rc; > + > + plt_mask = (u64)~0ull >> (64 - pts->counter_bits); > + > + 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); > + > + platform_timer_stamp = plt_stamp64; > + stime_platform_stamp = NOW(); > + > + return rc; > +} Moving here all this setting up of static/global data makes me wonder how you mean to consistently re-use this function for your new purpose. Jan
On 04/05/2016 11:09 AM, 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 >> @@ -533,6 +533,30 @@ static void resume_platform_timer(void) >> plt_stamp = plt_src.read_counter(); >> } >> >> +static int __init try_platform_timer(struct platform_timesource *pts) >> +{ >> + int rc = -1; > > Pointless initializer. In fact ... > >> + rc = pts->init(pts); > > ... this could be the initializer. > Ah yes. Will fix it. >> + if ( rc <= 0 ) >> + return rc; >> + >> + plt_mask = (u64)~0ull >> (64 - pts->counter_bits); >> + >> + 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); >> + >> + platform_timer_stamp = plt_stamp64; >> + stime_platform_stamp = NOW(); >> + >> + return rc; >> +} > > Moving here all this setting up of static/global data makes me > wonder how you mean to consistently re-use this function for > your new purpose. My purpose is to reuse this initialization part for the case of switching from from one clocksource to TSC at a later point (which is done in a different place i.e. verify_tsc_reliability). Though this static/global data part in particular is done if the clocksource initialization succeeds so I merged that in a single helper hence the name "try_platform_timer". It also looks cleaner, and we would leave opt_clocksource checking with plt_timers in init_platform_time. Joao
>>> On 05.04.16 at 12:55, <joao.m.martins@oracle.com> wrote: > On 04/05/2016 11:09 AM, Jan Beulich wrote: >>>>> On 29.03.16 at 15:44, <joao.m.martins@oracle.com> wrote: >>> + if ( rc <= 0 ) >>> + return rc; >>> + >>> + plt_mask = (u64)~0ull >> (64 - pts->counter_bits); >>> + >>> + 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); >>> + >>> + platform_timer_stamp = plt_stamp64; >>> + stime_platform_stamp = NOW(); >>> + >>> + return rc; >>> +} >> >> Moving here all this setting up of static/global data makes me >> wonder how you mean to consistently re-use this function for >> your new purpose. > My purpose is to reuse this initialization part for the case of switching > from > from one clocksource to TSC at a later point (which is done in a different > place > i.e. verify_tsc_reliability). Though this static/global data part in > particular > is done if the clocksource initialization succeeds so I merged that in a > single > helper hence the name "try_platform_timer". It also looks cleaner, and we > would > leave opt_clocksource checking with plt_timers in init_platform_time. Well, I understand all this, but still wonder - is this second use then correct? See also the comments on the next patch. Jan
diff --git a/xen/arch/x86/time.c b/xen/arch/x86/time.c index 687e39b..ed4ed24 100644 --- a/xen/arch/x86/time.c +++ b/xen/arch/x86/time.c @@ -533,6 +533,30 @@ static void resume_platform_timer(void) plt_stamp = plt_src.read_counter(); } +static int __init try_platform_timer(struct platform_timesource *pts) +{ + int rc = -1; + + rc = pts->init(pts); + if ( rc <= 0 ) + return rc; + + plt_mask = (u64)~0ull >> (64 - pts->counter_bits); + + 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); + + platform_timer_stamp = plt_stamp64; + stime_platform_stamp = NOW(); + + return rc; +} + static void __init init_platform_timer(void) { static struct platform_timesource * __initdata plt_timers[] = { @@ -549,7 +573,7 @@ static void __init init_platform_timer(void) pts = plt_timers[i]; if ( !strcmp(opt_clocksource, pts->id) ) { - rc = pts->init(pts); + rc = try_platform_timer(pts); break; } } @@ -565,26 +589,13 @@ static void __init init_platform_timer(void) for ( i = 0; i < ARRAY_SIZE(plt_timers); i++ ) { pts = plt_timers[i]; - if ( (rc = pts->init(pts)) > 0 ) + if ( (rc = try_platform_timer(pts)) > 0 ) break; } } BUG_ON(rc <= 0); - plt_mask = (u64)~0ull >> (64 - pts->counter_bits); - - 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); - - platform_timer_stamp = plt_stamp64; - stime_platform_stamp = NOW(); - printk("Platform timer is %s %s\n", freq_string(pts->frequency), pts->name); }
And accomodate platform time source initialization in try_platform_time(). This is a preparatory patch for deferring TSC clocksource initialization to the stage where all CPUS are up (verify_tsc_reliability init call). Signed-off-by: Joao Martins <joao.m.martins@oracle.com> --- Cc: Keir Fraser <keir@xen.org> Cc: Jan Beulich <jbeulich@suse.com> Cc: Andrew Cooper <andrew.cooper3@citrix.com> New in v2. --- xen/arch/x86/time.c | 41 ++++++++++++++++++++++++++--------------- 1 file changed, 26 insertions(+), 15 deletions(-)