Re: [LOCKDEP] cpufreq: possible circular locking dependency detected

From: Michael Wang
Date: Thu Jul 11 2013 - 04:49:18 EST

On 07/11/2013 04:47 PM, Michael Wang wrote:
> On 07/11/2013 04:22 PM, Sergey Senozhatsky wrote:
> [snip]
>>>
>>
>> Hello Michael,
>> nice job! works fine for me.
>>
>> Reported-and-Tested-by: Sergey Senozhatsky <sergey.senozhatsky@xxxxxxxxx>
>
> Thanks for the test :)
>
> Borislav may also doing some testing, let's wait for few days and see
> whether there are any point we missed.

s /Borislav/Bartlomiej

>
> And we should also thanks Srivatsa for catching the root issue ;-)
>
> Regards,
> Michael Wang
>
>>
>>
>> -ss
>>
>>> Regards,
>>> Michael Wang
>>>
>>> diff --git a/drivers/cpufreq/cpufreq_governor.c b/drivers/cpufreq/cpufreq_governor.c
>>> index dc9b72e..a64b544 100644
>>> --- a/drivers/cpufreq/cpufreq_governor.c
>>> +++ b/drivers/cpufreq/cpufreq_governor.c
>>> @@ -178,13 +178,14 @@ void gov_queue_work(struct dbs_data *dbs_data, struct cpufreq_policy *policy,
>>> {
>>> int i;
>>>
>>> + if (dbs_data->queue_stop)
>>> + return;
>>> +
>>> if (!all_cpus) {
>>> __gov_queue_work(smp_processor_id(), dbs_data, delay);
>>> } else {
>>> - get_online_cpus();
>>> for_each_cpu(i, policy->cpus)
>>> __gov_queue_work(i, dbs_data, delay);
>>> - put_online_cpus();
>>> }
>>> }
>>> EXPORT_SYMBOL_GPL(gov_queue_work);
>>> @@ -193,12 +194,27 @@ static inline void gov_cancel_work(struct dbs_data *dbs_data,
>>> struct cpufreq_policy *policy)
>>> {
>>> struct cpu_dbs_common_info *cdbs;
>>> - int i;
>>> + int i, round = 2;
>>>
>>> + dbs_data->queue_stop = 1;
>>> +redo:
>>> + round--;
>>> for_each_cpu(i, policy->cpus) {
>>> cdbs = dbs_data->cdata->get_cpu_cdbs(i);
>>> cancel_delayed_work_sync(&cdbs->work);
>>> }
>>> +
>>> + /*
>>> + * Since there is no lock to prvent re-queue the
>>> + * cancelled work, some early cancelled work might
>>> + * have been queued again by later cancelled work.
>>> + *
>>> + * Flush the work again with dbs_data->queue_stop
>>> + * enabled, this time there will be no survivors.
>>> + */
>>> + if (round)
>>> + goto redo;
>>> + dbs_data->queue_stop = 0;
>>> }
>>>
>>> /* Will return if we need to evaluate cpu load again or not */
>>> diff --git a/drivers/cpufreq/cpufreq_governor.h b/drivers/cpufreq/cpufreq_governor.h
>>> index e16a961..9116135 100644
>>> --- a/drivers/cpufreq/cpufreq_governor.h
>>> +++ b/drivers/cpufreq/cpufreq_governor.h
>>> @@ -213,6 +213,7 @@ struct dbs_data {
>>> unsigned int min_sampling_rate;
>>> int usage_count;
>>> void *tuners;
>>> + int queue_stop;
>>>
>>> /* dbs_mutex protects dbs_enable in governor start/stop */
>>> struct mutex mutex;
>>>
>>>>
>>>> Signed-off-by: Sergey Senozhatsky <sergey.senozhatsky@xxxxxxxxx>
>>>>
>>>> ---
>>>>
>>>> drivers/cpufreq/cpufreq.c | 5 +----
>>>> drivers/cpufreq/cpufreq_governor.c | 17 +++++++++++------
>>>> drivers/cpufreq/cpufreq_stats.c | 2 +-
>>>> 3 files changed, 13 insertions(+), 11 deletions(-)
>>>>
>>>> diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
>>>> index 6a015ad..f8aacf1 100644
>>>> --- a/drivers/cpufreq/cpufreq.c
>>>> +++ b/drivers/cpufreq/cpufreq.c
>>>> @@ -1943,13 +1943,10 @@ static int __cpuinit cpufreq_cpu_callback(struct notifier_block *nfb,
>>>> case CPU_ONLINE:
>>>> cpufreq_add_dev(dev, NULL);
>>>> break;
>>>> - case CPU_DOWN_PREPARE:
>>>> + case CPU_POST_DEAD:
>>>> case CPU_UP_CANCELED_FROZEN:
>>>> __cpufreq_remove_dev(dev, NULL);
>>>> break;
>>>> - case CPU_DOWN_FAILED:
>>>> - cpufreq_add_dev(dev, NULL);
>>>> - break;
>>>> }
>>>> }
>>>> return NOTIFY_OK;
>>>> diff --git a/drivers/cpufreq/cpufreq_governor.c b/drivers/cpufreq/cpufreq_governor.c
>>>> index 4645876..681d5d6 100644
>>>> --- a/drivers/cpufreq/cpufreq_governor.c
>>>> +++ b/drivers/cpufreq/cpufreq_governor.c
>>>> @@ -125,7 +125,11 @@ static inline void __gov_queue_work(int cpu, struct dbs_data *dbs_data,
>>>> unsigned int delay)
>>>> {
>>>> struct cpu_dbs_common_info *cdbs = dbs_data->cdata->get_cpu_cdbs(cpu);
>>>> -
>>>> + /* cpu offline might block existing gov_queue_work() user,
>>>> + * unblocking it after CPU_DEAD and before CPU_POST_DEAD.
>>>> + * thus potentially we can hit offlined CPU */
>>>> + if (unlikely(cpu_is_offline(cpu)))
>>>> + return;
>>>> mod_delayed_work_on(cpu, system_wq, &cdbs->work, delay);
>>>> }
>>>>
>>>> @@ -133,15 +137,14 @@ void gov_queue_work(struct dbs_data *dbs_data, struct cpufreq_policy *policy,
>>>> unsigned int delay, bool all_cpus)
>>>> {
>>>> int i;
>>>> -
>>>> + get_online_cpus();
>>>> if (!all_cpus) {
>>>> __gov_queue_work(smp_processor_id(), dbs_data, delay);
>>>> } else {
>>>> - get_online_cpus();
>>>> for_each_cpu(i, policy->cpus)
>>>> __gov_queue_work(i, dbs_data, delay);
>>>> - put_online_cpus();
>>>> }
>>>> + put_online_cpus();
>>>> }
>>>> EXPORT_SYMBOL_GPL(gov_queue_work);
>>>>
>>>> @@ -354,8 +357,10 @@ int cpufreq_governor_dbs(struct cpufreq_policy *policy,
>>>> /* Initiate timer time stamp */
>>>> cpu_cdbs->time_stamp = ktime_get();
>>>>
>>>> - gov_queue_work(dbs_data, policy,
>>>> - delay_for_sampling_rate(sampling_rate), true);
>>>> + /* hotplug lock already held */
>>>> + for_each_cpu(j, policy->cpus)
>>>> + __gov_queue_work(j, dbs_data,
>>>> + delay_for_sampling_rate(sampling_rate));
>>>> break;
>>>>
>>>> case CPUFREQ_GOV_STOP:
>>>> diff --git a/drivers/cpufreq/cpufreq_stats.c b/drivers/cpufreq/cpufreq_stats.c
>>>> index cd9e817..833816e 100644
>>>> --- a/drivers/cpufreq/cpufreq_stats.c
>>>> +++ b/drivers/cpufreq/cpufreq_stats.c
>>>> @@ -355,7 +355,7 @@ static int __cpuinit cpufreq_stat_cpu_callback(struct notifier_block *nfb,
>>>> case CPU_DOWN_PREPARE:
>>>> cpufreq_stats_free_sysfs(cpu);
>>>> break;
>>>> - case CPU_DEAD:
>>>> + case CPU_POST_DEAD:
>>>> cpufreq_stats_free_table(cpu);
>>>> break;
>>>> case CPU_UP_CANCELED_FROZEN:
>>>> --
>>>> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
>>>> the body of a message to majordomo@xxxxxxxxxxxxxxx
>>>> More majordomo info at http://vger.kernel.org/majordomo-info.html
>>>> Please read the FAQ at http://www.tux.org/lkml/
>>>>
>>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
>> the body of a message to majordomo@xxxxxxxxxxxxxxx
>> More majordomo info at http://vger.kernel.org/majordomo-info.html
>> Please read the FAQ at http://www.tux.org/lkml/
>>
>

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/