| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Fri, 13 Jul 2018 17:11:39 +0800
From: Dou Liyang <douly.fnst@...fujitsu.com>
To: Pavel Tatashin <pasha.tatashin@...cle.com>,
<steven.sistare@...cle.com>, <daniel.m.jordan@...cle.com>,
<linux@...linux.org.uk>, <schwidefsky@...ibm.com>,
<heiko.carstens@...ibm.com>, <john.stultz@...aro.org>,
<sboyd@...eaurora.org>, <x86@...nel.org>,
<linux-kernel@...r.kernel.org>, <mingo@...hat.com>,
<tglx@...utronix.de>, <hpa@...or.com>, <peterz@...radead.org>,
<prarit@...hat.com>, <feng.tang@...el.com>, <pmladek@...e.com>,
<gnomes@...rguk.ukuu.org.uk>, <linux-s390@...r.kernel.org>,
<boris.ostrovsky@...cle.com>, <jgross@...e.com>
Subject: Re: [PATCH v13 14/18] x86/tsc: initialize cyc2ns when tsc freq. is
determined
At 07/12/2018 08:04 AM, Pavel Tatashin wrote:
> cyc2ns converts tsc to nanoseconds, and it is handled in a per-cpu data
> structure.
>
> Currently, the setup code for c2ns data for every possible CPU goes through
> the same sequence of calculations as for the boot CPU, but is based on the
> same tsc frequency as the boot CPU, and thus this is not necessary.
>
> Initialize the boot cpu when tsc frequency is determined. Copy the
> calculated data from the boot CPU to the other CPUs in tsc_init().
>
> In addition do the following:
>
> - Remove unnecessary zeroing of c2ns data by removing cyc2ns_data_init()
> - Split set_cyc2ns_scale() into two functions, so set_cyc2ns_scale() can be
> called when system is up, and wraps around __set_cyc2ns_scale() that can
> be called directly when system is booting but avoids saving restoring
> IRQs and going and waking up from idle.
>
> Suggested-by: Thomas Gleixner <tglx@...utronix.de>
> Signed-off-by: Pavel Tatashin <pasha.tatashin@...cle.com>
> ---
> arch/x86/kernel/tsc.c | 94 ++++++++++++++++++++++++-------------------
> 1 file changed, 53 insertions(+), 41 deletions(-)
>
> diff --git a/arch/x86/kernel/tsc.c b/arch/x86/kernel/tsc.c
> index bc8eb82050a3..0b1abe7fdd8e 100644
> --- a/arch/x86/kernel/tsc.c
> +++ b/arch/x86/kernel/tsc.c
> @@ -103,23 +103,6 @@ void cyc2ns_read_end(void)
> * -johnstul@...ibm.com "math is hard, lets go shopping!"
> */
>
> -static void cyc2ns_data_init(struct cyc2ns_data *data)
> -{
> - data->cyc2ns_mul = 0;
> - data->cyc2ns_shift = 0;
> - data->cyc2ns_offset = 0;
> -}
> -
> -static void __init cyc2ns_init(int cpu)
> -{
> - struct cyc2ns *c2n = &per_cpu(cyc2ns, cpu);
> -
> - cyc2ns_data_init(&c2n->data[0]);
> - cyc2ns_data_init(&c2n->data[1]);
> -
> - seqcount_init(&c2n->seq);
> -}
> -
> static inline unsigned long long cycles_2_ns(unsigned long long cyc)
> {
> struct cyc2ns_data data;
> @@ -135,18 +118,11 @@ static inline unsigned long long cycles_2_ns(unsigned long long cyc)
> 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);
>
> @@ -178,12 +154,55 @@ static void set_cyc2ns_scale(unsigned long khz, int cpu, unsigned long long tsc_
> 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);
> } >
> +/*
> + * Initialize cyc2ns for boot cpu
> + */
> +static void __init cyc2ns_init_boot_cpu(void)
> +{
> + struct cyc2ns *c2n = this_cpu_ptr(&cyc2ns);
> +
> + seqcount_init(&c2n->seq);
> + __set_cyc2ns_scale(tsc_khz, smp_processor_id(), rdtsc());
> +}
Hi Paval,
Here we didn't protect it with local_irq_save()/local_irq_restore()
> +
> +/*
> + * Secondary CPUs do not run through cyc2ns_init(), so set up
> + * all the scale factors for all CPUs, assuming the same
> + * speed as the bootup CPU. (cpufreq notifiers will fix this
> + * up if their speed diverges)
> + */
> +static void __init cyc2ns_init_secondary_cpus(void)
> +{
> + unsigned int cpu, this_cpu = smp_processor_id();
> + struct cyc2ns *c2n = this_cpu_ptr(&cyc2ns);
> + struct cyc2ns_data *data = c2n->data;
> +
> + for_each_possible_cpu(cpu) {
> + if (cpu != this_cpu) {
> + seqcount_init(&c2n->seq);
> + c2n = per_cpu_ptr(&cyc2ns, cpu);
> + c2n->data[0] = data[0];
> + c2n->data[1] = data[1];
> + }
> + }
> +}
> +
> /*
> * Scheduler clock - returns current time in nanosec units.
> */
> @@ -1385,6 +1404,10 @@ void __init tsc_early_init(void)
> if (!determine_cpu_tsc_frequncies())
> return;
> loops_per_jiffy = get_loops_per_jiffy();
> +
> + /* Sanitize TSC ADJUST before cyc2ns gets initialized */
> + tsc_store_and_check_tsc_adjust(true);
> + cyc2ns_init_boot_cpu();
> }
>
> void __init tsc_init(void)
> @@ -1400,23 +1423,12 @@ void __init tsc_init(void)
> mark_tsc_unstable("could not calculate TSC khz");
> return;
> }
> + /* Sanitize TSC ADJUST before cyc2ns gets initialized */
> + tsc_store_and_check_tsc_adjust(true);
> + cyc2ns_init_boot_cpu();
In tsc_init(), the timer interrupt has worked, Is that OK?
IMO, no need to change so much, just use the original code,
eg:
+static void __init cyc2ns_init_bsp(void)
+{
+ cyc2ns_init(0);
+ set_cyc2ns_scale(tsc_khz, 0, rdtsc());
+}
+
+static void __init cyc2ns_init_cpus(void)
+{
+ struct cyc2ns *c2n0 = &per_cpu(cyc2ns, 0);
+ struct cyc2ns *c2n;
+ unsigned int cpu;
+
+ for_each_possible_cpu(cpu) {
+ if (cpu != 0) {
+ cyc2ns_init(cpu);
+ c2n = &per_cpu(cyc2ns, cpu);
+ c2n->data[0] = c2n0->data[0];
+ c2n->data[1] = c2n0->data[1];
+ }
+ }
+}
+
Thanks,
dou
> }
>
> - /* Sanitize TSC ADJUST before cyc2ns gets initialized */
> - tsc_store_and_check_tsc_adjust(true);
> -
> - /*
> - * Secondary CPUs do not run through tsc_init(), so set up
> - * all the scale factors for all CPUs, assuming the same
> - * speed as the bootup CPU. (cpufreq notifiers will fix this
> - * up if their speed diverges)
> - */
> - cyc = rdtsc();
> - for_each_possible_cpu(cpu) {
> - cyc2ns_init(cpu);
> - set_cyc2ns_scale(tsc_khz, cpu, cyc);
> - }
> -
> + cyc2ns_init_secondary_cpus();
> static_branch_enable(&__use_tsc);
>
> if (!no_sched_irq_time)
>
Powered by blists - more mailing lists