[PATCH] thermal: Use freezable workqueue

[PATCH] thermal: Use freezable workqueue

[Rafael J. Wysocki]

From: Rafael J. Wysocki <rjw@sisk.pl>

If thermal polling is enabled, which for example is the case for
ACPI thermal zones with the _TZP object defined, the thermal driver
uses delayed work items for this purpose.  Unfortunately, since
they are queued up using schedule_delayed_work(), the work function
may be executed during system suspend or resume, which is not
desirable.

To prevent that from happening, use a freezable workqueue for
queuing up delayed work items in the thermal driver.

Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
---
 drivers/thermal/thermal_sys.c |   22 ++++++++++++++++++++--
 1 file changed, 20 insertions(+), 2 deletions(-)

Index: linux-2.6/drivers/thermal/thermal_sys.c
===================================================================
--- linux-2.6.orig/drivers/thermal/thermal_sys.c
+++ linux-2.6/drivers/thermal/thermal_sys.c
@@ -62,6 +62,20 @@ static DEFINE_MUTEX(thermal_list_lock);
 
 static unsigned int thermal_event_seqnum;
 
+static struct workqueue_struct *thermal_wq;
+
+static int __init thermal_start_workqueue(void)
+{
+	thermal_wq = alloc_workqueue("thermal", WQ_FREEZEABLE, 0);
+	return thermal_wq ? 0 : -ENOMEM;
+}
+
+static inline void thermal_destroy_workqueue(void)
+{
+	if (thermal_wq)
+		destroy_workqueue(thermal_wq);
+}
+
 static struct genl_family thermal_event_genl_family = {
 	.id = GENL_ID_GENERATE,
 	.name = THERMAL_GENL_FAMILY_NAME,
@@ -611,10 +625,10 @@ static void thermal_zone_device_set_poll
 		return;
 
 	if (delay > 1000)
-		schedule_delayed_work(&(tz->poll_queue),
+		queue_delayed_work(thermal_wq, &(tz->poll_queue),
 				      round_jiffies(msecs_to_jiffies(delay)));
 	else
-		schedule_delayed_work(&(tz->poll_queue),
+		queue_delayed_work(thermal_wq, &(tz->poll_queue),
 				      msecs_to_jiffies(delay));
 }
 
@@ -1306,11 +1320,14 @@ static int __init thermal_init(void)
 	int result = 0;
 
 	result = class_register(&thermal_class);
+	if (!result)
+		result = thermal_start_workqueue();
 	if (result) {
 		idr_destroy(&thermal_tz_idr);
 		idr_destroy(&thermal_cdev_idr);
 		mutex_destroy(&thermal_idr_lock);
 		mutex_destroy(&thermal_list_lock);
+		thermal_destroy_workqueue();
 	}
 	result = genetlink_init();
 	return result;
@@ -1328,6 +1345,7 @@ static void __exit thermal_exit(void)
 	idr_destroy(&thermal_cdev_idr);
 	mutex_destroy(&thermal_idr_lock);
 	mutex_destroy(&thermal_list_lock);
+	thermal_destroy_workqueue();
 	genetlink_exit();
 }
 

Re: [PATCH] thermal: Use freezable workqueue

[Dmitry Torokhov]

On Tue, Feb 08, 2011 at 12:29:57AM +0100, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rjw@sisk.pl>
> 
> If thermal polling is enabled, which for example is the case for
> ACPI thermal zones with the _TZP object defined, the thermal driver
> uses delayed work items for this purpose.  Unfortunately, since
> they are queued up using schedule_delayed_work(), the work function
> may be executed during system suspend or resume, which is not
> desirable.
> 
> To prevent that from happening, use a freezable workqueue for
> queuing up delayed work items in the thermal driver.
> 
> Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
> ---
>  drivers/thermal/thermal_sys.c |   22 ++++++++++++++++++++--
>  1 file changed, 20 insertions(+), 2 deletions(-)
> 
> Index: linux-2.6/drivers/thermal/thermal_sys.c
> ===================================================================
> --- linux-2.6.orig/drivers/thermal/thermal_sys.c
> +++ linux-2.6/drivers/thermal/thermal_sys.c
> @@ -62,6 +62,20 @@ static DEFINE_MUTEX(thermal_list_lock);
>  
>  static unsigned int thermal_event_seqnum;
>  
> +static struct workqueue_struct *thermal_wq;
> +
> +static int __init thermal_start_workqueue(void)
> +{
> +	thermal_wq = alloc_workqueue("thermal", WQ_FREEZEABLE, 0);

Should probably be unbound as well.

FWIW, I would not mind if we had a global freezeable workqueue already
predefined. I could switch input_polldev and vmw_balloon to it and there
probably could be more users...

Thanks.

-- 
Dmitry

Re: [PATCH] thermal: Use freezable workqueue

[Rafael J. Wysocki]

On Tuesday, February 08, 2011, Dmitry Torokhov wrote:
> On Tue, Feb 08, 2011 at 12:29:57AM +0100, Rafael J. Wysocki wrote:
> > From: Rafael J. Wysocki <rjw@sisk.pl>
> > 
> > If thermal polling is enabled, which for example is the case for
> > ACPI thermal zones with the _TZP object defined, the thermal driver
> > uses delayed work items for this purpose.  Unfortunately, since
> > they are queued up using schedule_delayed_work(), the work function
> > may be executed during system suspend or resume, which is not
> > desirable.
> > 
> > To prevent that from happening, use a freezable workqueue for
> > queuing up delayed work items in the thermal driver.
> > 
> > Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
> > ---
> >  drivers/thermal/thermal_sys.c |   22 ++++++++++++++++++++--
> >  1 file changed, 20 insertions(+), 2 deletions(-)
> > 
> > Index: linux-2.6/drivers/thermal/thermal_sys.c
> > ===================================================================
> > --- linux-2.6.orig/drivers/thermal/thermal_sys.c
> > +++ linux-2.6/drivers/thermal/thermal_sys.c
> > @@ -62,6 +62,20 @@ static DEFINE_MUTEX(thermal_list_lock);
> >  
> >  static unsigned int thermal_event_seqnum;
> >  
> > +static struct workqueue_struct *thermal_wq;
> > +
> > +static int __init thermal_start_workqueue(void)
> > +{
> > +	thermal_wq = alloc_workqueue("thermal", WQ_FREEZEABLE, 0);
> 
> Should probably be unbound as well.

Yup, thanks.

> FWIW, I would not mind if we had a global freezeable workqueue already
> predefined. I could switch input_polldev and vmw_balloon to it and there
> probably could be more users...

Hmm, OK.

Do you think we should add system_freezeable_wq to the set of predefined
workqueues?

Rafael

Re: [PATCH] thermal: Use freezable workqueue

[Tejun Heo]

Hello, Dmitry, Rafael.

On Tue, Feb 08, 2011 at 10:20:25AM +0100, Rafael J. Wysocki wrote:
> > > +	thermal_wq = alloc_workqueue("thermal", WQ_FREEZEABLE, 0);
> > 
> > Should probably be unbound as well.
> 
> Yup, thanks.

Hmm.. why should it be unbound?  Unbound wqs are primarily useful for
very long running (system daemon type) works which may also consume
considerable amount of cpu cycles.

> > FWIW, I would not mind if we had a global freezeable workqueue already
> > predefined. I could switch input_polldev and vmw_balloon to it and there
> > probably could be more users...
> 
> Hmm, OK.
> 
> Do you think we should add system_freezeable_wq to the set of predefined
> workqueues?

Oh, yeah, definitely.  I'll write up a patch.

Thanks.

-- 
tejun

[PATCH wq#for-2.6.39] workqueue: add system_freezeable_wq

[Tejun Heo]

>From de62c151df9b6568c67977920267392c432f340a Mon Sep 17 00:00:00 2001
From: Tejun Heo <tj@kernel.org>
Date: Tue, 8 Feb 2011 10:39:03 +0100

Add system wide freezeable workqueue.

Signed-off-by: Tejun Heo <tj@kernel.org>
Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com>
Cc: "Rafael J. Wysocki" <rjw@sisk.pl>
---
Please feel free to pull from the following branch.

 git://git.kernel.org/pub/scm/linux/kernel/git/tj/wq.git for-2.6.39

Thanks.

 include/linux/workqueue.h |    4 ++++
 kernel/workqueue.c        |    4 +++-
 2 files changed, 7 insertions(+), 1 deletions(-)

diff --git a/include/linux/workqueue.h b/include/linux/workqueue.h
index 1ac1158..de6a755 100644
--- a/include/linux/workqueue.h
+++ b/include/linux/workqueue.h
@@ -286,11 +286,15 @@ enum {
  * any specific CPU, not concurrency managed, and all queued works are
  * executed immediately as long as max_active limit is not reached and
  * resources are available.
+ *
+ * system_freezeable_wq is equivalent to system_wq except that it's
+ * freezeable.
  */
 extern struct workqueue_struct *system_wq;
 extern struct workqueue_struct *system_long_wq;
 extern struct workqueue_struct *system_nrt_wq;
 extern struct workqueue_struct *system_unbound_wq;
+extern struct workqueue_struct *system_freezeable_wq;
 
 extern struct workqueue_struct *
 __alloc_workqueue_key(const char *name, unsigned int flags, int max_active,
diff --git a/kernel/workqueue.c b/kernel/workqueue.c
index 11869fa..2b8de0c 100644
--- a/kernel/workqueue.c
+++ b/kernel/workqueue.c
@@ -3764,8 +3764,10 @@ static int __init init_workqueues(void)
 	system_nrt_wq = alloc_workqueue("events_nrt", WQ_NON_REENTRANT, 0);
 	system_unbound_wq = alloc_workqueue("events_unbound", WQ_UNBOUND,
 					    WQ_UNBOUND_MAX_ACTIVE);
+	system_freezeable_wq = alloc_workqueue("events_freezeable",
+					       WQ_FREEZEABLE, 0);
 	BUG_ON(!system_wq || !system_long_wq || !system_nrt_wq ||
-	       !system_unbound_wq);
+	       !system_unbound_wq || !system_freezeable_wq);
 	return 0;
 }
 early_initcall(init_workqueues);
-- 
1.7.1

Re: [linux-pm] [PATCH wq#for-2.6.39] workqueue: add system_freezeable_wq

[Alan Stern]

On Tue, 8 Feb 2011, Tejun Heo wrote:

> From de62c151df9b6568c67977920267392c432f340a Mon Sep 17 00:00:00 2001
> From: Tejun Heo <tj@kernel.org>
> Date: Tue, 8 Feb 2011 10:39:03 +0100
> 
> Add system wide freezeable workqueue.

The preferred spelling is "freezable".

Alan Stern

Re: [linux-pm] [PATCH wq#for-2.6.39] workqueue: add system_freezeable_wq

[Tejun Heo]

On Tue, Feb 08, 2011 at 10:10:46AM -0500, Alan Stern wrote:
> On Tue, 8 Feb 2011, Tejun Heo wrote:
> 
> > From de62c151df9b6568c67977920267392c432f340a Mon Sep 17 00:00:00 2001
> > From: Tejun Heo <tj@kernel.org>
> > Date: Tue, 8 Feb 2011 10:39:03 +0100
> > 
> > Add system wide freezeable workqueue.
> 
> The preferred spelling is "freezable".

Hmmm... workqueue always has used freezeable (even before cmwq
changes).  It seems like mass renaming is in order.  :-(

Thanks.

-- 
tejun

Re: [linux-pm] [PATCH wq#for-2.6.39] workqueue: add system_freezeable_wq

[Alan Stern]

On Tue, 8 Feb 2011, Tejun Heo wrote:

> On Tue, Feb 08, 2011 at 10:10:46AM -0500, Alan Stern wrote:
> > On Tue, 8 Feb 2011, Tejun Heo wrote:
> > 
> > > From de62c151df9b6568c67977920267392c432f340a Mon Sep 17 00:00:00 2001
> > > From: Tejun Heo <tj@kernel.org>
> > > Date: Tue, 8 Feb 2011 10:39:03 +0100
> > > 
> > > Add system wide freezeable workqueue.
> > 
> > The preferred spelling is "freezable".
> 
> Hmmm... workqueue always has used freezeable (even before cmwq
> changes).  It seems like mass renaming is in order.  :-(

The name of create_freezeable_workqueue was misspelled originally and
then never changed.  But other code and most of the comments and 
documentation are spelled correctly -- grep for both spellings and 
you'll see.

Agreed, some renaming and editing is in order.

Alan Stern

Re: [PATCH wq#for-2.6.39] workqueue: add system_freezeable_wq

[Dmitry Torokhov]

On Tue, Feb 08, 2011 at 10:41:55AM +0100, Tejun Heo wrote:
> From de62c151df9b6568c67977920267392c432f340a Mon Sep 17 00:00:00 2001
> From: Tejun Heo <tj@kernel.org>
> Date: Tue, 8 Feb 2011 10:39:03 +0100
> 
> Add system wide freezeable workqueue.
> 
> Signed-off-by: Tejun Heo <tj@kernel.org>
> Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com>

Acked-by: Dmitry Torokhov <dtor@mail.ru>

And if, as Alan said, it could be "freezable" that would be great.

-- 
Dmitry