Re: About CPU hot-plug stress test failed in cpufreq driver

["Rafael J. Wysocki" <rafael@kernel.org>]

On Tue, Dec 10, 2019 at 8:05 AM Viresh Kumar <viresh.kumar@linaro.org> wrote:
>
> +few more guys
>
> On 10-12-19, 05:53, Peng Fan wrote:
> > But per https://elixir.bootlin.com/linux/v5.5-rc1/source/kernel/sched/sched.h#L2293
> > cpu_of(rq) and smp_processor_id() is possible to not the same,
> >
> > When cpu_of(rq) is not equal to smp_processor_id(), dbs_update_util_handler
> > will use irq_work_queue to smp_processor_id(), not cpu_of(rq). Is this
> > expected?
> > Or should the irq_work be queued to cpu_of(rq)?
>
> Okay, sorry for the long weekend where I couldn't get time to reply at all.

No worries. :-)

> First of all, lets try to understand dvfs_possible_from_any_cpu.
>
> Who can update the frequency of a CPU ? For many architectures/platforms the
> eventual code that writes to some register to change the frequency should only
> run on the local CPU, as these registers are per-cpu registers and not something
> shared between CPUs.
>
> But for the ARM architecture, we have a PLL and then some more registers to play
> with the clk provided to the CPU blocks and these registers (which are updated
> as a result of clk_set_rate()) are part of a block outside of the CPU blocks.
> And so any CPU (even if it is not part of the same cpufreq policy) can update
> it. Setting this flag allows that and eventually we may end up updating the
> frequency sooner, instead of later (which may be less effective). That was the
> idea of the remote-wakeup series. This stuff is absolutely correct and so
> cpufreq-dt does it for everyone.
>
> This also means that the normal work and irq-work both can run on any CPU for
> your platform and it should be okay to do that.

And it the failing case all of the CPUs in the system are in the same
policy anyway, so dvfs_possible_from_any_cpu is a red herring.

> Now, we have necessary measures in place to make sure that after stopping and
> before starting a governor, the scheduler hooks to save the cpufreq governor
> pointer and updates to policy->cpus are made properly, to make sure that we
> never ever schedule a work or irq-work on a CPU which is offline. Now it looks
> like this isn't working as expected and we need to find what exactly is broken
> here.
>
> And yes, I did the testing on Hikey 620, an octa-core ARM platform which has a
> single cpufreq policy which has all the 8 CPUs. And yes, I am using cpufreq-dt
> only and I wasn't able to reproduce the problem with mainline kernel as I
> explained earlier.
>
> The problem is somewhere between the scheduler's governor hook running or
> queuing work on a CPU which is in the middle of getting offline/online and there
> is some race around that. The problem hence may not be related to just cpufreq,
> but a wider variety of clients.

The problem is that a CPU is running a governor hook which it
shouldn't be running at all.

The observation that dvfs_possible_from_any_cpu makes a difference
only means that the governor hook is running on a CPU that is not
present in the policy->cpus mask.  On the platform(s) in question this
cannot happen as long as RCU works as expected.