Re: [PATCH V2] cpufreq: Call transition notifier only once for each policy

From: Viresh Kumar <viresh.kumar@linaro.org>
To: "Rafael J. Wysocki" <rafael@kernel.org>
Cc: "Rafael Wysocki" <rjw@rjwysocki.net>,
	"Russell King" <linux@armlinux.org.uk>,
	"David S. Miller" <davem@davemloft.net>,
	"Thomas Gleixner" <tglx@linutronix.de>,
	"Ingo Molnar" <mingo@redhat.com>,
	"Borislav Petkov" <bp@alien8.de>,
	"H. Peter Anvin" <hpa@zytor.com>,
	"the arch/x86 maintainers" <x86@kernel.org>,
	"Paolo Bonzini" <pbonzini@redhat.com>,
	"Radim Krčmář" <rkrcmar@redhat.com>,
	"Linux PM" <linux-pm@vger.kernel.org>,
	"Vincent Guittot" <vincent.guittot@linaro.org>,
	"Linux ARM" <linux-arm-kernel@lists.infradead.org>,
	"Linux Kernel Mailing List" <linux-kernel@vger.kernel.org>,
	sparclinux@vger.kernel.org, kvm@vger.kernel.org
Subject: Re: [PATCH V2] cpufreq: Call transition notifier only once for each policy
Date: Tue, 19 Mar 2019 16:19:33 +0530	[thread overview]
Message-ID: <20190319104933.vj6zcscubyp4gwxq@vireshk-i7> (raw)
In-Reply-To: <CAJZ5v0gzjXNizE=e+AdqMdJvE_-wYCq0kvzB-thE=SufKfyRrQ@mail.gmail.com>

On 19-03-19, 10:41, Rafael J. Wysocki wrote:
> On Tue, Mar 19, 2019 at 6:50 AM Viresh Kumar <viresh.kumar@linaro.org> wrote:
> >
> > On 18-03-19, 12:49, Rafael J. Wysocki wrote:
> > > To summarize, I think that it would be sufficient to do this just for
> > > policy->cpu and, as Peter said, warn once if there are more CPUs in
> > > the policy or policy->cpu is not the CPU running this code.  And mark
> > > the TSC as unstable in both of these cases.
> >
> > How about this ?
> 
> We guarantee that this will always run on policy->cpu IIRC, so it LGTM

Yeah, the governor guarantees that unless dvfs_possible_from_any_cpu is set for
the policy. But there are few direct invocations to cpufreq_driver_target() and
__cpufreq_driver_target() which don't take that into account. First one is done
from cpufreq_online(), which can get called on any CPU I believe. Other one is
from cpufreq_generic_suspend(). But I think none of them gets called for x86 and
so below code should be safe.

> overall, but the new message is missing "one".

Talking about print message ?

> > diff --git a/arch/x86/kernel/tsc.c b/arch/x86/kernel/tsc.c
> > index 3fae23834069..4d3681cfb6e0 100644
> > --- a/arch/x86/kernel/tsc.c
> > +++ b/arch/x86/kernel/tsc.c
> > @@ -958,10 +958,13 @@ 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 policy has more than CPU");
> 
> Also I would check policy->cpus here.  After all, we don't care about
> CPUs that are never online.

Because the CPU isn't in the policy->cpus mask, we can't say it was *never*
online. Just that it isn't online at that moment of time. I used related_cpus as
the code here should be robust against any corner cases and shouldn't have
different behavior based on value of policy->cpus.

If the cpufreq driver is probed after all but one CPUs of a policy are offlined,
then you won't see the warning if policy->cpus is used. But the warning will
appear if any other CPU is onlined. For me that is wrong, we should have got the
warning earlier as well as it was just wrong to not warn earlier.

> And the message could be something like "cpufreq changes: related CPUs
> affected".

Sure.

I also forgot to add a "return" statement here. We shouldn't continue in this
case, right ?

> > +
> >         lpj = &boot_cpu_data.loops_per_jiffy;
> >  #ifdef CONFIG_SMP
> >         if (!(freq->flags & CPUFREQ_CONST_LOOPS))
> > -               lpj = &cpu_data(freq->cpu).loops_per_jiffy;
> > +               lpj = &cpu_data(freq->policy->cpu).loops_per_jiffy;
> >  #endif
> >
> >         if (!ref_freq) {
> > @@ -977,7 +980,7 @@ static int time_cpufreq_notifier(struct notifier_block *nb, unsigned long val,
> >                 if (!(freq->flags & CPUFREQ_CONST_LOOPS))
> >                         mark_tsc_unstable("cpufreq changes");
> >
> > -               set_cyc2ns_scale(tsc_khz, freq->cpu, rdtsc());
> > +               set_cyc2ns_scale(tsc_khz, freq->policy->cpu, rdtsc());
> >         }
> >
> >         return 0;

-- 
viresh