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

[Peng Fan <peng.fan@nxp.com>]

> Subject: Re: About CPU hot-plug stress test failed in cpufreq driver
> 
> On Tue, Dec 10, 2019 at 9:29 AM Anson Huang <anson.huang@nxp.com>
> wrote:
> >
> >
> >
> > > -----Original Message-----
> > > From: Rafael J. Wysocki <rafael@kernel.org>
> > > Sent: Tuesday, December 10, 2019 4:22 PM
> > > To: Viresh Kumar <viresh.kumar@linaro.org>
> > > Cc: Peng Fan <peng.fan@nxp.com>; Rafael J. Wysocki
> > > <rafael@kernel.org>; Anson Huang <anson.huang@nxp.com>; Rafael J.
> > > Wysocki <rjw@rjwysocki.net>; Jacky Bai <ping.bai@nxp.com>; linux-
> > > pm@vger.kernel.org; Vincent Guittot <vincent.guittot@linaro.org>;
> > > Peter Zijlstra <peterz@infradead.org>; Paul McKenney
> > > <paulmck@linux.vnet.ibm.com>
> > > Subject: Re: About CPU hot-plug stress test failed in cpufreq driver
> > >
> > > 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://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%
> > > > > 2Fel
> > > > > ixir.bootlin.com%2Flinux%2Fv5.5-
> > > rc1%2Fsource%2Fkernel%2Fsched%2Fsche
> > > > >
> > >
> d.h%23L2293&amp;data=02%7C01%7Canson.huang%40nxp.com%7C6f4490
> 0
> > > be3404
> > > > >
> > >
> e7d355708d77d4a16fa%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0
> %
> > > 7C637
> > > > >
> > >
> 115629475456329&amp;sdata=XXhwvuTOBb3TLmerwkr1zKbaWNA8xA%2Bl
> > > W%2Faw31
> > > > > 0AYcM%3D&amp;reserved=0
> > > > > 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
> > > policy->happen as
> > > long as RCU works as expected.
> >
> > If I understand correctly, the governor hook ONLY be clear on the CPU
> > being offline and after governor stopped, but the CPU being offline
> > could still run into below function to help other CPU update the util,
> > and it ONLY checks the cpu_of(rq)'s governor hook which is valid as that
> CPU is online.
> >
> > So the question is how to avoid the CPU being offline and already
> > finish the governor stop flow be scheduled to help other CPU update the
> util.
> >
> >  static inline void cpufreq_update_util(struct rq *rq, unsigned int
> > flags)  {
> >          struct update_util_data *data;
> >
> >          data =
> rcu_dereference_sched(*per_cpu_ptr(&cpufreq_update_util_data,
> >
> cpu_of(rq)));
> >          if (data)
> >                  data->func(data, rq_clock(rq), flags);  }
> 
> OK, so that's where the problem is, good catch!
> 
> So what happens is that a CPU going offline runs some scheduler code that
> invokes cpufreq_update_util().  Incidentally, it is not the cpu_of(rq), but that
> CPU is still online, so the callback is invoked and then policy->cpus test is
> bypassed because of dvfs_possible_from_any_cpu.

gov_clear_update_util(policy_dbs->policy)->sync rcu; should wait rcu_derefence_sched,
right? or I understand wrong?

Thanks,
Peng.