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

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

On Tue, Dec 10, 2019 at 6:53 AM Peng Fan <peng.fan@nxp.com> wrote:
>
> > Subject: Re: About CPU hot-plug stress test failed in cpufreq driver
> >
> > On Mon, Dec 9, 2019 at 1:32 PM Anson Huang <anson.huang@nxp.com>
> > wrote:
> > >
> > >
> > >
> > > From Anson's iPhone 6
> > >
> > >
> > > > 在 2019年12月9日，19:23，Rafael J. Wysocki <rafael@kernel.org> 写
> > 道：
> > > >
> > > >> On Mon, Dec 9, 2019 at 11:57 AM Anson Huang <anson.huang@nxp.com>
> > wrote:
> > > >>
> > > >> Forgot to mentioned that below patch on v5.4 can easily reproduce the
> > panic() on our platforms which I think is unexpected, as the policy->cpus
> > already be updated after governor stop, but still try to have irq work queued
> > on it.
> > > >>
> > > >> static void dbs_update_util_handler(struct update_util_data *data,
> > > >> u64 time, unsigned int flags)
> > > >> +       if (!cpumask_test_cpu(smp_processor_id(),
> > policy_dbs->policy->cpus))
> > > >> +               panic("...irq work on offline cpu %d\n",
> > > >> + smp_processor_id());
> > > >>        irq_work_queue(&policy_dbs->irq_work);
> > > >
> > > > Yes, that is unexpected.
> > > >
> > > > In cpufreq_offline(), we have:
> > > >
> > > >    down_write(&policy->rwsem);
> > > >    if (has_target())
> > > >        cpufreq_stop_governor(policy);
> > > >
> > > >    cpumask_clear_cpu(cpu, policy->cpus);
> > > >
> > > > and cpufreq_stop_governor() calls policy->governor->stop(policy)
> > > > which is cpufreq_dbs_governor_stop().
> > > >
> > > > That calls gov_clear_update_util(policy_dbs->policy) first, which
> > > > invokes cpufreq_remove_update_util_hook() for each CPU in
> > > > policy->cpus and synchronizes RCU, so after that point none of the
> > > > policy->cpus is expected to run dbs_update_util_handler().
> > > >
> > > > policy->cpus is updated next and the governor is started again with
> > > > the new policy->cpus.  Because the offline CPU is not there, it is
> > > > not expected to run dbs_update_util_handler() again.
> > > >
> > > > Do you only get the original error when one of the CPUs goes back online?
> > >
> > > No, sometimes I also got this error during a CPU is being offline.
> > >
> > > But the point is NOT that dbs_update_util_handler() called during
> > > governor stop, it is that this function is running on a CPU which
> > > already finish the governor stop function,
> >
> > Yes, it is, and which should not be possible as per the above.
> >
> > The offline CPU is not there in prolicy->cpus when
> > cpufreq_dbs_governor_start() is called for the policy, so its
> > cpufreq_update_util_data pointer is not set (it is NULL at that time).
> > Therefore it is not expected to run dbs_update_util_handler() until it is turn
> > back online.
> >
> > > I thought the original expectation is that this function ONLY be executed on
> > the CPU which needs scaling frequency?
> > > Is this correct?
> >
> > Yes, it is.
> >
> > > v4.19 follows this expectation while v5.4 is NOT.
> >
> > As per the kernel code, they both do.
>
> 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?

Yes, it is, in general.