| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Wed, 10 Jul 2013 11:29:45 +0800
From: Michael Wang <wangyun@...ux.vnet.ibm.com>
To: "Srivatsa S. Bhat" <srivatsa.bhat@...ux.vnet.ibm.com>
CC: Bartlomiej Zolnierkiewicz <b.zolnierkie@...sung.com>,
"Rafael J. Wysocki" <rafael.j.wysocki@...el.com>,
Viresh Kumar <viresh.kumar@...aro.org>,
Borislav Petkov <bp@...en8.de>, Jiri Kosina <jkosina@...e.cz>,
Tomasz Figa <t.figa@...sung.com>, linux-kernel@...r.kernel.org,
linux-pm@...r.kernel.org
Subject: Re: [v3.10 regression] deadlock on cpu hotplug
On 07/09/2013 09:07 PM, Srivatsa S. Bhat wrote:
[snip]
>>
>
> Yeah, exactly!
>
> So I had proposed doing an asynchronous cancel-work or doing the
> synchronous cancel-work in the CPU_POST_DEAD phase, where the
> cpu_hotplug.lock is not held. See this thread:
>
> http://marc.info/?l=linux-kernel&m=137241212616799&w=2
> http://marc.info/?l=linux-pm&m=137242906622537&w=2
>
> But now that I look at commit 2f7021a8 again, I still think we should
> revert it and fix the _actual_ root-cause of the bug.
Agree, or we could revert it with some better fix, otherwise the prev
bug report will back again...
>
> Cpufreq subsystem has enough synchronization to ensure that policy->cpus
> always contains online CPUs. And it also has the concept of cancelling
> queued work items, *before* that CPU is taken offline.
> So, where is the chance that we try to queue work items on offline CPUs?
>
> To answer that question, I was looking at the cpufreq code yesterday
> and found something very interesting: the gov_cancel_work() that is
> invoked before a CPU goes offline, can actually end up effectively
> *NOT* cancelling the queued work item!
>
> The reason is, the per-cpu work items are not just self-queueing (if
> that was the case, gov_cancel_work would have been successful without
> any doubt), but instead, they can also queue work items on *other* CPUs!
>
> Example from ondemand governor's per-cpu work item:
>
> static void od_dbs_timer(struct work_struct *work)
> {
> ...
> bool modify_all = true;
> ...
> gov_queue_work(dbs_data, dbs_info->cdbs.cur_policy, delay, modify_all);
> }
>
> So, every per-cpu work item can re-queue the work item on *many other*
> CPUs, and not just itself!
>
> So that leads to a race which makes gov_cancel_work() ineffective.
> The call to cancel_delayed_work_sync() will cancel all pending work items
> on say CPU 3 (which is going down), but immediately after that, say CPU4's
> work item fires and queues the work item on CPU4 as well as CPU3. Thus,
> gov_cancel_work() _effectively_ didn't do anything useful.
That's interesting, sense like a little closer to the root, the timer is
supposed to stop but failed... I need some investigation here...
Regards,
Michael Wang
>
> But this still doesn't immediately explain how we can end up trying to
> queue work items on offline CPUs (since policy->cpus is supposed to always
> contain online cpus only, and this does look correct in the code as well,
> at a first glance). But I just wanted to share this finding, in case it
> helps us find out the real root-cause.
>
> Also, you might perhaps want to try the (untested) patch shown below, and
> see if it resolves your problem. It basically makes work-items requeue
> themselves on only their respective CPUs and not others, so that
> gov_cancel_work succeeds in its mission. However, I guess the patch is
> wrong from a cpufreq perspective, in case cpufreq really depends on the
> "requeue-work-on-everybody" model.
>
> Regards,
> Srivatsa S. Bhat
>
> ------------------------------------------------------------------------
>
> drivers/cpufreq/cpufreq_conservative.c | 2 +-
> drivers/cpufreq/cpufreq_governor.c | 2 --
> drivers/cpufreq/cpufreq_ondemand.c | 2 +-
> 3 files changed, 2 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/cpufreq/cpufreq_conservative.c b/drivers/cpufreq/cpufreq_conservative.c
> index 0ceb2ef..bbfc1dd 100644
> --- a/drivers/cpufreq/cpufreq_conservative.c
> +++ b/drivers/cpufreq/cpufreq_conservative.c
> @@ -120,7 +120,7 @@ static void cs_dbs_timer(struct work_struct *work)
> struct dbs_data *dbs_data = dbs_info->cdbs.cur_policy->governor_data;
> struct cs_dbs_tuners *cs_tuners = dbs_data->tuners;
> int delay = delay_for_sampling_rate(cs_tuners->sampling_rate);
> - bool modify_all = true;
> + bool modify_all = false;
>
> mutex_lock(&core_dbs_info->cdbs.timer_mutex);
> if (!need_load_eval(&core_dbs_info->cdbs, cs_tuners->sampling_rate))
> diff --git a/drivers/cpufreq/cpufreq_governor.c b/drivers/cpufreq/cpufreq_governor.c
> index 4645876..ec4baeb 100644
> --- a/drivers/cpufreq/cpufreq_governor.c
> +++ b/drivers/cpufreq/cpufreq_governor.c
> @@ -137,10 +137,8 @@ void gov_queue_work(struct dbs_data *dbs_data, struct cpufreq_policy *policy,
> 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);
> diff --git a/drivers/cpufreq/cpufreq_ondemand.c b/drivers/cpufreq/cpufreq_ondemand.c
> index 93eb5cb..241ebc0 100644
> --- a/drivers/cpufreq/cpufreq_ondemand.c
> +++ b/drivers/cpufreq/cpufreq_ondemand.c
> @@ -230,7 +230,7 @@ static void od_dbs_timer(struct work_struct *work)
> struct dbs_data *dbs_data = dbs_info->cdbs.cur_policy->governor_data;
> struct od_dbs_tuners *od_tuners = dbs_data->tuners;
> int delay = 0, sample_type = core_dbs_info->sample_type;
> - bool modify_all = true;
> + bool modify_all = false;
>
> mutex_lock(&core_dbs_info->cdbs.timer_mutex);
> if (!need_load_eval(&core_dbs_info->cdbs, od_tuners->sampling_rate)) {
>
>
>
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@...r.kernel.org
> 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@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists