| Message ID | 1459259051-4943-6-git-send-email-joao.m.martins@oracle.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, Mar 29, 2016 at 02:44:10PM +0100, Joao Martins wrote: > To fetch the last read from the clocksource which was used to > calculate system_time. In the case of clocksource=tsc we will > use it to set tsc_timestamp. > > Signed-off-by: Joao Martins <joao.m.martins@oracle.com> > Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com> Reviewed-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
>>> On 29.03.16 at 15:44, <joao.m.martins@oracle.com> wrote: > -static s_time_t read_platform_stime(void) > +static s_time_t read_platform_stime(u64 *stamp) > { > - u64 count; > + u64 plt_stamp_counter, count; "stamp" and "counter" seem kind of redundant. > s_time_t stime; > > ASSERT(!local_irq_is_enabled()); > > spin_lock(&platform_timer_lock); > - count = plt_stamp64 + ((plt_src.read_counter() - plt_stamp) & plt_mask); > + plt_stamp_counter = plt_src.read_counter(); > + count = plt_stamp64 + ((plt_stamp_counter - plt_stamp) & plt_mask); > stime = __read_platform_stime(count); > + if ( stamp ) > + *stamp = plt_stamp_counter; > spin_unlock(&platform_timer_lock); What reason is there to do that conditional write inside the locked region? Jan
On 04/05/2016 12:52 PM, Jan Beulich wrote: >>>> On 29.03.16 at 15:44, <joao.m.martins@oracle.com> wrote: >> -static s_time_t read_platform_stime(void) >> +static s_time_t read_platform_stime(u64 *stamp) >> { >> - u64 count; >> + u64 plt_stamp_counter, count; > > "stamp" and "counter" seem kind of redundant. > A bit, perhaps you prefer the latter? There was a variable named "count", so I named "stamp" for clearer distinction between the variables and the output arg. >> s_time_t stime; >> >> ASSERT(!local_irq_is_enabled()); >> >> spin_lock(&platform_timer_lock); >> - count = plt_stamp64 + ((plt_src.read_counter() - plt_stamp) & plt_mask); >> + plt_stamp_counter = plt_src.read_counter(); >> + count = plt_stamp64 + ((plt_stamp_counter - plt_stamp) & plt_mask); >> stime = __read_platform_stime(count); >> + if ( stamp ) >> + *stamp = plt_stamp_counter; >> spin_unlock(&platform_timer_lock); > > What reason is there to do that conditional write inside the locked > region? > None, I should move this conditional write out of this region. Joao
>>> On 05.04.16 at 17:22, <joao.m.martins@oracle.com> wrote: > On 04/05/2016 12:52 PM, Jan Beulich wrote: >>>>> On 29.03.16 at 15:44, <joao.m.martins@oracle.com> wrote: >>> -static s_time_t read_platform_stime(void) >>> +static s_time_t read_platform_stime(u64 *stamp) >>> { >>> - u64 count; >>> + u64 plt_stamp_counter, count; >> >> "stamp" and "counter" seem kind of redundant. >> > A bit, perhaps you prefer the latter? There was a variable named "count", so I > named "stamp" for clearer distinction between the variables and the output arg. Between plt_stamp and plt_counter I'd indeed have a slight preference towards the latter. Jan
On 04/05/2016 04:26 PM, Jan Beulich wrote: >>>> On 05.04.16 at 17:22, <joao.m.martins@oracle.com> wrote: >> On 04/05/2016 12:52 PM, Jan Beulich wrote: >>>>>> On 29.03.16 at 15:44, <joao.m.martins@oracle.com> wrote: >>>> -static s_time_t read_platform_stime(void) >>>> +static s_time_t read_platform_stime(u64 *stamp) >>>> { >>>> - u64 count; >>>> + u64 plt_stamp_counter, count; >>> >>> "stamp" and "counter" seem kind of redundant. >>> >> A bit, perhaps you prefer the latter? There was a variable named "count", so I >> named "stamp" for clearer distinction between the variables and the output arg. > > Between plt_stamp and plt_counter I'd indeed have a slight preference > towards the latter. OK, I will fix that.
diff --git a/xen/arch/x86/time.c b/xen/arch/x86/time.c index 9cadfcb..123aa42 100644 --- a/xen/arch/x86/time.c +++ b/xen/arch/x86/time.c @@ -568,16 +568,19 @@ static void plt_overflow(void *unused) set_timer(&plt_overflow_timer, NOW() + plt_overflow_period); } -static s_time_t read_platform_stime(void) +static s_time_t read_platform_stime(u64 *stamp) { - u64 count; + u64 plt_stamp_counter, count; s_time_t stime; ASSERT(!local_irq_is_enabled()); spin_lock(&platform_timer_lock); - count = plt_stamp64 + ((plt_src.read_counter() - plt_stamp) & plt_mask); + plt_stamp_counter = plt_src.read_counter(); + count = plt_stamp64 + ((plt_stamp_counter - plt_stamp) & plt_mask); stime = __read_platform_stime(count); + if ( stamp ) + *stamp = plt_stamp_counter; spin_unlock(&platform_timer_lock); return stime; @@ -727,7 +730,7 @@ void cstate_restore_tsc(void) if ( boot_cpu_has(X86_FEATURE_NONSTOP_TSC) ) return; - write_tsc(stime2tsc(read_platform_stime())); + write_tsc(stime2tsc(read_platform_stime(NULL))); } /*************************************************************************** @@ -1046,7 +1049,7 @@ int cpu_frequency_change(u64 freq) local_irq_disable(); /* Platform time /first/, as we may be delayed by platform_timer_lock. */ - t->stime_master_stamp = read_platform_stime(); + t->stime_master_stamp = read_platform_stime(NULL); /* TSC-extrapolated time may be bogus after frequency change. */ /*t->stime_local_stamp = get_s_time();*/ t->stime_local_stamp = t->stime_master_stamp; @@ -1364,7 +1367,7 @@ static void time_calibration_tsc_rendezvous(void *_r) if ( r->master_stime == 0 ) { - r->master_stime = read_platform_stime(); + r->master_stime = read_platform_stime(NULL); r->master_tsc_stamp = rdtsc(); } atomic_inc(&r->semaphore); @@ -1409,7 +1412,7 @@ static void time_calibration_std_rendezvous(void *_r) { while ( atomic_read(&r->semaphore) != (total_cpus - 1) ) cpu_relax(); - r->master_stime = read_platform_stime(); + r->master_stime = read_platform_stime(NULL); mb(); /* write r->master_stime /then/ signal */ atomic_inc(&r->semaphore); } @@ -1456,7 +1459,7 @@ void init_percpu_time(void) local_irq_save(flags); t->local_tsc_stamp = rdtsc(); - now = read_platform_stime(); + now = read_platform_stime(NULL); local_irq_restore(flags); t->stime_master_stamp = now;