linux-kernel - Re: [PATCH V3] cpufreq: Call transition notifier only once for each policy

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 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, 22 Mar 2019 10:55:59 +0100
From:   "Rafael J. Wysocki" <rafael@...nel.org>
To:     Viresh Kumar <viresh.kumar@...aro.org>
Cc:     Thomas Gleixner <tglx@...utronix.de>,
        Rafael Wysocki <rjw@...ysocki.net>,
        Peter Zijlstra <peterz@...radead.org>,
        Russell King <linux@...linux.org.uk>,
        "David S. Miller" <davem@...emloft.net>,
        Ingo Molnar <mingo@...hat.com>, Borislav Petkov <bp@...en8.de>,
        "H. Peter Anvin" <hpa@...or.com>,
        "the arch/x86 maintainers" <x86@...nel.org>,
        Paolo Bonzini <pbonzini@...hat.com>,
        Radim Krčmář <rkrcmar@...hat.com>,
        Linux PM <linux-pm@...r.kernel.org>,
        Vincent Guittot <vincent.guittot@...aro.org>,
        Linux ARM <linux-arm-kernel@...ts.infradead.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        sparclinux@...r.kernel.org, kvm@...r.kernel.org
Subject: Re: [PATCH V3] cpufreq: Call transition notifier only once for each policy

On Fri, Mar 22, 2019 at 7:28 AM Viresh Kumar <viresh.kumar@...aro.org> wrote:
>
> On 21-03-19, 16:49, Thomas Gleixner wrote:
> > On Wed, 20 Mar 2019, Viresh Kumar wrote:
> > >
> > > diff --git a/arch/x86/kernel/tsc.c b/arch/x86/kernel/tsc.c
> > > index 3fae23834069..b2fe665878f7 100644
> > > --- a/arch/x86/kernel/tsc.c
> > > +++ b/arch/x86/kernel/tsc.c
> > > @@ -958,10 +958,15 @@ static int time_cpufreq_notifier(struct notifier_block *nb, unsigned long val,
> > >     struct cpufreq_freqs *freq = data;
> > >     unsigned long *lpj;
> > >
> > > +   if (WARN_ON_ONCE(cpumask_weight(freq->policy->related_cpus) != 1)) {
> > > +           mark_tsc_unstable("cpufreq changes: related CPUs affected");
> > > +           return 0;
> > > +   }
> >
> > You might add a check which ensures that policy->cpu == smp_processor_id()
> > because if this is not the case ....
>
> How about something like this ?
>
>         if (WARN_ON_ONCE(cpumask_weight(freq->policy->related_cpus) != 1 ||
>                          freq->policy->cpu != smp_processor_id())) {
>                 mark_tsc_unstable("cpufreq changes: related CPUs affected");
>                 return 0;
>         }
>
>
> Thanks for your feedback.

Peter suggested something like this IIRC.

Anyway, I'm still concerned that this approach in general
fundamentally doesn't work on SMP with frequency synchronization,
which is the case for the platforms affected by the problem it
attempts to overcome.

The frequency has just been changed on one CPU, presumably to the
requested value (so this cannot work when turbo is enabled anyway),
but then it also has changed for all of the other CPUs in the system
(or at least in the package), so it is not sufficient to update the
single CPU here as it is only a messenger, so to speak.  However,
updating the other CPUs from here would be fundamentally racy AFAICS.