On 07/11/2013 07:47 PM, Bartlomiej Zolnierkiewicz wrote:
[snip]
>
> Michael's patch also works for me. Thanks to everyone involved!
> (My only nitpick for the patch is that ->queue_stop can be made bool.)
>
> Reported-and-Tested-by: Bartlomiej Zolnierkiewicz <b.zolnierkie@xxxxxxxxxxx>
>
> I think that it would also be helpful if Jiri or Borislav could test
> the patch and see if it really works for them and fixes the original
> warning they were experiencing on x86.
Thanks for the testing :)
I plan to send out the formal patch next week, so Jiri and Borislav
would have chance to join the discussion.
Regards,
Michael Wang
>
> Best regards,
> --
> Bartlomiej Zolnierkiewicz
> Samsung R&D Institute Poland
> Samsung Electronics
>
>>> 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 cpufreq" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
