Re: [PATCH v2] cpufreq: stats: Walk online CPUs with CPU offline/online locked

[Viresh Kumar]

On 23-05-16, 15:40, Rafael J. Wysocki wrote:
> On Monday, May 23, 2016 09:27:03 AM Viresh Kumar wrote:
> > On 20-05-16, 23:33, Rafael J. Wysocki wrote:
> > > The policy rwsem is really only needed in cpufreq_stats_create_table(), because
> > > the policy notifier is gone when _free_table() runs, so another version of the
> > > patch goes below.
> > 
> > Right. I saw that while reading your previous version but didn't reply
> > because I wanted to do a more careful review.
> > 
> > The first issue I have here is that the _init and _exit routines in
> > cpufreq-stats aren't opposite of each other. Which shouldn't be the
> > case.
> 
> I'm not sure what you mean here.

Sorry about that. I meant that exit() should look opposite of init() ideally,
whereas if you look at current code, both are (un)registering the
POLICY_NOTIFIER at the top.

> > I am still trying to understand why we will ever have a race here. We
> > might have it, but I just want to know how.
> > 
> > This is what we do in on addition of a policy:
> > - send the CREATE notifier
> > - Add policy to the list
> > 
> > So, the notifiers are guaranteed to complete before the policy is
> > present in the list.
> > 
> > CPU 0                                   CPU 1
> > notifier                                cpufreq_stats_init()
> > CREATE-POLICY X                         cpufreq_stats_create_table()
> > __cpufreq_stats_create_table()          cpufreq_cpu_get()
> > 
> > AFAICT, whatever may happen, __cpufreq_stats_create_table() will *not*
> > get called in parallel for the same policy.
> > 
> > If __cpufreq_stats_create_table() is in progress on CPU0, CPU 1 will
> > not find the policy with cpufreq_cpu_get(). And if cpufreq_cpu_get()
> > finds a policy, the notifier would already have completed.
> > 
> > What do you say ?

Until now I thought you are trying to prevent the race where
__cpufreq_stats_create_table() gets called in parallel for the same policy. So,
above explains that it can't happen for sure.

> Say cpufreq_stats_init() runs in parallel with a CPU online (say someone
> loads the cpufreq_stats module and a CPU goes online at the same time,
> not likely to happen, but still possible).

Of course, that will be a design problem if it ever happens. I agree.

> Then, the notifier may get invoked when the loop is in progress and because the
> CPU is added to policy->cpus (and the CPU's per-CPU pointer is set to it) before
> invoking the notifier, cpufreq_stats_init() may get the policy pointer for a
> policy that hasn't been initialized completely yet and then run in parallel with
> the notifier for that policy.

If the policy isn't initialized fully before its added to the list, then that's
a problem in cpufreq.c I would say.

But, I don't see a problem here. The policy's kobject, etc gets initialized
fully before its added to the list or the notifier is sent for CREATE_POLICY.
Just that the governor isn't set properly, nothing else.

And if you think about it the other way round, we are sending the CREATE_POLICY
notifier right at that point where we add it to the list, and the cpufreq-stats
layer is expected to work on the policy right from that call. So, it is fully
initialized from the perspective of the stats layer. Nothing should go wrong.

-- 
viresh