All of lore.kernel.org
 help / color / mirror / Atom feed
From: venkatesh.pallipadi-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org
To: Dave Jones <davej-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
Cc: linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
	cpufreq-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
	kernel-testers-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
	Ingo Molnar <mingo-X9Un+BFzKDI@public.gmane.org>,
	"Rafael J. Wysocki" <rjw-KKrjLPT3xs0@public.gmane.org>,
	Dave Young
	<hidave.darkstar-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>,
	Pekka Enberg <penberg-bbCR+/B0CizivPeTLB3BmA@public.gmane.org>,
	Mathieu Desnoyers
	<mathieu.desnoyers-scC8bbJcJLCw5LPnMra/2Q@public.gmane.org>,
	Thomas Renninger <trenn-l3A5Bk7waGM@public.gmane.org>,
	Venkatesh Pallipadi
	<venkatesh.pallipadi-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
Subject: [patch 1/3] cpufreq: remove rwsem lock from CPUFREQ_GOV_STOP call (second call site)
Date: Thu, 25 Jun 2009 11:33:55 -0700	[thread overview]
Message-ID: <20090625183601.300706000@intel.com> (raw)
In-Reply-To: 20090625183354.491259000@intel.com

[-- Attachment #1: 0001-remove-rwsem-lock-from-CPUFREQ_GOV_STOP-call-second.patch --]
[-- Type: text/plain, Size: 2289 bytes --]

From: Mathieu Desnoyers <mathieu.desnoyers-scC8bbJcJLCw5LPnMra/2Q@public.gmane.org>

commit	42a06f2166f2f6f7bf04f32b4e823eacdceafdc9

Missed a call site for CPUFREQ_GOV_STOP to remove the rwlock taken around the
teardown. To make a long story short, the rwlock write-lock causes a circular
dependency with cancel_delayed_work_sync(), because the timer handler takes the
write lock.

Note that all callers to __cpufreq_set_policy are taking the rwsem. All sysfs
callers (writers) hold the write rwsem at the earliest sysfs calling stage.

However, the rwlock write-lock is not needed upon governor stop.

Change :
- Added comment from Venki at lock definition site.

Signed-off-by: Mathieu Desnoyers <mathieu.desnoyers-scC8bbJcJLCw5LPnMra/2Q@public.gmane.org>
Signed-off-by: Thomas Renninger <trenn-l3A5Bk7waGM@public.gmane.org>
Signed-off-by: Venkatesh Pallipadi <venkatesh.pallipadi-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>

---
 drivers/cpufreq/cpufreq.c |   13 ++++++++++++-
 1 files changed, 12 insertions(+), 1 deletions(-)

diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
index 6e2ec0b..728656c 100644
--- a/drivers/cpufreq/cpufreq.c
+++ b/drivers/cpufreq/cpufreq.c
@@ -61,6 +61,8 @@ static DEFINE_SPINLOCK(cpufreq_driver_lock);
  *   are concerned with are online after they get the lock.
  * - Governor routines that can be called in cpufreq hotplug path should not
  *   take this sem as top level hotplug notifier handler takes this.
+ * - Lock should not be held across
+ *     __cpufreq_governor(data, CPUFREQ_GOV_STOP);
  */
 static DEFINE_PER_CPU(int, policy_cpu);
 static DEFINE_PER_CPU(struct rw_semaphore, cpu_policy_rwsem);
@@ -1697,8 +1699,17 @@ static int __cpufreq_set_policy(struct cpufreq_policy *data,
 			dprintk("governor switch\n");
 
 			/* end old governor */
-			if (data->governor)
+			if (data->governor) {
+				/*
+				 * Need to release the rwsem around governor
+				 * stop due to lock dependency between
+				 * cancel_delayed_work_sync and the read lock
+				 * taken in the delayed work handler.
+				 */
+				unlock_policy_rwsem_write(data->cpu);
 				__cpufreq_governor(data, CPUFREQ_GOV_STOP);
+				lock_policy_rwsem_write(data->cpu);
+			}
 
 			/* start new governor */
 			data->governor = policy->governor;
-- 
1.6.0.6

-- 

WARNING: multiple messages have this Message-ID (diff)
From: venkatesh.pallipadi@intel.com
To: "Dave Jones" <davej@redhat.com>
Cc: linux-kernel@vger.kernel.org, cpufreq@vger.kernel.org,
	kernel-testers@vger.kernel.org, "Ingo Molnar" <mingo@elte.hu>,
	"Rafael J. Wysocki" <rjw@sisk.pl>,
	"Dave Young" <hidave.darkstar@gmail.com>,
	"Pekka Enberg" <penberg@cs.helsinki.fi>,
	"Mathieu Desnoyers" <mathieu.desnoyers@polymtl.ca>,
	"Thomas Renninger" <trenn@suse.de>,
	"Venkatesh Pallipadi" <venkatesh.pallipadi@intel.com>
Subject: [patch 1/3] cpufreq: remove rwsem lock from CPUFREQ_GOV_STOP call (second call site)
Date: Thu, 25 Jun 2009 11:33:55 -0700	[thread overview]
Message-ID: <20090625183601.300706000@intel.com> (raw)
In-Reply-To: 20090625183354.491259000@intel.com

[-- Attachment #1: 0001-remove-rwsem-lock-from-CPUFREQ_GOV_STOP-call-second.patch --]
[-- Type: text/plain, Size: 2181 bytes --]

From: Mathieu Desnoyers <mathieu.desnoyers@polymtl.ca>

commit	42a06f2166f2f6f7bf04f32b4e823eacdceafdc9

Missed a call site for CPUFREQ_GOV_STOP to remove the rwlock taken around the
teardown. To make a long story short, the rwlock write-lock causes a circular
dependency with cancel_delayed_work_sync(), because the timer handler takes the
write lock.

Note that all callers to __cpufreq_set_policy are taking the rwsem. All sysfs
callers (writers) hold the write rwsem at the earliest sysfs calling stage.

However, the rwlock write-lock is not needed upon governor stop.

Change :
- Added comment from Venki at lock definition site.

Signed-off-by: Mathieu Desnoyers <mathieu.desnoyers@polymtl.ca>
Signed-off-by: Thomas Renninger <trenn@suse.de>
Signed-off-by: Venkatesh Pallipadi <venkatesh.pallipadi@intel.com>

---
 drivers/cpufreq/cpufreq.c |   13 ++++++++++++-
 1 files changed, 12 insertions(+), 1 deletions(-)

diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
index 6e2ec0b..728656c 100644
--- a/drivers/cpufreq/cpufreq.c
+++ b/drivers/cpufreq/cpufreq.c
@@ -61,6 +61,8 @@ static DEFINE_SPINLOCK(cpufreq_driver_lock);
  *   are concerned with are online after they get the lock.
  * - Governor routines that can be called in cpufreq hotplug path should not
  *   take this sem as top level hotplug notifier handler takes this.
+ * - Lock should not be held across
+ *     __cpufreq_governor(data, CPUFREQ_GOV_STOP);
  */
 static DEFINE_PER_CPU(int, policy_cpu);
 static DEFINE_PER_CPU(struct rw_semaphore, cpu_policy_rwsem);
@@ -1697,8 +1699,17 @@ static int __cpufreq_set_policy(struct cpufreq_policy *data,
 			dprintk("governor switch\n");
 
 			/* end old governor */
-			if (data->governor)
+			if (data->governor) {
+				/*
+				 * Need to release the rwsem around governor
+				 * stop due to lock dependency between
+				 * cancel_delayed_work_sync and the read lock
+				 * taken in the delayed work handler.
+				 */
+				unlock_policy_rwsem_write(data->cpu);
 				__cpufreq_governor(data, CPUFREQ_GOV_STOP);
+				lock_policy_rwsem_write(data->cpu);
+			}
 
 			/* start new governor */
 			data->governor = policy->governor;
-- 
1.6.0.6

-- 


  reply	other threads:[~2009-06-25 18:33 UTC|newest]

Thread overview: 13+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2009-06-25 18:33 [patch 0/3] Take care of cpufreq lockdep issues venkatesh.pallipadi-ral2JQCrhuEAvxtiuMwx3w
2009-06-25 18:33 ` venkatesh.pallipadi
2009-06-25 18:33 ` venkatesh.pallipadi-ral2JQCrhuEAvxtiuMwx3w [this message]
2009-06-25 18:33   ` [patch 1/3] cpufreq: remove rwsem lock from CPUFREQ_GOV_STOP call (second call site) venkatesh.pallipadi
2009-06-25 18:33 ` [patch 2/3] cpufreq: Define dbs_mutex purpose and cleanup its usage venkatesh.pallipadi-ral2JQCrhuEAvxtiuMwx3w
2009-06-25 18:33   ` venkatesh.pallipadi
     [not found]   ` <20090625183601.493904000-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
2009-06-25 19:46     ` Mathieu Desnoyers
2009-06-25 19:46       ` Mathieu Desnoyers
2009-06-25 20:54       ` Pallipadi, Venkatesh
     [not found]         ` <1245963285.4534.20542.camel-bi+AKbBUZKY6gyzm1THtWbp2dZbC/Bob@public.gmane.org>
2009-06-25 21:32           ` Mathieu Desnoyers
2009-06-25 21:32             ` Mathieu Desnoyers
2009-06-25 18:33 ` [patch 3/3] cpufreq: Define dbs_mutex purpose and cleanup its usage conservative gov venkatesh.pallipadi-ral2JQCrhuEAvxtiuMwx3w
2009-06-25 18:33   ` venkatesh.pallipadi

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20090625183601.300706000@intel.com \
    --to=venkatesh.pallipadi-ral2jqcrhueavxtiumwx3w@public.gmane.org \
    --cc=cpufreq-u79uwXL29TY76Z2rM5mHXA@public.gmane.org \
    --cc=davej-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org \
    --cc=hidave.darkstar-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org \
    --cc=kernel-testers-u79uwXL29TY76Z2rM5mHXA@public.gmane.org \
    --cc=linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org \
    --cc=mathieu.desnoyers-scC8bbJcJLCw5LPnMra/2Q@public.gmane.org \
    --cc=mingo-X9Un+BFzKDI@public.gmane.org \
    --cc=penberg-bbCR+/B0CizivPeTLB3BmA@public.gmane.org \
    --cc=rjw-KKrjLPT3xs0@public.gmane.org \
    --cc=trenn-l3A5Bk7waGM@public.gmane.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.