On Fri, Jul 13, 2018 at 5:13 AM Dou Liyang <douly.fnst@xxxxxxxxxxxxxx> wrote:
>
>
>
> 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@xxxxxxxxxxxxx>
> > Signed-off-by: Pavel Tatashin <pasha.tatashin@xxxxxxxxxx>
> > ---
> > 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@xxxxxxxxxx "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()
Hi Dou,
Thank you for reviewing this work.
Correct, but no need to do that: we do local_irqsave()/restore() only
in set_cyc2ns_scale(), because that is called when system is live, and
frequency changes.
cyc2ns_init_boot_cpu(), and cyc2ns_init_secondary_cpus(), are called
during boot, before smp_init(), and frequency does not change during
that time. So, nothing to protect us from.
> In tsc_init(), the timer interrupt has worked, Is that OK?
>
> IMO, no need to change so much, just use the original code,
> eg:
The request and suggestions on how to cleanup cyc2ns came from Thomas.
Overall, code is cleaner now.
Thank you,
Pavel
--
To unsubscribe from this list: send the line "unsubscribe linux-s390" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
