On Fri, Mar 22, 2019 at 7:28 AM Viresh Kumar <viresh.kumar@xxxxxxxxxx> 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.
