linux-kernel - Re: [PATCH v13 14/18] x86/tsc: initialize cyc2ns when tsc freq. is determined

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Fri, 13 Jul 2018 07:39:02 -0400
From:   Pavel Tatashin <pasha.tatashin@...cle.com>
To:     douly.fnst@...fujitsu.com
Cc:     Steven Sistare <steven.sistare@...cle.com>,
        Daniel Jordan <daniel.m.jordan@...cle.com>,
        linux@...linux.org.uk, schwidefsky@...ibm.com,
        Heiko Carstens <heiko.carstens@...ibm.com>,
        John Stultz <john.stultz@...aro.org>, sboyd@...eaurora.org,
        x86@...nel.org, LKML <linux-kernel@...r.kernel.org>,
        mingo@...hat.com, tglx@...utronix.de, hpa@...or.com,
        peterz@...radead.org, prarit@...hat.com, feng.tang@...el.com,
        Petr Mladek <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

On Fri, Jul 13, 2018 at 5:13 AM Dou Liyang <douly.fnst@...fujitsu.com> 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@...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()

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