Re: [PATCH v10 7/7] x86/tsc: use tsc early

[Pavel Tatashin]

On 06/19/2018 07:52 PM, Thomas Gleixner wrote:
> On Fri, 15 Jun 2018, Pavel Tatashin wrote:
> 
>> tsc_early_init():
>> Determines offset, shift and multiplier for the early clock based on the
>> TSC frequency.
>>
>> tsc_early_fini()
>> Implement the finish part of early tsc feature, prints message about the
>> offset, which can be useful to find out how much time was spent in post and
>> boot manager (if TSC starts from 0 during boot)
>>
>> sched_clock_early():
>> TSC based implementation of early clock and is called from sched_clock().
>>
>> start_early_clock():
>> Calls tsc_early_init(), and makes sched_clock() to use early boot clock
>>
>> set_final_clock():
>> Sets the final clock which is either platform specific or
>> native_sched_clock(). Also calls tsc_early_fini() if early clock was
>> previously initialized.
>>
>> Call start_early_clock() to start using early boot time stamps
>> functionality on the supported x86 platforms, and call set_final_clock() to
>> finish this feature and switch back to the default clock. The supported x86
>> systems are those where TSC frequency is determined early in boot.
> 
> Lots of functions for dubious value.
>   
>> +static struct cyc2ns_data  cyc2ns_early;
>> +
>> +static u64 sched_clock_early(void)
>> +{
>> +	u64 ns = mul_u64_u32_shr(rdtsc(), cyc2ns_early.cyc2ns_mul,
>> +				 cyc2ns_early.cyc2ns_shift);
>> +	return ns + cyc2ns_early.cyc2ns_offset;
>> +}
>> +
>> +/*
>> + * Initialize clock for early time stamps
>> + */
>> +static void __init tsc_early_init(unsigned int khz)
>> +{
>> +	clocks_calc_mult_shift(&cyc2ns_early.cyc2ns_mul,
>> +			       &cyc2ns_early.cyc2ns_shift,
>> +			       khz, NSEC_PER_MSEC, 0);
>> +	cyc2ns_early.cyc2ns_offset = -sched_clock_early();
>> +}
>> +
>> +/*
>> + * Finish clock for early time stamps, and hand over to permanent clock by
>> + * setting __sched_clock_offset appropriately for continued time keeping.
>> + */
>> +static void __init tsc_early_fini(void)
>> +{
>> +	unsigned long long t;
>> +	unsigned long r;
>> +
>> +	t = -cyc2ns_early.cyc2ns_offset;
>> +	r = do_div(t, NSEC_PER_SEC);
>> +
>> +	__sched_clock_offset = sched_clock_early() - sched_clock();
>> +	pr_info("early sched clock is finished, offset [%lld.%09lds]\n", t, r);
>> +}
>> +
>> +#ifdef CONFIG_PARAVIRT
>> +static inline void __init start_early_clock(void)
>> +{
>> +	tsc_early_init(tsc_khz);
>> +	pv_time_ops.active_sched_clock = sched_clock_early;
>> +}
>> +
>> +static inline void __init set_final_clock(void)
>> +{
>> +	pv_time_ops.active_sched_clock = pv_time_ops.sched_clock;
>> +
>> +	/* We did not have early sched clock if multiplier is 0 */
>> +	if (cyc2ns_early.cyc2ns_mul)
>> +		tsc_early_fini();
>> +}
>> +#else /* CONFIG_PARAVIRT */
>> +/*
>> + * For native clock we use two switches static and dynamic, the static switch is
>> + * initially true, so we check the dynamic switch, which is initially false.
>> + * Later  when early clock is disabled, we can alter the static switch in order
>> + * to avoid branch check on every sched_clock() call.
>> + */
>> +static bool __tsc_early;
>> +static DEFINE_STATIC_KEY_TRUE(__tsc_early_static);
>> +
>> +static inline void __init start_early_clock(void)
>> +{
>> +	tsc_early_init(tsc_khz);
>> +	__tsc_early = true;
>> +}
>> +
>> +static inline void __init set_final_clock(void)
>> +{
>> +	__tsc_early = false;
>> +	static_branch_disable(&__tsc_early_static);
>> +
>> +	/* We did not have early sched clock if multiplier is 0 */
>> +	if (cyc2ns_early.cyc2ns_mul)
>> +		tsc_early_fini();
>> +}
>> +#endif /* CONFIG_PARAVIRT */
>> +
>>  /*
>>   * Scheduler clock - returns current time in nanosec units.
>>   */
>> @@ -194,6 +272,13 @@ u64 native_sched_clock(void)
>>  		return cycles_2_ns(tsc_now);
>>  	}
>>  
>> +#ifndef CONFIG_PARAVIRT
>> +	if (static_branch_unlikely(&__tsc_early_static)) {
>> +		if (__tsc_early)
>> +			return sched_clock_early();
>> +	}
>> +#endif /* !CONFIG_PARAVIRT */
>> +
> 
> This whole function maze plus the ifdeffery which comes with it is really
> horrible and not required. What's wrong with reusing the existing
> functionality?
> 
> The patch below (uncompiled and untested) should achieve the same thing
> without all the paravirt muck (which can be easily added w/o all the
> ifdeffery if really required) by just reusing the existing conversion and
> initialization functions.
> 
> If I'm not completely mistaken then the second invocation of
> set_cyc2ns_scale() from tsc_init() will also take care of the smooth
> sched_clock() transition from early to final w/o touching the core
> __sched_clock_offset at all. Though my tired brain might trick me.
> 
> It might not work as is, but it should not be rocket science to make it do
> so.
> 
> Thanks,
> 
> 	tglx
> 
> 8<----------------------
> 
> tsc.c |   59 ++++++++++++++++++++++++++++++++++++++++++++---------------
>  1 file changed, 44 insertions(+), 15 deletions(-)
> 
> --- a/arch/x86/kernel/tsc.c
> +++ b/arch/x86/kernel/tsc.c
> @@ -39,6 +39,9 @@ EXPORT_SYMBOL(tsc_khz);
>  static int __read_mostly tsc_unstable;
>  
>  static DEFINE_STATIC_KEY_FALSE(__use_tsc);
> +static DEFINE_STATIC_KEY_TRUE(tsc_early_enabled);
> +
> +static bool tsc_early_sched_clock;
>  
>  int tsc_clocksource_reliable;
>  
> @@ -133,18 +136,12 @@ static inline unsigned long long cycles_
>  	return ns;
>  }
>  
> -static void set_cyc2ns_scale(unsigned long khz, int cpu, unsigned long long tsc_now)
> +static void __set_cyc2ns_scale(unsigned long khz, int cpu,
> +			       unsigned long long tsc_now)
>  {
>  	unsigned long long ns_now;
>  	struct cyc2ns_data data;
>  	struct cyc2ns *c2n;
> -	unsigned long flags;
> -
> -	local_irq_save(flags);
> -	sched_clock_idle_sleep_event();
> -
> -	if (!khz)
> -		goto done;
>  
>  	ns_now = cycles_2_ns(tsc_now);
>  
> @@ -176,22 +173,46 @@ static void set_cyc2ns_scale(unsigned lo
>  	c2n->data[0] = data;
>  	raw_write_seqcount_latch(&c2n->seq);
>  	c2n->data[1] = data;
> +}
> +
> +static void set_cyc2ns_scale(unsigned long khz, int cpu,
> +			     unsigned long long tsc_now)
> +{
> +	unsigned long flags;
> +
> +	local_irq_save(flags);
> +	sched_clock_idle_sleep_event();
> +
> +	if (khz)
> +		__set_cyc2ns_scale(khz, cpu, tsc_now);
>  
> -done:
>  	sched_clock_idle_wakeup_event();
>  	local_irq_restore(flags);
>  }
>  
> +static void __init sched_clock_early_init(unsigned int khz)
> +{
> +	cyc2ns_init(smp_processor_id());
> +	__set_cyc2ns_scale(khz, smp_processor_id(), rdtsc());
> +	tsc_early_sched_clock = true;
> +}
> +
> +static void __init sched_clock_early_exit(void)
> +{
> +	static_branch_disable(&tsc_early_enabled);
> +}
> +
>  /*
>   * Scheduler clock - returns current time in nanosec units.
>   */
>  u64 native_sched_clock(void)
>  {
> -	if (static_branch_likely(&__use_tsc)) {
> -		u64 tsc_now = rdtsc();
> +	if (static_branch_likely(&__use_tsc))
> +		return cycles_2_ns(rdtsc());
>  
> -		/* return the value in ns */
> -		return cycles_2_ns(tsc_now);
> +	if (static_branch_unlikely(&tsc_early_enabled)) {
> +		if (tsc_early_sched_clock)
> +			return cycles_2_ns(rdtsc());
>  	}
>  
>  	/*
> @@ -1332,9 +1353,10 @@ void __init tsc_early_delay_calibrate(vo
>  	lpj = tsc_khz * 1000;
>  	do_div(lpj, HZ);
>  	loops_per_jiffy = lpj;
> +	sched_clock_early_init(tsc_khz);
>  }
>  
> -void __init tsc_init(void)
> +static void __init __tsc_init(void)
>  {
>  	u64 lpj, cyc;
>  	int cpu;
> @@ -1384,7 +1406,8 @@ void __init tsc_init(void)
>  	 */
>  	cyc = rdtsc();
>  	for_each_possible_cpu(cpu) {
> -		cyc2ns_init(cpu);
> +		if (!tsc_early_sched_clock || cpu != smp_processor_id())
> +			cyc2ns_init(cpu);
>  		set_cyc2ns_scale(tsc_khz, cpu, cyc);
>  	}
>  
> @@ -1411,6 +1434,12 @@ void __init tsc_init(void)
>  	detect_art();
>  }
>  
> +void __init tsc_init(void)
> +{
> +	__tsc_init();
> +	sched_clock_early_exit();
> +}
> +
>  #ifdef CONFIG_SMP
>  /*
>   * If we have a constant TSC and are using the TSC for the delay loop,
> 
> 
> 

Hi Thomas,

I like this approach, as you said in patch 6, new paravirt functions are not needed because if hypervisor provides sched_clock() just use it, so we do not need to have ifdefs for paravirt stuff here.

I will update the whole series and send it out again. It will be in a much better shape after your review!

Thank you,
Pavel