[PATCH v4] pm_qos: make update_request non blocking

[PATCH v4] pm_qos: make update_request non blocking

[florian]

In order to allow drivers to use pm_qos_update_request from interrupt
context we call the notifiers via schedule_work().

Signed-off-by: Florian Mickler <florian@mickler.org>
---

Well, this would be the schedule_work() alternative. 

 kernel/pm_qos_params.c |   47 ++++++++++++++++++++++++++++++-----------------
 1 files changed, 30 insertions(+), 17 deletions(-)

diff --git a/kernel/pm_qos_params.c b/kernel/pm_qos_params.c
index f42d3f7..296343a 100644
--- a/kernel/pm_qos_params.c
+++ b/kernel/pm_qos_params.c
@@ -40,6 +40,7 @@
 #include <linux/string.h>
 #include <linux/platform_device.h>
 #include <linux/init.h>
+#include <linux/workqueue.h>
 
 #include <linux/uaccess.h>
 
@@ -60,11 +61,13 @@ struct pm_qos_request_list {
 
 static s32 max_compare(s32 v1, s32 v2);
 static s32 min_compare(s32 v1, s32 v2);
+static void update_notify(struct work_struct *work);
 
 struct pm_qos_object {
 	struct pm_qos_request_list requests;
 	struct blocking_notifier_head *notifiers;
 	struct miscdevice pm_qos_power_miscdev;
+	struct work_struct notify;
 	char *name;
 	s32 default_value;
 	atomic_t target_value;
@@ -72,10 +75,12 @@ struct pm_qos_object {
 };
 
 static struct pm_qos_object null_pm_qos;
+
 static BLOCKING_NOTIFIER_HEAD(cpu_dma_lat_notifier);
 static struct pm_qos_object cpu_dma_pm_qos = {
 	.requests = {LIST_HEAD_INIT(cpu_dma_pm_qos.requests.list)},
 	.notifiers = &cpu_dma_lat_notifier,
+	.notify = __WORK_INITIALIZER(cpu_dma_pm_qos.notify, update_notify),
 	.name = "cpu_dma_latency",
 	.default_value = 2000 * USEC_PER_SEC,
 	.target_value = ATOMIC_INIT(2000 * USEC_PER_SEC),
@@ -86,6 +91,7 @@ static BLOCKING_NOTIFIER_HEAD(network_lat_notifier);
 static struct pm_qos_object network_lat_pm_qos = {
 	.requests = {LIST_HEAD_INIT(network_lat_pm_qos.requests.list)},
 	.notifiers = &network_lat_notifier,
+	.notify = __WORK_INITIALIZER(network_lat_pm_qos.notify, update_notify),
 	.name = "network_latency",
 	.default_value = 2000 * USEC_PER_SEC,
 	.target_value = ATOMIC_INIT(2000 * USEC_PER_SEC),
@@ -97,13 +103,14 @@ static BLOCKING_NOTIFIER_HEAD(network_throughput_notifier);
 static struct pm_qos_object network_throughput_pm_qos = {
 	.requests = {LIST_HEAD_INIT(network_throughput_pm_qos.requests.list)},
 	.notifiers = &network_throughput_notifier,
+	.notify = __WORK_INITIALIZER(network_throughput_pm_qos.notify,
+			update_notify),
 	.name = "network_throughput",
 	.default_value = 0,
 	.target_value = ATOMIC_INIT(0),
 	.comparitor = max_compare
 };
 
-
 static struct pm_qos_object *pm_qos_array[] = {
 	&null_pm_qos,
 	&cpu_dma_pm_qos,
@@ -135,35 +142,40 @@ static s32 min_compare(s32 v1, s32 v2)
 	return min(v1, v2);
 }
 
+static void update_notify(struct work_struct *work)
+{
+	struct pm_qos_object *obj =
+		container_of(work, struct pm_qos_object, notify);
+
+	s32 extreme_value = atomic_read(&obj->target_value);
+	blocking_notifier_call_chain(
+		obj->notifiers,
+			(unsigned long) extreme_value, NULL);
+}
 
 static void update_target(int pm_qos_class)
 {
 	s32 extreme_value;
 	struct pm_qos_request_list *node;
+	struct pm_qos_object *obj = pm_qos_array[pm_qos_class];
 	unsigned long flags;
 	int call_notifier = 0;
 
 	spin_lock_irqsave(&pm_qos_lock, flags);
-	extreme_value = pm_qos_array[pm_qos_class]->default_value;
-	list_for_each_entry(node,
-			&pm_qos_array[pm_qos_class]->requests.list, list) {
-		extreme_value = pm_qos_array[pm_qos_class]->comparitor(
-				extreme_value, node->value);
-	}
-	if (atomic_read(&pm_qos_array[pm_qos_class]->target_value) !=
-			extreme_value) {
+	extreme_value = obj->default_value;
+	list_for_each_entry(node, &obj->requests.list, list)
+		extreme_value = obj->comparitor(extreme_value, node->value);
+
+	if (atomic_read(&obj->target_value) != extreme_value) {
 		call_notifier = 1;
-		atomic_set(&pm_qos_array[pm_qos_class]->target_value,
-				extreme_value);
+		atomic_set(&obj->target_value, extreme_value);
 		pr_debug(KERN_ERR "new target for qos %d is %d\n", pm_qos_class,
-			atomic_read(&pm_qos_array[pm_qos_class]->target_value));
+			atomic_read(&obj->target_value));
 	}
 	spin_unlock_irqrestore(&pm_qos_lock, flags);
 
 	if (call_notifier)
-		blocking_notifier_call_chain(
-				pm_qos_array[pm_qos_class]->notifiers,
-					(unsigned long) extreme_value, NULL);
+		schedule_work(&obj->notify);
 }
 
 static int register_pm_qos_misc(struct pm_qos_object *qos)
@@ -301,7 +313,7 @@ EXPORT_SYMBOL_GPL(pm_qos_remove_request);
  * @pm_qos_class: identifies which qos target changes should be notified.
  * @notifier: notifier block managed by caller.
  *
- * will register the notifier into a notification chain that gets called
+ * Will register the notifier into a notification chain that gets called
  * upon changes to the pm_qos_class target value.
  */
 int pm_qos_add_notifier(int pm_qos_class, struct notifier_block *notifier)
@@ -320,7 +332,7 @@ EXPORT_SYMBOL_GPL(pm_qos_add_notifier);
  * @pm_qos_class: identifies which qos target changes are notified.
  * @notifier: notifier block to be removed.
  *
- * will remove the notifier from the notification chain that gets called
+ * Will remove the notifier from the notification chain that gets called
  * upon changes to the pm_qos_class target value.
  */
 int pm_qos_remove_notifier(int pm_qos_class, struct notifier_block *notifier)
@@ -392,6 +404,7 @@ static int __init pm_qos_power_init(void)
 {
 	int ret = 0;
 
+
 	ret = register_pm_qos_misc(&cpu_dma_pm_qos);
 	if (ret < 0) {
 		printk(KERN_ERR "pm_qos_param: cpu_dma_latency setup failed\n");
-- 
1.7.1

Re: [PATCH v4] pm_qos: make update_request non blocking

[James Bottomley]

On Wed, 2010-06-09 at 17:29 +0200, florian@mickler.org wrote:
> In order to allow drivers to use pm_qos_update_request from interrupt
> context we call the notifiers via schedule_work().
> 
> Signed-off-by: Florian Mickler <florian@mickler.org>
> ---
> 
> Well, this would be the schedule_work() alternative. 
> 
>  kernel/pm_qos_params.c |   47 ++++++++++++++++++++++++++++++-----------------
>  1 files changed, 30 insertions(+), 17 deletions(-)
> 
> diff --git a/kernel/pm_qos_params.c b/kernel/pm_qos_params.c
> index f42d3f7..296343a 100644
> --- a/kernel/pm_qos_params.c
> +++ b/kernel/pm_qos_params.c
> @@ -40,6 +40,7 @@
>  #include <linux/string.h>
>  #include <linux/platform_device.h>
>  #include <linux/init.h>
> +#include <linux/workqueue.h>
>  
>  #include <linux/uaccess.h>
>  
> @@ -60,11 +61,13 @@ struct pm_qos_request_list {
>  
>  static s32 max_compare(s32 v1, s32 v2);
>  static s32 min_compare(s32 v1, s32 v2);
> +static void update_notify(struct work_struct *work);
>  
>  struct pm_qos_object {
>  	struct pm_qos_request_list requests;
>  	struct blocking_notifier_head *notifiers;
>  	struct miscdevice pm_qos_power_miscdev;
> +	struct work_struct notify;
>  	char *name;
>  	s32 default_value;
>  	atomic_t target_value;
> @@ -72,10 +75,12 @@ struct pm_qos_object {
>  };
>  
>  static struct pm_qos_object null_pm_qos;
> +
>  static BLOCKING_NOTIFIER_HEAD(cpu_dma_lat_notifier);
>  static struct pm_qos_object cpu_dma_pm_qos = {
>  	.requests = {LIST_HEAD_INIT(cpu_dma_pm_qos.requests.list)},
>  	.notifiers = &cpu_dma_lat_notifier,
> +	.notify = __WORK_INITIALIZER(cpu_dma_pm_qos.notify, update_notify),
>  	.name = "cpu_dma_latency",
>  	.default_value = 2000 * USEC_PER_SEC,
>  	.target_value = ATOMIC_INIT(2000 * USEC_PER_SEC),
> @@ -86,6 +91,7 @@ static BLOCKING_NOTIFIER_HEAD(network_lat_notifier);
>  static struct pm_qos_object network_lat_pm_qos = {
>  	.requests = {LIST_HEAD_INIT(network_lat_pm_qos.requests.list)},
>  	.notifiers = &network_lat_notifier,
> +	.notify = __WORK_INITIALIZER(network_lat_pm_qos.notify, update_notify),
>  	.name = "network_latency",
>  	.default_value = 2000 * USEC_PER_SEC,
>  	.target_value = ATOMIC_INIT(2000 * USEC_PER_SEC),
> @@ -97,13 +103,14 @@ static BLOCKING_NOTIFIER_HEAD(network_throughput_notifier);
>  static struct pm_qos_object network_throughput_pm_qos = {
>  	.requests = {LIST_HEAD_INIT(network_throughput_pm_qos.requests.list)},
>  	.notifiers = &network_throughput_notifier,
> +	.notify = __WORK_INITIALIZER(network_throughput_pm_qos.notify,
> +			update_notify),
>  	.name = "network_throughput",
>  	.default_value = 0,
>  	.target_value = ATOMIC_INIT(0),
>  	.comparitor = max_compare
>  };
>  
> -
>  static struct pm_qos_object *pm_qos_array[] = {
>  	&null_pm_qos,
>  	&cpu_dma_pm_qos,
> @@ -135,35 +142,40 @@ static s32 min_compare(s32 v1, s32 v2)
>  	return min(v1, v2);
>  }
>  
> +static void update_notify(struct work_struct *work)
> +{
> +	struct pm_qos_object *obj =
> +		container_of(work, struct pm_qos_object, notify);
> +
> +	s32 extreme_value = atomic_read(&obj->target_value);
> +	blocking_notifier_call_chain(
> +		obj->notifiers,
> +			(unsigned long) extreme_value, NULL);
> +}
>  
>  static void update_target(int pm_qos_class)
>  {
>  	s32 extreme_value;
>  	struct pm_qos_request_list *node;
> +	struct pm_qos_object *obj = pm_qos_array[pm_qos_class];
>  	unsigned long flags;
>  	int call_notifier = 0;
>  
>  	spin_lock_irqsave(&pm_qos_lock, flags);
> -	extreme_value = pm_qos_array[pm_qos_class]->default_value;
> -	list_for_each_entry(node,
> -			&pm_qos_array[pm_qos_class]->requests.list, list) {
> -		extreme_value = pm_qos_array[pm_qos_class]->comparitor(
> -				extreme_value, node->value);
> -	}
> -	if (atomic_read(&pm_qos_array[pm_qos_class]->target_value) !=
> -			extreme_value) {
> +	extreme_value = obj->default_value;
> +	list_for_each_entry(node, &obj->requests.list, list)
> +		extreme_value = obj->comparitor(extreme_value, node->value);
> +
> +	if (atomic_read(&obj->target_value) != extreme_value) {
>  		call_notifier = 1;
> -		atomic_set(&pm_qos_array[pm_qos_class]->target_value,
> -				extreme_value);
> +		atomic_set(&obj->target_value, extreme_value);
>  		pr_debug(KERN_ERR "new target for qos %d is %d\n", pm_qos_class,
> -			atomic_read(&pm_qos_array[pm_qos_class]->target_value));
> +			atomic_read(&obj->target_value));
>  	}
>  	spin_unlock_irqrestore(&pm_qos_lock, flags);
>  
>  	if (call_notifier)
> -		blocking_notifier_call_chain(
> -				pm_qos_array[pm_qos_class]->notifiers,
> -					(unsigned long) extreme_value, NULL);
> +		schedule_work(&obj->notify);
>  }

This still isn't resilient against two successive calls to update.  If
the second one gets to schedule_work() before the work of the first one
has finished, you'll corrupt the workqueue.

The mechanisms to solve this race (refcounting a pool of structures) are
so heavyweight that I concluded it was simpler just to have atomically
updated pm_qos objects and enforce it ... rather than trying to allow
blocking chains to be added to atomic sites via a workqueue.

James

Re: [PATCH v4] pm_qos: make update_request non blocking

[James Bottomley]

On Wed, 2010-06-09 at 17:29 +0200, florian@mickler.org wrote:
> In order to allow drivers to use pm_qos_update_request from interrupt
> context we call the notifiers via schedule_work().
> 
> Signed-off-by: Florian Mickler <florian@mickler.org>
> ---
> 
> Well, this would be the schedule_work() alternative. 
> 
>  kernel/pm_qos_params.c |   47 ++++++++++++++++++++++++++++++-----------------
>  1 files changed, 30 insertions(+), 17 deletions(-)
> 
> diff --git a/kernel/pm_qos_params.c b/kernel/pm_qos_params.c
> index f42d3f7..296343a 100644
> --- a/kernel/pm_qos_params.c
> +++ b/kernel/pm_qos_params.c
> @@ -40,6 +40,7 @@
>  #include <linux/string.h>
>  #include <linux/platform_device.h>
>  #include <linux/init.h>
> +#include <linux/workqueue.h>
>  
>  #include <linux/uaccess.h>
>  
> @@ -60,11 +61,13 @@ struct pm_qos_request_list {
>  
>  static s32 max_compare(s32 v1, s32 v2);
>  static s32 min_compare(s32 v1, s32 v2);
> +static void update_notify(struct work_struct *work);
>  
>  struct pm_qos_object {
>  	struct pm_qos_request_list requests;
>  	struct blocking_notifier_head *notifiers;
>  	struct miscdevice pm_qos_power_miscdev;
> +	struct work_struct notify;
>  	char *name;
>  	s32 default_value;
>  	atomic_t target_value;
> @@ -72,10 +75,12 @@ struct pm_qos_object {
>  };
>  
>  static struct pm_qos_object null_pm_qos;
> +
>  static BLOCKING_NOTIFIER_HEAD(cpu_dma_lat_notifier);
>  static struct pm_qos_object cpu_dma_pm_qos = {
>  	.requests = {LIST_HEAD_INIT(cpu_dma_pm_qos.requests.list)},
>  	.notifiers = &cpu_dma_lat_notifier,
> +	.notify = __WORK_INITIALIZER(cpu_dma_pm_qos.notify, update_notify),
>  	.name = "cpu_dma_latency",
>  	.default_value = 2000 * USEC_PER_SEC,
>  	.target_value = ATOMIC_INIT(2000 * USEC_PER_SEC),
> @@ -86,6 +91,7 @@ static BLOCKING_NOTIFIER_HEAD(network_lat_notifier);
>  static struct pm_qos_object network_lat_pm_qos = {
>  	.requests = {LIST_HEAD_INIT(network_lat_pm_qos.requests.list)},
>  	.notifiers = &network_lat_notifier,
> +	.notify = __WORK_INITIALIZER(network_lat_pm_qos.notify, update_notify),
>  	.name = "network_latency",
>  	.default_value = 2000 * USEC_PER_SEC,
>  	.target_value = ATOMIC_INIT(2000 * USEC_PER_SEC),
> @@ -97,13 +103,14 @@ static BLOCKING_NOTIFIER_HEAD(network_throughput_notifier);
>  static struct pm_qos_object network_throughput_pm_qos = {
>  	.requests = {LIST_HEAD_INIT(network_throughput_pm_qos.requests.list)},
>  	.notifiers = &network_throughput_notifier,
> +	.notify = __WORK_INITIALIZER(network_throughput_pm_qos.notify,
> +			update_notify),
>  	.name = "network_throughput",
>  	.default_value = 0,
>  	.target_value = ATOMIC_INIT(0),
>  	.comparitor = max_compare
>  };
>  
> -
>  static struct pm_qos_object *pm_qos_array[] = {
>  	&null_pm_qos,
>  	&cpu_dma_pm_qos,
> @@ -135,35 +142,40 @@ static s32 min_compare(s32 v1, s32 v2)
>  	return min(v1, v2);
>  }
>  
> +static void update_notify(struct work_struct *work)
> +{
> +	struct pm_qos_object *obj =
> +		container_of(work, struct pm_qos_object, notify);
> +
> +	s32 extreme_value = atomic_read(&obj->target_value);
> +	blocking_notifier_call_chain(
> +		obj->notifiers,
> +			(unsigned long) extreme_value, NULL);
> +}
>  
>  static void update_target(int pm_qos_class)
>  {
>  	s32 extreme_value;
>  	struct pm_qos_request_list *node;
> +	struct pm_qos_object *obj = pm_qos_array[pm_qos_class];
>  	unsigned long flags;
>  	int call_notifier = 0;
>  
>  	spin_lock_irqsave(&pm_qos_lock, flags);
> -	extreme_value = pm_qos_array[pm_qos_class]->default_value;
> -	list_for_each_entry(node,
> -			&pm_qos_array[pm_qos_class]->requests.list, list) {
> -		extreme_value = pm_qos_array[pm_qos_class]->comparitor(
> -				extreme_value, node->value);
> -	}
> -	if (atomic_read(&pm_qos_array[pm_qos_class]->target_value) !=
> -			extreme_value) {
> +	extreme_value = obj->default_value;
> +	list_for_each_entry(node, &obj->requests.list, list)
> +		extreme_value = obj->comparitor(extreme_value, node->value);
> +
> +	if (atomic_read(&obj->target_value) != extreme_value) {
>  		call_notifier = 1;
> -		atomic_set(&pm_qos_array[pm_qos_class]->target_value,
> -				extreme_value);
> +		atomic_set(&obj->target_value, extreme_value);
>  		pr_debug(KERN_ERR "new target for qos %d is %d\n", pm_qos_class,
> -			atomic_read(&pm_qos_array[pm_qos_class]->target_value));
> +			atomic_read(&obj->target_value));
>  	}
>  	spin_unlock_irqrestore(&pm_qos_lock, flags);
>  
>  	if (call_notifier)
> -		blocking_notifier_call_chain(
> -				pm_qos_array[pm_qos_class]->notifiers,
> -					(unsigned long) extreme_value, NULL);
> +		schedule_work(&obj->notify);
>  }

This still isn't resilient against two successive calls to update.  If
the second one gets to schedule_work() before the work of the first one
has finished, you'll corrupt the workqueue.

The mechanisms to solve this race (refcounting a pool of structures) are
so heavyweight that I concluded it was simpler just to have atomically
updated pm_qos objects and enforce it ... rather than trying to allow
blocking chains to be added to atomic sites via a workqueue.

James

Re: [PATCH v4] pm_qos: make update_request non blocking

[Florian Mickler]

On Wed, 09 Jun 2010 11:37:12 -0400
James Bottomley <James.Bottomley@suse.de> wrote:


> This still isn't resilient against two successive calls to update.  If
> the second one gets to schedule_work() before the work of the first one
> has finished, you'll corrupt the workqueue.

Sorry, I don't see it. Can you elaborate?

In "run_workqueue(" we do a list_del_init() which resets the
list-pointers of the workitem and only after that reset the
WORK_STRUCT_PENDING member of said structure. 


schedule_work does a queue_work_on which does a test_and_set_bit on
the WORK_STRUCT_PENDING member of the work and only queues the work
via list_add_tail in insert_work afterwards.

Where is my think'o? Or was this fixed while you didn't look?

So what _can_ happen, is that we miss a new notfication while the old
notification is still in the queue. But I don't think this is a problem.

Cheers,
Flo

Re: [PATCH v4] pm_qos: make update_request non blocking

[Florian Mickler]

On Wed, 09 Jun 2010 11:37:12 -0400
James Bottomley <James.Bottomley@suse.de> wrote:


> This still isn't resilient against two successive calls to update.  If
> the second one gets to schedule_work() before the work of the first one
> has finished, you'll corrupt the workqueue.

Sorry, I don't see it. Can you elaborate?

In "run_workqueue(" we do a list_del_init() which resets the
list-pointers of the workitem and only after that reset the
WORK_STRUCT_PENDING member of said structure. 


schedule_work does a queue_work_on which does a test_and_set_bit on
the WORK_STRUCT_PENDING member of the work and only queues the work
via list_add_tail in insert_work afterwards.

Where is my think'o? Or was this fixed while you didn't look?

So what _can_ happen, is that we miss a new notfication while the old
notification is still in the queue. But I don't think this is a problem.

Cheers,
Flo

Re: [PATCH v4] pm_qos: make update_request non blocking

[James Bottomley]

On Wed, 2010-06-09 at 18:00 +0200, Florian Mickler wrote:
> On Wed, 09 Jun 2010 11:37:12 -0400
> James Bottomley <James.Bottomley@suse.de> wrote:
> 
> 
> > This still isn't resilient against two successive calls to update.  If
> > the second one gets to schedule_work() before the work of the first one
> > has finished, you'll corrupt the workqueue.
> 
> Sorry, I don't see it. Can you elaborate?
> 
> In "run_workqueue(" we do a list_del_init() which resets the
> list-pointers of the workitem and only after that reset the
> WORK_STRUCT_PENDING member of said structure. 
> 
> 
> schedule_work does a queue_work_on which does a test_and_set_bit on
> the WORK_STRUCT_PENDING member of the work and only queues the work
> via list_add_tail in insert_work afterwards.
> 
> Where is my think'o? Or was this fixed while you didn't look?
> 
> So what _can_ happen, is that we miss a new notfication while the old
> notification is still in the queue. But I don't think this is a problem.

OK, so the expression of the race is that the latest notification gets
lost.  If something is tracking values, you'd really like to lose the
previous one (which is now irrelevant) not the latest one.  The point is
there's still a race.

James

Re: [PATCH v4] pm_qos: make update_request non blocking

[James Bottomley]

On Wed, 2010-06-09 at 18:00 +0200, Florian Mickler wrote:
> On Wed, 09 Jun 2010 11:37:12 -0400
> James Bottomley <James.Bottomley@suse.de> wrote:
> 
> 
> > This still isn't resilient against two successive calls to update.  If
> > the second one gets to schedule_work() before the work of the first one
> > has finished, you'll corrupt the workqueue.
> 
> Sorry, I don't see it. Can you elaborate?
> 
> In "run_workqueue(" we do a list_del_init() which resets the
> list-pointers of the workitem and only after that reset the
> WORK_STRUCT_PENDING member of said structure. 
> 
> 
> schedule_work does a queue_work_on which does a test_and_set_bit on
> the WORK_STRUCT_PENDING member of the work and only queues the work
> via list_add_tail in insert_work afterwards.
> 
> Where is my think'o? Or was this fixed while you didn't look?
> 
> So what _can_ happen, is that we miss a new notfication while the old
> notification is still in the queue. But I don't think this is a problem.

OK, so the expression of the race is that the latest notification gets
lost.  If something is tracking values, you'd really like to lose the
previous one (which is now irrelevant) not the latest one.  The point is
there's still a race.

James

Re: [PATCH v4] pm_qos: make update_request non blocking

[Florian Mickler]

On Wed, 09 Jun 2010 12:07:25 -0400
James Bottomley <James.Bottomley@suse.de> wrote:

> On Wed, 2010-06-09 at 18:00 +0200, Florian Mickler wrote:
> > On Wed, 09 Jun 2010 11:37:12 -0400
> > James Bottomley <James.Bottomley@suse.de> wrote:
> > 
> > 
> > > This still isn't resilient against two successive calls to update.  If
> > > the second one gets to schedule_work() before the work of the first one
> > > has finished, you'll corrupt the workqueue.
> > 
> > Sorry, I don't see it. Can you elaborate?
> > 
> > In "run_workqueue(" we do a list_del_init() which resets the
> > list-pointers of the workitem and only after that reset the
> > WORK_STRUCT_PENDING member of said structure. 
> > 
> > 
> > schedule_work does a queue_work_on which does a test_and_set_bit on
> > the WORK_STRUCT_PENDING member of the work and only queues the work
> > via list_add_tail in insert_work afterwards.
> > 
> > Where is my think'o? Or was this fixed while you didn't look?
> > 
> > So what _can_ happen, is that we miss a new notfication while the old
> > notification is still in the queue. But I don't think this is a problem.
> 
> OK, so the expression of the race is that the latest notification gets
> lost.  If something is tracking values, you'd really like to lose the
> previous one (which is now irrelevant) not the latest one.  The point is
> there's still a race.
> 
> James
> 

Yeah, but for blocking notification it is not that bad. 

Doesn't using blocking notifiers mean that you need to always check
via pm_qos_request() to get the latest value anyways? I.e. the
notification is just a hint, that something changed so you can act on
it. But if you act (may it because of notification or because of
something other) then you have to get the current value anyways.

I think there are 2 schemes of operation. The one which needs
atomic notifiers and where it would be bad if we lost any notification
and the other where it is just a way of doing some work in a timely
fashion but it isn't critical that it is done right away.

pm_qos was the second kind of operation up until now, because the
notifiers may have got scheduled away while blocked. 

I think we should allow for both kinds of operations simultaneous. But
as I got that pushback to the change from John Linville as I touched his
code, I got a bit reluctant and just have done the simple variant. :)

(for example if we want to implement some sort of debugging stats, we
could use blocking notifications, but if we for example block some
c-stats due to a latency-requirement we probably need some atomic
notifications.)

Cheers,
Flo

Re: [PATCH v4] pm_qos: make update_request non blocking

[Florian Mickler]

On Wed, 09 Jun 2010 12:07:25 -0400
James Bottomley <James.Bottomley@suse.de> wrote:

> On Wed, 2010-06-09 at 18:00 +0200, Florian Mickler wrote:
> > On Wed, 09 Jun 2010 11:37:12 -0400
> > James Bottomley <James.Bottomley@suse.de> wrote:
> > 
> > 
> > > This still isn't resilient against two successive calls to update.  If
> > > the second one gets to schedule_work() before the work of the first one
> > > has finished, you'll corrupt the workqueue.
> > 
> > Sorry, I don't see it. Can you elaborate?
> > 
> > In "run_workqueue(" we do a list_del_init() which resets the
> > list-pointers of the workitem and only after that reset the
> > WORK_STRUCT_PENDING member of said structure. 
> > 
> > 
> > schedule_work does a queue_work_on which does a test_and_set_bit on
> > the WORK_STRUCT_PENDING member of the work and only queues the work
> > via list_add_tail in insert_work afterwards.
> > 
> > Where is my think'o? Or was this fixed while you didn't look?
> > 
> > So what _can_ happen, is that we miss a new notfication while the old
> > notification is still in the queue. But I don't think this is a problem.
> 
> OK, so the expression of the race is that the latest notification gets
> lost.  If something is tracking values, you'd really like to lose the
> previous one (which is now irrelevant) not the latest one.  The point is
> there's still a race.
> 
> James
> 

Yeah, but for blocking notification it is not that bad. 

Doesn't using blocking notifiers mean that you need to always check
via pm_qos_request() to get the latest value anyways? I.e. the
notification is just a hint, that something changed so you can act on
it. But if you act (may it because of notification or because of
something other) then you have to get the current value anyways.

I think there are 2 schemes of operation. The one which needs
atomic notifiers and where it would be bad if we lost any notification
and the other where it is just a way of doing some work in a timely
fashion but it isn't critical that it is done right away.

pm_qos was the second kind of operation up until now, because the
notifiers may have got scheduled away while blocked. 

I think we should allow for both kinds of operations simultaneous. But
as I got that pushback to the change from John Linville as I touched his
code, I got a bit reluctant and just have done the simple variant. :)

(for example if we want to implement some sort of debugging stats, we
could use blocking notifications, but if we for example block some
c-stats due to a latency-requirement we probably need some atomic
notifications.)

Cheers,
Flo

Re: [PATCH v4] pm_qos: make update_request non blocking

[James Bottomley]

On Wed, 2010-06-09 at 18:32 +0200, Florian Mickler wrote:
> On Wed, 09 Jun 2010 12:07:25 -0400
> James Bottomley <James.Bottomley@suse.de> wrote:
> 
> > On Wed, 2010-06-09 at 18:00 +0200, Florian Mickler wrote:
> > > On Wed, 09 Jun 2010 11:37:12 -0400
> > > James Bottomley <James.Bottomley@suse.de> wrote:
> > > 
> > > 
> > > > This still isn't resilient against two successive calls to update.  If
> > > > the second one gets to schedule_work() before the work of the first one
> > > > has finished, you'll corrupt the workqueue.
> > > 
> > > Sorry, I don't see it. Can you elaborate?
> > > 
> > > In "run_workqueue(" we do a list_del_init() which resets the
> > > list-pointers of the workitem and only after that reset the
> > > WORK_STRUCT_PENDING member of said structure. 
> > > 
> > > 
> > > schedule_work does a queue_work_on which does a test_and_set_bit on
> > > the WORK_STRUCT_PENDING member of the work and only queues the work
> > > via list_add_tail in insert_work afterwards.
> > > 
> > > Where is my think'o? Or was this fixed while you didn't look?
> > > 
> > > So what _can_ happen, is that we miss a new notfication while the old
> > > notification is still in the queue. But I don't think this is a problem.
> > 
> > OK, so the expression of the race is that the latest notification gets
> > lost.  If something is tracking values, you'd really like to lose the
> > previous one (which is now irrelevant) not the latest one.  The point is
> > there's still a race.
> > 
> > James
> > 
> 
> Yeah, but for blocking notification it is not that bad. 

The network latency notifier uses the value to recalculate something.
Losing the last value will mean it's using stale data.

> Doesn't using blocking notifiers mean that you need to always check
> via pm_qos_request() to get the latest value anyways? I.e. the
> notification is just a hint, that something changed so you can act on
> it. But if you act (may it because of notification or because of
> something other) then you have to get the current value anyways.

Well, the network notifier assumes the notifier value is the latest ... 

> I think there are 2 schemes of operation. The one which needs
> atomic notifiers and where it would be bad if we lost any notification
> and the other where it is just a way of doing some work in a timely
> fashion but it isn't critical that it is done right away.
> 
> pm_qos was the second kind of operation up until now, because the
> notifiers may have got scheduled away while blocked. 

Actually, no ... the current API preserves ordering semantics.  If a
notifier sleeps and another change comes along, the update callsite will
sleep on the notifier's mutex ... so ordering is always preserved.

> I think we should allow for both kinds of operations simultaneous. But
> as I got that pushback to the change from John Linville as I touched his
> code, I got a bit reluctant and just have done the simple variant. :)

The code I proposed does ... but it relies on the callsite supplying the
necessary context.

James

Re: [PATCH v4] pm_qos: make update_request non blocking

[James Bottomley]

On Wed, 2010-06-09 at 18:32 +0200, Florian Mickler wrote:
> On Wed, 09 Jun 2010 12:07:25 -0400
> James Bottomley <James.Bottomley@suse.de> wrote:
> 
> > On Wed, 2010-06-09 at 18:00 +0200, Florian Mickler wrote:
> > > On Wed, 09 Jun 2010 11:37:12 -0400
> > > James Bottomley <James.Bottomley@suse.de> wrote:
> > > 
> > > 
> > > > This still isn't resilient against two successive calls to update.  If
> > > > the second one gets to schedule_work() before the work of the first one
> > > > has finished, you'll corrupt the workqueue.
> > > 
> > > Sorry, I don't see it. Can you elaborate?
> > > 
> > > In "run_workqueue(" we do a list_del_init() which resets the
> > > list-pointers of the workitem and only after that reset the
> > > WORK_STRUCT_PENDING member of said structure. 
> > > 
> > > 
> > > schedule_work does a queue_work_on which does a test_and_set_bit on
> > > the WORK_STRUCT_PENDING member of the work and only queues the work
> > > via list_add_tail in insert_work afterwards.
> > > 
> > > Where is my think'o? Or was this fixed while you didn't look?
> > > 
> > > So what _can_ happen, is that we miss a new notfication while the old
> > > notification is still in the queue. But I don't think this is a problem.
> > 
> > OK, so the expression of the race is that the latest notification gets
> > lost.  If something is tracking values, you'd really like to lose the
> > previous one (which is now irrelevant) not the latest one.  The point is
> > there's still a race.
> > 
> > James
> > 
> 
> Yeah, but for blocking notification it is not that bad. 

The network latency notifier uses the value to recalculate something.
Losing the last value will mean it's using stale data.

> Doesn't using blocking notifiers mean that you need to always check
> via pm_qos_request() to get the latest value anyways? I.e. the
> notification is just a hint, that something changed so you can act on
> it. But if you act (may it because of notification or because of
> something other) then you have to get the current value anyways.

Well, the network notifier assumes the notifier value is the latest ... 

> I think there are 2 schemes of operation. The one which needs
> atomic notifiers and where it would be bad if we lost any notification
> and the other where it is just a way of doing some work in a timely
> fashion but it isn't critical that it is done right away.
> 
> pm_qos was the second kind of operation up until now, because the
> notifiers may have got scheduled away while blocked. 

Actually, no ... the current API preserves ordering semantics.  If a
notifier sleeps and another change comes along, the update callsite will
sleep on the notifier's mutex ... so ordering is always preserved.

> I think we should allow for both kinds of operations simultaneous. But
> as I got that pushback to the change from John Linville as I touched his
> code, I got a bit reluctant and just have done the simple variant. :)

The code I proposed does ... but it relies on the callsite supplying the
necessary context.

James

Re: [PATCH v4] pm_qos: make update_request non blocking

[Florian Mickler]

On Wed, 09 Jun 2010 13:05:49 -0400
James Bottomley <James.Bottomley@suse.de> wrote:

> On Wed, 2010-06-09 at 18:32 +0200, Florian Mickler wrote:
> > Yeah, but for blocking notification it is not that bad. 
> 
> The network latency notifier uses the value to recalculate something.
> Losing the last value will mean it's using stale data.

Ah. Indeed. I didn't do my homework there. That sucks. (Btw, I know why
you said "recalculate _something_" :) )

It even fetches via pm_qos_get_request() in the middle. But obviously
if we do not report the last value but the value before that, then this
is bloed. 

We could fix it though for all current (two) in tree users by passing
always a negative value in to the notification. Then the network
notifier fetches the value via pm_qos_get_request(); 
And cpuidle ignores the value anyways. 

> > 
> > pm_qos was the second kind of operation up until now, because the
> > notifiers may have got scheduled away while blocked. 
> 
> Actually, no ... the current API preserves ordering semantics.  If a
> notifier sleeps and another change comes along, the update callsite will
> sleep on the notifier's mutex ... so ordering is always preserved.

Ah! Ordering. Something I didn't thought of. But still no luck for me. 

> 
> > I think we should allow for both kinds of operations simultaneous. But
> > as I got that pushback to the change from John Linville as I touched his
> > code, I got a bit reluctant and just have done the simple variant. :)
> 
> The code I proposed does ... but it relies on the callsite supplying the
> necessary context.
> 

Yes, I know. It's just that if we want to have both kind's of notifiers
we need to use a atomic_notifier chain for that constraint. And then we
have the issue that John objected to, namely pushing all the
schedule_work code to each of the API-Users. 

Anyway, I think your patch is better in that it is correct.

It just doesn't solve the issue for constraints where drivers want to
update from interrupt but notifiers want to be able to work from
process context. 

> James
> 

Cheers,
Flo

Re: [PATCH v4] pm_qos: make update_request non blocking

[Florian Mickler]

On Wed, 09 Jun 2010 13:05:49 -0400
James Bottomley <James.Bottomley@suse.de> wrote:

> On Wed, 2010-06-09 at 18:32 +0200, Florian Mickler wrote:
> > Yeah, but for blocking notification it is not that bad. 
> 
> The network latency notifier uses the value to recalculate something.
> Losing the last value will mean it's using stale data.

Ah. Indeed. I didn't do my homework there. That sucks. (Btw, I know why
you said "recalculate _something_" :) )

It even fetches via pm_qos_get_request() in the middle. But obviously
if we do not report the last value but the value before that, then this
is bloed. 

We could fix it though for all current (two) in tree users by passing
always a negative value in to the notification. Then the network
notifier fetches the value via pm_qos_get_request(); 
And cpuidle ignores the value anyways. 

> > 
> > pm_qos was the second kind of operation up until now, because the
> > notifiers may have got scheduled away while blocked. 
> 
> Actually, no ... the current API preserves ordering semantics.  If a
> notifier sleeps and another change comes along, the update callsite will
> sleep on the notifier's mutex ... so ordering is always preserved.

Ah! Ordering. Something I didn't thought of. But still no luck for me. 

> 
> > I think we should allow for both kinds of operations simultaneous. But
> > as I got that pushback to the change from John Linville as I touched his
> > code, I got a bit reluctant and just have done the simple variant. :)
> 
> The code I proposed does ... but it relies on the callsite supplying the
> necessary context.
> 

Yes, I know. It's just that if we want to have both kind's of notifiers
we need to use a atomic_notifier chain for that constraint. And then we
have the issue that John objected to, namely pushing all the
schedule_work code to each of the API-Users. 

Anyway, I think your patch is better in that it is correct.

It just doesn't solve the issue for constraints where drivers want to
update from interrupt but notifiers want to be able to work from
process context. 

> James
> 

Cheers,
Flo

Re: [PATCH v4] pm_qos: make update_request non blocking

[Florian Mickler]

On Wed, 09 Jun 2010 13:05:49 -0400
James Bottomley <James.Bottomley@suse.de> wrote:
> On Wed, 2010-06-09 at 18:32 +0200, Florian Mickler wrote:
> > On Wed, 09 Jun 2010 12:07:25 -0400
> > James Bottomley <James.Bottomley@suse.de> wrote:
> > > OK, so the expression of the race is that the latest notification gets
> > > lost.  If something is tracking values, you'd really like to lose the
> > > previous one (which is now irrelevant) not the latest one.  The point is
> > > there's still a race.
> > > 
> > > James
> > > 
> > 
> > Yeah, but for blocking notification it is not that bad. 
> 
> The network latency notifier uses the value to recalculate something.
> Losing the last value will mean it's using stale data.

Actually after pondering a bit, it is not stale data that gets
delivered: (Correct me if I'm wrong)

The update_notify() function determines the extreme value and then
calls the blocking_notifier_chain. 

But just before the update_notify() function get's called, the
work-structure is reset and re-queue-able. So it is possible to queue it
already even before the extreme_value in update_notify get's
determined. 

So the notified value is always the latest or there is another
notification underway.

> 
> > Doesn't using blocking notifiers mean that you need to always check
> > via pm_qos_request() to get the latest value anyways? I.e. the
> > notification is just a hint, that something changed so you can act on
> > it. But if you act (may it because of notification or because of
> > something other) then you have to get the current value anyways.
> 
> Well, the network notifier assumes the notifier value is the latest ... 
> 
> > I think there are 2 schemes of operation. The one which needs
> > atomic notifiers and where it would be bad if we lost any notification
> > and the other where it is just a way of doing some work in a timely
> > fashion but it isn't critical that it is done right away.
> > 
> > pm_qos was the second kind of operation up until now, because the
> > notifiers may have got scheduled away while blocked. 
> 
> Actually, no ... the current API preserves ordering semantics.  If a
> notifier sleeps and another change comes along, the update callsite will
> sleep on the notifier's mutex ... so ordering is always preserved.

If my above thinkings are right, then the change in semantics is only,
that now you can not determine by the number of notifications the
number of update_request-calls.

> 
> James
> 

Cheers,
Flo

Re: [PATCH v4] pm_qos: make update_request non blocking

[Florian Mickler]

On Wed, 09 Jun 2010 13:05:49 -0400
James Bottomley <James.Bottomley@suse.de> wrote:
> On Wed, 2010-06-09 at 18:32 +0200, Florian Mickler wrote:
> > On Wed, 09 Jun 2010 12:07:25 -0400
> > James Bottomley <James.Bottomley@suse.de> wrote:
> > > OK, so the expression of the race is that the latest notification gets
> > > lost.  If something is tracking values, you'd really like to lose the
> > > previous one (which is now irrelevant) not the latest one.  The point is
> > > there's still a race.
> > > 
> > > James
> > > 
> > 
> > Yeah, but for blocking notification it is not that bad. 
> 
> The network latency notifier uses the value to recalculate something.
> Losing the last value will mean it's using stale data.

Actually after pondering a bit, it is not stale data that gets
delivered: (Correct me if I'm wrong)

The update_notify() function determines the extreme value and then
calls the blocking_notifier_chain. 

But just before the update_notify() function get's called, the
work-structure is reset and re-queue-able. So it is possible to queue it
already even before the extreme_value in update_notify get's
determined. 

So the notified value is always the latest or there is another
notification underway.

> 
> > Doesn't using blocking notifiers mean that you need to always check
> > via pm_qos_request() to get the latest value anyways? I.e. the
> > notification is just a hint, that something changed so you can act on
> > it. But if you act (may it because of notification or because of
> > something other) then you have to get the current value anyways.
> 
> Well, the network notifier assumes the notifier value is the latest ... 
> 
> > I think there are 2 schemes of operation. The one which needs
> > atomic notifiers and where it would be bad if we lost any notification
> > and the other where it is just a way of doing some work in a timely
> > fashion but it isn't critical that it is done right away.
> > 
> > pm_qos was the second kind of operation up until now, because the
> > notifiers may have got scheduled away while blocked. 
> 
> Actually, no ... the current API preserves ordering semantics.  If a
> notifier sleeps and another change comes along, the update callsite will
> sleep on the notifier's mutex ... so ordering is always preserved.

If my above thinkings are right, then the change in semantics is only,
that now you can not determine by the number of notifications the
number of update_request-calls.

> 
> James
> 

Cheers,
Flo

Re: [linux-pm] [PATCH v4] pm_qos: make update_request non blocking

[James Bottomley]

On Thu, 2010-06-10 at 09:45 +0200, Florian Mickler wrote:
> On Wed, 09 Jun 2010 13:05:49 -0400
> James Bottomley <James.Bottomley@suse.de> wrote:
> > On Wed, 2010-06-09 at 18:32 +0200, Florian Mickler wrote:
> > > On Wed, 09 Jun 2010 12:07:25 -0400
> > > James Bottomley <James.Bottomley@suse.de> wrote:
> > > > OK, so the expression of the race is that the latest notification gets
> > > > lost.  If something is tracking values, you'd really like to lose the
> > > > previous one (which is now irrelevant) not the latest one.  The point is
> > > > there's still a race.
> > > > 
> > > > James
> > > > 
> > > 
> > > Yeah, but for blocking notification it is not that bad. 
> > 
> > The network latency notifier uses the value to recalculate something.
> > Losing the last value will mean it's using stale data.
> 
> Actually after pondering a bit, it is not stale data that gets
> delivered: (Correct me if I'm wrong)
> 
> The update_notify() function determines the extreme value and then
> calls the blocking_notifier_chain. 
> 
> But just before the update_notify() function get's called, the
> work-structure is reset and re-queue-able. So it is possible to queue it
> already even before the extreme_value in update_notify get's
> determined. 
> 
> So the notified value is always the latest or there is another
> notification underway.

Well, no ... it's a race, and like all good races the winner is non
deterministic.

If the update comes in before the work queue is run, then everyone
eventually sees the new value.  If it comes in as the chain is running,
some notifiers see the old value and some the new.  If it comes in
during back end processing, no-one sees the new value.

James

Re: [PATCH v4] pm_qos: make update_request non blocking

[James Bottomley]

On Thu, 2010-06-10 at 09:45 +0200, Florian Mickler wrote:
> On Wed, 09 Jun 2010 13:05:49 -0400
> James Bottomley <James.Bottomley@suse.de> wrote:
> > On Wed, 2010-06-09 at 18:32 +0200, Florian Mickler wrote:
> > > On Wed, 09 Jun 2010 12:07:25 -0400
> > > James Bottomley <James.Bottomley@suse.de> wrote:
> > > > OK, so the expression of the race is that the latest notification gets
> > > > lost.  If something is tracking values, you'd really like to lose the
> > > > previous one (which is now irrelevant) not the latest one.  The point is
> > > > there's still a race.
> > > > 
> > > > James
> > > > 
> > > 
> > > Yeah, but for blocking notification it is not that bad. 
> > 
> > The network latency notifier uses the value to recalculate something.
> > Losing the last value will mean it's using stale data.
> 
> Actually after pondering a bit, it is not stale data that gets
> delivered: (Correct me if I'm wrong)
> 
> The update_notify() function determines the extreme value and then
> calls the blocking_notifier_chain. 
> 
> But just before the update_notify() function get's called, the
> work-structure is reset and re-queue-able. So it is possible to queue it
> already even before the extreme_value in update_notify get's
> determined. 
> 
> So the notified value is always the latest or there is another
> notification underway.

Well, no ... it's a race, and like all good races the winner is non
deterministic.

If the update comes in before the work queue is run, then everyone
eventually sees the new value.  If it comes in as the chain is running,
some notifiers see the old value and some the new.  If it comes in
during back end processing, no-one sees the new value.

James

Re: [linux-pm] [PATCH v4] pm_qos: make update_request non blocking

[Florian Mickler]

On Thu, 10 Jun 2010 08:39:04 -0500
James Bottomley <James.Bottomley@suse.de> wrote:

> On Thu, 2010-06-10 at 09:45 +0200, Florian Mickler wrote:
> > On Wed, 09 Jun 2010 13:05:49 -0400
> > James Bottomley <James.Bottomley@suse.de> wrote:
> > > On Wed, 2010-06-09 at 18:32 +0200, Florian Mickler wrote:
> > > > On Wed, 09 Jun 2010 12:07:25 -0400
> > > > James Bottomley <James.Bottomley@suse.de> wrote:
> > > > > OK, so the expression of the race is that the latest notification gets
> > > > > lost.  If something is tracking values, you'd really like to lose the
> > > > > previous one (which is now irrelevant) not the latest one.  The point is
> > > > > there's still a race.
> > > > > 
> > > > > James
> > > > > 
> > > > 
> > > > Yeah, but for blocking notification it is not that bad. 
> > > 
> > > The network latency notifier uses the value to recalculate something.
> > > Losing the last value will mean it's using stale data.
> > 
> > Actually after pondering a bit, it is not stale data that gets
> > delivered: (Correct me if I'm wrong)
> > 
> > The update_notify() function determines the extreme value and then
> > calls the blocking_notifier_chain. 
> > 
> > But just before the update_notify() function get's called, the
> > work-structure is reset and re-queue-able. So it is possible to queue it
> > already even before the extreme_value in update_notify get's
> > determined. 
> > 
> > So the notified value is always the latest or there is another
> > notification underway.
> 
> Well, no ... it's a race, and like all good races the winner is non
> deterministic.

Can you point out where I'm wrong?

U1. update_request gets called
U2. 	new extreme value gets calculated under spinlock
U3.	notify gets queued if its WORK_PENDING_BIT is not set.

run_workqueue() does the following:
R1. clears the WORK_PENDING_BIT
R2. calls update_notify()
R3. 	reads the current extreme value
R4. 	notification gets called with that value		


If another update_request comes to schedule_work before
run_workqueue() has cleared the WORK_PENDING_BIT, the work will not be
requeued, but R3 isn't yet executed. So the notifiers will get the last
value.

If another update_request comes to schedule_work() after
run_workqueue() has cleared the WORK_PENDING_BIT, the work will be
requeued and another update_notify will be executed later.

> 
> If the update comes in before the work queue is run, then everyone
> eventually sees the new value.  If it comes in as the chain is running,
> some notifiers see the old value and some the new.  If it comes in
> during back end processing, no-one sees the new value.
> 
> James
> 

No, I think that is not correct. There will always be another
update_notify()-call after the last update_request-recalculation due to
the fact that run_workqueue() calls work_clear_pending() before calling
the callback function of that work. 

(We could even requeue the work from within that callback function if we
wanted to keep the number of notifications the same. But then we would
need to queue the extreme_values in the framework. I think this
isn't needed.)

Cheers,
Flo

Re: [PATCH v4] pm_qos: make update_request non blocking

[Florian Mickler]

On Thu, 10 Jun 2010 08:39:04 -0500
James Bottomley <James.Bottomley@suse.de> wrote:

> On Thu, 2010-06-10 at 09:45 +0200, Florian Mickler wrote:
> > On Wed, 09 Jun 2010 13:05:49 -0400
> > James Bottomley <James.Bottomley@suse.de> wrote:
> > > On Wed, 2010-06-09 at 18:32 +0200, Florian Mickler wrote:
> > > > On Wed, 09 Jun 2010 12:07:25 -0400
> > > > James Bottomley <James.Bottomley@suse.de> wrote:
> > > > > OK, so the expression of the race is that the latest notification gets
> > > > > lost.  If something is tracking values, you'd really like to lose the
> > > > > previous one (which is now irrelevant) not the latest one.  The point is
> > > > > there's still a race.
> > > > > 
> > > > > James
> > > > > 
> > > > 
> > > > Yeah, but for blocking notification it is not that bad. 
> > > 
> > > The network latency notifier uses the value to recalculate something.
> > > Losing the last value will mean it's using stale data.
> > 
> > Actually after pondering a bit, it is not stale data that gets
> > delivered: (Correct me if I'm wrong)
> > 
> > The update_notify() function determines the extreme value and then
> > calls the blocking_notifier_chain. 
> > 
> > But just before the update_notify() function get's called, the
> > work-structure is reset and re-queue-able. So it is possible to queue it
> > already even before the extreme_value in update_notify get's
> > determined. 
> > 
> > So the notified value is always the latest or there is another
> > notification underway.
> 
> Well, no ... it's a race, and like all good races the winner is non
> deterministic.

Can you point out where I'm wrong?

U1. update_request gets called
U2. 	new extreme value gets calculated under spinlock
U3.	notify gets queued if its WORK_PENDING_BIT is not set.

run_workqueue() does the following:
R1. clears the WORK_PENDING_BIT
R2. calls update_notify()
R3. 	reads the current extreme value
R4. 	notification gets called with that value		


If another update_request comes to schedule_work before
run_workqueue() has cleared the WORK_PENDING_BIT, the work will not be
requeued, but R3 isn't yet executed. So the notifiers will get the last
value.

If another update_request comes to schedule_work() after
run_workqueue() has cleared the WORK_PENDING_BIT, the work will be
requeued and another update_notify will be executed later.

> 
> If the update comes in before the work queue is run, then everyone
> eventually sees the new value.  If it comes in as the chain is running,
> some notifiers see the old value and some the new.  If it comes in
> during back end processing, no-one sees the new value.
> 
> James
> 

No, I think that is not correct. There will always be another
update_notify()-call after the last update_request-recalculation due to
the fact that run_workqueue() calls work_clear_pending() before calling
the callback function of that work. 

(We could even requeue the work from within that callback function if we
wanted to keep the number of notifications the same. But then we would
need to queue the extreme_values in the framework. I think this
isn't needed.)

Cheers,
Flo

Re: [linux-pm] [PATCH v4] pm_qos: make update_request non blocking

[James Bottomley]

On Thu, 2010-06-10 at 16:41 +0200, Florian Mickler wrote:
> > > So the notified value is always the latest or there is another
> > > notification underway.
> > 
> > Well, no ... it's a race, and like all good races the winner is non
> > deterministic.
> 
> Can you point out where I'm wrong?
> 
> U1. update_request gets called
> U2. 	new extreme value gets calculated under spinlock
> U3.	notify gets queued if its WORK_PENDING_BIT is not set.
> 
> run_workqueue() does the following:
> R1. clears the WORK_PENDING_BIT
> R2. calls update_notify()
> R3. 	reads the current extreme value
> R4. 	notification gets called with that value		
> 
> 
> If another update_request comes to schedule_work before
> run_workqueue() has cleared the WORK_PENDING_BIT, the work will not be
> requeued, but R3 isn't yet executed. So the notifiers will get the last
> value.

So the race now only causes lost older notifications ... as long as the
consumers are OK with that (it is an API change) then this should work.
You're still not taking advantage of the user context passed in, though,
so this does needlessly delay notifications for that case.

Actually, pm_qos_remove now needs a flush_scheduled work since you don't
want to return until the list is clear (since the next action may be to
free the object).

James

Re: [PATCH v4] pm_qos: make update_request non blocking

[James Bottomley]

On Thu, 2010-06-10 at 16:41 +0200, Florian Mickler wrote:
> > > So the notified value is always the latest or there is another
> > > notification underway.
> > 
> > Well, no ... it's a race, and like all good races the winner is non
> > deterministic.
> 
> Can you point out where I'm wrong?
> 
> U1. update_request gets called
> U2. 	new extreme value gets calculated under spinlock
> U3.	notify gets queued if its WORK_PENDING_BIT is not set.
> 
> run_workqueue() does the following:
> R1. clears the WORK_PENDING_BIT
> R2. calls update_notify()
> R3. 	reads the current extreme value
> R4. 	notification gets called with that value		
> 
> 
> If another update_request comes to schedule_work before
> run_workqueue() has cleared the WORK_PENDING_BIT, the work will not be
> requeued, but R3 isn't yet executed. So the notifiers will get the last
> value.

So the race now only causes lost older notifications ... as long as the
consumers are OK with that (it is an API change) then this should work.
You're still not taking advantage of the user context passed in, though,
so this does needlessly delay notifications for that case.

Actually, pm_qos_remove now needs a flush_scheduled work since you don't
want to return until the list is clear (since the next action may be to
free the object).

James

Re: [linux-pm] [PATCH v4] pm_qos: make update_request non blocking

[Florian Mickler]

On Fri, 11 Jun 2010 09:25:52 -0500
James Bottomley <James.Bottomley@suse.de> wrote:

> On Thu, 2010-06-10 at 16:41 +0200, Florian Mickler wrote:
> > > > So the notified value is always the latest or there is another
> > > > notification underway.
> > > 
> > > Well, no ... it's a race, and like all good races the winner is non
> > > deterministic.
> > 
> > Can you point out where I'm wrong?
> > 
> > U1. update_request gets called
> > U2. 	new extreme value gets calculated under spinlock
> > U3.	notify gets queued if its WORK_PENDING_BIT is not set.
> > 
> > run_workqueue() does the following:
> > R1. clears the WORK_PENDING_BIT
> > R2. calls update_notify()
> > R3. 	reads the current extreme value
> > R4. 	notification gets called with that value		
> > 
> > 
> > If another update_request comes to schedule_work before
> > run_workqueue() has cleared the WORK_PENDING_BIT, the work will not be
> > requeued, but R3 isn't yet executed. So the notifiers will get the last
> > value.
> 
> So the race now only causes lost older notifications ... as long as the
> consumers are OK with that (it is an API change) then this should work.
> You're still not taking advantage of the user context passed in, though,
> so this does needlessly delay notifications for that case.

Right. We can use execute_in_process_context. 
 
> Actually, pm_qos_remove now needs a flush_scheduled work since you don't
> want to return until the list is clear (since the next action may be to
> free the object).

Yes. Good point, will fix. 

> James
> 

Cheers,
Flo

Re: [PATCH v4] pm_qos: make update_request non blocking

[Florian Mickler]

On Fri, 11 Jun 2010 09:25:52 -0500
James Bottomley <James.Bottomley@suse.de> wrote:

> On Thu, 2010-06-10 at 16:41 +0200, Florian Mickler wrote:
> > > > So the notified value is always the latest or there is another
> > > > notification underway.
> > > 
> > > Well, no ... it's a race, and like all good races the winner is non
> > > deterministic.
> > 
> > Can you point out where I'm wrong?
> > 
> > U1. update_request gets called
> > U2. 	new extreme value gets calculated under spinlock
> > U3.	notify gets queued if its WORK_PENDING_BIT is not set.
> > 
> > run_workqueue() does the following:
> > R1. clears the WORK_PENDING_BIT
> > R2. calls update_notify()
> > R3. 	reads the current extreme value
> > R4. 	notification gets called with that value		
> > 
> > 
> > If another update_request comes to schedule_work before
> > run_workqueue() has cleared the WORK_PENDING_BIT, the work will not be
> > requeued, but R3 isn't yet executed. So the notifiers will get the last
> > value.
> 
> So the race now only causes lost older notifications ... as long as the
> consumers are OK with that (it is an API change) then this should work.
> You're still not taking advantage of the user context passed in, though,
> so this does needlessly delay notifications for that case.

Right. We can use execute_in_process_context. 
 
> Actually, pm_qos_remove now needs a flush_scheduled work since you don't
> want to return until the list is clear (since the next action may be to
> free the object).

Yes. Good point, will fix. 

> James
> 

Cheers,
Flo

Re: [linux-pm] [PATCH v4] pm_qos: make update_request non blocking

[Florian Mickler]

On Fri, 11 Jun 2010 09:25:52 -0500
James Bottomley <James.Bottomley@suse.de> wrote:

> 
> Actually, pm_qos_remove now needs a flush_scheduled work since you don't
> want to return until the list is clear (since the next action may be to
> free the object).

The work-items are allocated in the pm_qos objects (which get never
freed), so we should be fine there.

> 
> James
> 

Re: [PATCH v4] pm_qos: make update_request non blocking

[Florian Mickler]

On Fri, 11 Jun 2010 09:25:52 -0500
James Bottomley <James.Bottomley@suse.de> wrote:

> 
> Actually, pm_qos_remove now needs a flush_scheduled work since you don't
> want to return until the list is clear (since the next action may be to
> free the object).

The work-items are allocated in the pm_qos objects (which get never
freed), so we should be fine there.

> 
> James
> 

Re: [linux-pm] [PATCH v4] pm_qos: make update_request non blocking

[James Bottomley]

On Mon, 2010-06-14 at 16:33 +0200, Florian Mickler wrote:
> On Fri, 11 Jun 2010 09:25:52 -0500
> James Bottomley <James.Bottomley@suse.de> wrote:
> 
> > 
> > Actually, pm_qos_remove now needs a flush_scheduled work since you don't
> > want to return until the list is clear (since the next action may be to
> > free the object).
> 
> The work-items are allocated in the pm_qos objects (which get never
> freed), so we should be fine there.

That's not a safe assumption.  Once we get into drivers, timers and cpu
ilde states, I can see these things being in modules.

Regardless, it's bad programming practise to be using something after
the final remove is called, it certainly violates the principle of least
surprise and would usually eventually cause problems.

James

Re: [PATCH v4] pm_qos: make update_request non blocking

[James Bottomley]

On Mon, 2010-06-14 at 16:33 +0200, Florian Mickler wrote:
> On Fri, 11 Jun 2010 09:25:52 -0500
> James Bottomley <James.Bottomley@suse.de> wrote:
> 
> > 
> > Actually, pm_qos_remove now needs a flush_scheduled work since you don't
> > want to return until the list is clear (since the next action may be to
> > free the object).
> 
> The work-items are allocated in the pm_qos objects (which get never
> freed), so we should be fine there.

That's not a safe assumption.  Once we get into drivers, timers and cpu
ilde states, I can see these things being in modules.

Regardless, it's bad programming practise to be using something after
the final remove is called, it certainly violates the principle of least
surprise and would usually eventually cause problems.

James

[PATCH 1/3] pm_qos: make update_request non blocking

[florian]

In order to allow drivers to use pm_qos_update_request from interrupt
context we call the notifiers via schedule_work().

Signed-off-by: Florian Mickler <florian@mickler.org>
---

No difference to "[PATCH v4] pm_qos: make update_request non blocking".

In the 3rd patch of this series we change the logic to use in_interrupt() to determine if it is 
necessary to schedule a workitem or if we can just execute the notifiers right away. 

As the pm_qos_objects are never destroyed, a flush_scheduled_work() is not needed.


kernel/pm_qos_params.c |   47 ++++++++++++++++++++++++++++++-----------------
 1 files changed, 30 insertions(+), 17 deletions(-)

diff --git a/kernel/pm_qos_params.c b/kernel/pm_qos_params.c
index f42d3f7..296343a 100644
--- a/kernel/pm_qos_params.c
+++ b/kernel/pm_qos_params.c
@@ -40,6 +40,7 @@
 #include <linux/string.h>
 #include <linux/platform_device.h>
 #include <linux/init.h>
+#include <linux/workqueue.h>
 
 #include <linux/uaccess.h>
 
@@ -60,11 +61,13 @@ struct pm_qos_request_list {
 
 static s32 max_compare(s32 v1, s32 v2);
 static s32 min_compare(s32 v1, s32 v2);
+static void update_notify(struct work_struct *work);
 
 struct pm_qos_object {
 	struct pm_qos_request_list requests;
 	struct blocking_notifier_head *notifiers;
 	struct miscdevice pm_qos_power_miscdev;
+	struct work_struct notify;
 	char *name;
 	s32 default_value;
 	atomic_t target_value;
@@ -72,10 +75,12 @@ struct pm_qos_object {
 };
 
 static struct pm_qos_object null_pm_qos;
+
 static BLOCKING_NOTIFIER_HEAD(cpu_dma_lat_notifier);
 static struct pm_qos_object cpu_dma_pm_qos = {
 	.requests = {LIST_HEAD_INIT(cpu_dma_pm_qos.requests.list)},
 	.notifiers = &cpu_dma_lat_notifier,
+	.notify = __WORK_INITIALIZER(cpu_dma_pm_qos.notify, update_notify),
 	.name = "cpu_dma_latency",
 	.default_value = 2000 * USEC_PER_SEC,
 	.target_value = ATOMIC_INIT(2000 * USEC_PER_SEC),
@@ -86,6 +91,7 @@ static BLOCKING_NOTIFIER_HEAD(network_lat_notifier);
 static struct pm_qos_object network_lat_pm_qos = {
 	.requests = {LIST_HEAD_INIT(network_lat_pm_qos.requests.list)},
 	.notifiers = &network_lat_notifier,
+	.notify = __WORK_INITIALIZER(network_lat_pm_qos.notify, update_notify),
 	.name = "network_latency",
 	.default_value = 2000 * USEC_PER_SEC,
 	.target_value = ATOMIC_INIT(2000 * USEC_PER_SEC),
@@ -97,13 +103,14 @@ static BLOCKING_NOTIFIER_HEAD(network_throughput_notifier);
 static struct pm_qos_object network_throughput_pm_qos = {
 	.requests = {LIST_HEAD_INIT(network_throughput_pm_qos.requests.list)},
 	.notifiers = &network_throughput_notifier,
+	.notify = __WORK_INITIALIZER(network_throughput_pm_qos.notify,
+			update_notify),
 	.name = "network_throughput",
 	.default_value = 0,
 	.target_value = ATOMIC_INIT(0),
 	.comparitor = max_compare
 };
 
-
 static struct pm_qos_object *pm_qos_array[] = {
 	&null_pm_qos,
 	&cpu_dma_pm_qos,
@@ -135,35 +142,40 @@ static s32 min_compare(s32 v1, s32 v2)
 	return min(v1, v2);
 }
 
+static void update_notify(struct work_struct *work)
+{
+	struct pm_qos_object *obj =
+		container_of(work, struct pm_qos_object, notify);
+
+	s32 extreme_value = atomic_read(&obj->target_value);
+	blocking_notifier_call_chain(
+		obj->notifiers,
+			(unsigned long) extreme_value, NULL);
+}
 
 static void update_target(int pm_qos_class)
 {
 	s32 extreme_value;
 	struct pm_qos_request_list *node;
+	struct pm_qos_object *obj = pm_qos_array[pm_qos_class];
 	unsigned long flags;
 	int call_notifier = 0;
 
 	spin_lock_irqsave(&pm_qos_lock, flags);
-	extreme_value = pm_qos_array[pm_qos_class]->default_value;
-	list_for_each_entry(node,
-			&pm_qos_array[pm_qos_class]->requests.list, list) {
-		extreme_value = pm_qos_array[pm_qos_class]->comparitor(
-				extreme_value, node->value);
-	}
-	if (atomic_read(&pm_qos_array[pm_qos_class]->target_value) !=
-			extreme_value) {
+	extreme_value = obj->default_value;
+	list_for_each_entry(node, &obj->requests.list, list)
+		extreme_value = obj->comparitor(extreme_value, node->value);
+
+	if (atomic_read(&obj->target_value) != extreme_value) {
 		call_notifier = 1;
-		atomic_set(&pm_qos_array[pm_qos_class]->target_value,
-				extreme_value);
+		atomic_set(&obj->target_value, extreme_value);
 		pr_debug(KERN_ERR "new target for qos %d is %d\n", pm_qos_class,
-			atomic_read(&pm_qos_array[pm_qos_class]->target_value));
+			atomic_read(&obj->target_value));
 	}
 	spin_unlock_irqrestore(&pm_qos_lock, flags);
 
 	if (call_notifier)
-		blocking_notifier_call_chain(
-				pm_qos_array[pm_qos_class]->notifiers,
-					(unsigned long) extreme_value, NULL);
+		schedule_work(&obj->notify);
 }
 
 static int register_pm_qos_misc(struct pm_qos_object *qos)
@@ -301,7 +313,7 @@ EXPORT_SYMBOL_GPL(pm_qos_remove_request);
  * @pm_qos_class: identifies which qos target changes should be notified.
  * @notifier: notifier block managed by caller.
  *
- * will register the notifier into a notification chain that gets called
+ * Will register the notifier into a notification chain that gets called
  * upon changes to the pm_qos_class target value.
  */
 int pm_qos_add_notifier(int pm_qos_class, struct notifier_block *notifier)
@@ -320,7 +332,7 @@ EXPORT_SYMBOL_GPL(pm_qos_add_notifier);
  * @pm_qos_class: identifies which qos target changes are notified.
  * @notifier: notifier block to be removed.
  *
- * will remove the notifier from the notification chain that gets called
+ * Will remove the notifier from the notification chain that gets called
  * upon changes to the pm_qos_class target value.
  */
 int pm_qos_remove_notifier(int pm_qos_class, struct notifier_block *notifier)
@@ -392,6 +404,7 @@ static int __init pm_qos_power_init(void)
 {
 	int ret = 0;
 
+
 	ret = register_pm_qos_misc(&cpu_dma_pm_qos);
 	if (ret < 0) {
 		printk(KERN_ERR "pm_qos_param: cpu_dma_latency setup failed\n");
-- 
1.7.1

[PATCH 2/3] pm_qos: add atomic notifier chain

[florian]

This allows for atomic notifications.
This may be necessary to have in some future scenarios,
as it eliminates any scheduling between update_request and doing the
notifier_callback.
---

I took the naming of the interface functions James Bottomly suggested in an earlier patch.

 include/linux/pm_qos_params.h |    1 +
 kernel/pm_qos_params.c        |   76 ++++++++++++++++++++++++++++++++---------
 2 files changed, 61 insertions(+), 16 deletions(-)

diff --git a/include/linux/pm_qos_params.h b/include/linux/pm_qos_params.h
index 8ba440e..0f41378 100644
--- a/include/linux/pm_qos_params.h
+++ b/include/linux/pm_qos_params.h
@@ -23,5 +23,6 @@ void pm_qos_remove_request(struct pm_qos_request_list *pm_qos_req);
 
 int pm_qos_request(int pm_qos_class);
 int pm_qos_add_notifier(int pm_qos_class, struct notifier_block *notifier);
+int pm_qos_add_atomic_notifier(int pm_qos_class, struct notifier_block *notifier);
 int pm_qos_remove_notifier(int pm_qos_class, struct notifier_block *notifier);
 
diff --git a/kernel/pm_qos_params.c b/kernel/pm_qos_params.c
index 296343a..9346906 100644
--- a/kernel/pm_qos_params.c
+++ b/kernel/pm_qos_params.c
@@ -65,7 +65,8 @@ static void update_notify(struct work_struct *work);
 
 struct pm_qos_object {
 	struct pm_qos_request_list requests;
-	struct blocking_notifier_head *notifiers;
+	struct blocking_notifier_head *blocking_notifiers;
+	struct atomic_notifier_head *atomic_notifiers;
 	struct miscdevice pm_qos_power_miscdev;
 	struct work_struct notify;
 	char *name;
@@ -76,21 +77,25 @@ struct pm_qos_object {
 
 static struct pm_qos_object null_pm_qos;
 
-static BLOCKING_NOTIFIER_HEAD(cpu_dma_lat_notifier);
+static BLOCKING_NOTIFIER_HEAD(cpu_dma_lat_notifier_blocking);
+static ATOMIC_NOTIFIER_HEAD(cpu_dma_lat_notifier_atomic);
 static struct pm_qos_object cpu_dma_pm_qos = {
 	.requests = {LIST_HEAD_INIT(cpu_dma_pm_qos.requests.list)},
-	.notifiers = &cpu_dma_lat_notifier,
+	.blocking_notifiers = &cpu_dma_lat_notifier_blocking,
+	.atomic_notifiers = &cpu_dma_lat_notifier_atomic,
 	.notify = __WORK_INITIALIZER(cpu_dma_pm_qos.notify, update_notify),
 	.name = "cpu_dma_latency",
 	.default_value = 2000 * USEC_PER_SEC,
 	.target_value = ATOMIC_INIT(2000 * USEC_PER_SEC),
-	.comparitor = min_compare
+	.comparitor = min_compare,
 };
 
-static BLOCKING_NOTIFIER_HEAD(network_lat_notifier);
+static BLOCKING_NOTIFIER_HEAD(network_lat_notifier_blocking);
+static ATOMIC_NOTIFIER_HEAD(network_lat_notifier_atomic);
 static struct pm_qos_object network_lat_pm_qos = {
 	.requests = {LIST_HEAD_INIT(network_lat_pm_qos.requests.list)},
-	.notifiers = &network_lat_notifier,
+	.blocking_notifiers = &network_lat_notifier_blocking,
+	.atomic_notifiers = &network_lat_notifier_atomic,
 	.notify = __WORK_INITIALIZER(network_lat_pm_qos.notify, update_notify),
 	.name = "network_latency",
 	.default_value = 2000 * USEC_PER_SEC,
@@ -99,10 +104,12 @@ static struct pm_qos_object network_lat_pm_qos = {
 };
 
 
-static BLOCKING_NOTIFIER_HEAD(network_throughput_notifier);
+static BLOCKING_NOTIFIER_HEAD(network_throughput_notifier_blocking);
+static ATOMIC_NOTIFIER_HEAD(network_throughput_notifier_atomic);
 static struct pm_qos_object network_throughput_pm_qos = {
 	.requests = {LIST_HEAD_INIT(network_throughput_pm_qos.requests.list)},
-	.notifiers = &network_throughput_notifier,
+	.blocking_notifiers = &network_throughput_notifier_blocking,
+	.atomic_notifiers = &network_throughput_notifier_atomic,
 	.notify = __WORK_INITIALIZER(network_throughput_pm_qos.notify,
 			update_notify),
 	.name = "network_throughput",
@@ -142,6 +149,16 @@ static s32 min_compare(s32 v1, s32 v2)
 	return min(v1, v2);
 }
 
+static void pm_qos_call_notifiers(struct pm_qos_object *o,
+				  unsigned long curr_value)
+{
+	schedule_work(&o->notify);
+
+	if (o->atomic_notifiers)
+		atomic_notifier_call_chain(o->atomic_notifiers,
+					   curr_value, NULL);
+}
+
 static void update_notify(struct work_struct *work)
 {
 	struct pm_qos_object *obj =
@@ -149,7 +166,7 @@ static void update_notify(struct work_struct *work)
 
 	s32 extreme_value = atomic_read(&obj->target_value);
 	blocking_notifier_call_chain(
-		obj->notifiers,
+		obj->blocking_notifiers,
 			(unsigned long) extreme_value, NULL);
 }
 
@@ -175,7 +192,7 @@ static void update_target(int pm_qos_class)
 	spin_unlock_irqrestore(&pm_qos_lock, flags);
 
 	if (call_notifier)
-		schedule_work(&obj->notify);
+		pm_qos_call_notifiers(obj, extreme_value);
 }
 
 static int register_pm_qos_misc(struct pm_qos_object *qos)
@@ -310,24 +327,48 @@ EXPORT_SYMBOL_GPL(pm_qos_remove_request);
 
 /**
  * pm_qos_add_notifier - sets notification entry for changes to target value
- * @pm_qos_class: identifies which qos target changes should be notified.
+ * @pm_qos_class: identifies which qos target changes should trigger
+ *   	notifications.
  * @notifier: notifier block managed by caller.
  *
  * Will register the notifier into a notification chain that gets called
  * upon changes to the pm_qos_class target value.
  */
-int pm_qos_add_notifier(int pm_qos_class, struct notifier_block *notifier)
+int pm_qos_add_notifier(int pm_qos_class, struct notifier_block *nb)
 {
 	int retval;
 
 	retval = blocking_notifier_chain_register(
-			pm_qos_array[pm_qos_class]->notifiers, notifier);
+			pm_qos_array[pm_qos_class]->blocking_notifiers, nb);
 
 	return retval;
 }
 EXPORT_SYMBOL_GPL(pm_qos_add_notifier);
 
 /**
+ * pm_qos_add_atomic_notifier - sets notification entry for changes to target value
+ * @pm_qos_class: identifies which qos target changes should trigger
+ * 	notifications.
+ * @notifier: notifier block managed by caller.
+ *
+ * Will register the notifier into a notification chain that gets
+ * called upon changes to the pm_qos_class target value.  The notifier
+ * may be called from atomic context.  use @pm_qos_remove_notifier to
+ * unregister.
+ *
+ */
+int pm_qos_add_atomic_notifier(int pm_qos_class, struct notifier_block *notifier)
+{
+	/* guard against programming error */
+	BUG_ON(!pm_qos_array[pm_qos_class]->atomic_notifiers);
+
+	return atomic_notifier_chain_register(
+		pm_qos_array[pm_qos_class]->atomic_notifiers,
+		notifier);
+}
+EXPORT_SYMBOL_GPL(pm_qos_add_atomic_notifier);
+
+/**
  * pm_qos_remove_notifier - deletes notification entry from chain.
  * @pm_qos_class: identifies which qos target changes are notified.
  * @notifier: notifier block to be removed.
@@ -335,13 +376,16 @@ EXPORT_SYMBOL_GPL(pm_qos_add_notifier);
  * Will remove the notifier from the notification chain that gets called
  * upon changes to the pm_qos_class target value.
  */
-int pm_qos_remove_notifier(int pm_qos_class, struct notifier_block *notifier)
+int pm_qos_remove_notifier(int pm_qos_class, struct notifier_block *nb)
 {
 	int retval;
 
 	retval = blocking_notifier_chain_unregister(
-			pm_qos_array[pm_qos_class]->notifiers, notifier);
-
+			pm_qos_array[pm_qos_class]->blocking_notifiers, nb);
+	if (retval) {
+		retval = atomic_notifier_chain_unregister(
+			pm_qos_array[pm_qos_class]->atomic_notifiers, nb);
+	}
 	return retval;
 }
 EXPORT_SYMBOL_GPL(pm_qos_remove_notifier);
-- 
1.7.1

[PATCH 3/3] pm_qos: only schedule work when in interrupt context

[florian]

With this patch we only schedule the work when in interrupt context.

Before update_request was callable from interrupt-context there was a
1:1 relation between a change in the request-value and a notification.
This patch restores that behaviour for all constraints that have update_request
never called from interrupt context.

The notifier mutex serializes calls to blocking_notifier_call_chain, so
that we are serialized against any pending or currently executing notification.

Signed-off-by: Florian Mickler <florian@mickler.org>
---
 kernel/pm_qos_params.c |   10 +++++++---
 1 files changed, 7 insertions(+), 3 deletions(-)

diff --git a/kernel/pm_qos_params.c b/kernel/pm_qos_params.c
index 9346906..c06cae9 100644
--- a/kernel/pm_qos_params.c
+++ b/kernel/pm_qos_params.c
@@ -152,11 +152,15 @@ static s32 min_compare(s32 v1, s32 v2)
 static void pm_qos_call_notifiers(struct pm_qos_object *o,
 				  unsigned long curr_value)
 {
-	schedule_work(&o->notify);
-
 	if (o->atomic_notifiers)
 		atomic_notifier_call_chain(o->atomic_notifiers,
-					   curr_value, NULL);
+				(unsigned long) curr_value, NULL);
+
+	if (in_interrupt()) 
+		schedule_work(&o->notify);
+	else 
+		blocking_notifier_call_chain(o->blocking_notifiers, 
+				(unsigned long) curr_value, NULL);
 }
 
 static void update_notify(struct work_struct *work)
-- 
1.7.1

Re: [linux-pm] [PATCH v4] pm_qos: make update_request non blocking

[Florian Mickler]

On Mon, 14 Jun 2010 09:44:06 -0500
James Bottomley <James.Bottomley@suse.de> wrote:

> On Mon, 2010-06-14 at 16:33 +0200, Florian Mickler wrote:
> > On Fri, 11 Jun 2010 09:25:52 -0500
> > James Bottomley <James.Bottomley@suse.de> wrote:
> > 
> > > 
> > > Actually, pm_qos_remove now needs a flush_scheduled work since you don't
> > > want to return until the list is clear (since the next action may be to
> > > free the object).
> > 
> > The work-items are allocated in the pm_qos objects (which get never
> > freed), so we should be fine there.
> 
> That's not a safe assumption.  Once we get into drivers, timers and cpu
> ilde states, I can see these things being in modules.
> 
> Regardless, it's bad programming practise to be using something after
> the final remove is called, it certainly violates the principle of least
> surprise and would usually eventually cause problems.
> 
> James
> 

I absolutely defer to you in this question. But there is no
pm_qos_remove at the moment, as far as I see? Should I add one? When
and how would it be called?

Maybe I'm not understanding you right at the moment. 

Cheers,
Flo

Re: [PATCH v4] pm_qos: make update_request non blocking

[Florian Mickler]

On Mon, 14 Jun 2010 09:44:06 -0500
James Bottomley <James.Bottomley@suse.de> wrote:

> On Mon, 2010-06-14 at 16:33 +0200, Florian Mickler wrote:
> > On Fri, 11 Jun 2010 09:25:52 -0500
> > James Bottomley <James.Bottomley@suse.de> wrote:
> > 
> > > 
> > > Actually, pm_qos_remove now needs a flush_scheduled work since you don't
> > > want to return until the list is clear (since the next action may be to
> > > free the object).
> > 
> > The work-items are allocated in the pm_qos objects (which get never
> > freed), so we should be fine there.
> 
> That's not a safe assumption.  Once we get into drivers, timers and cpu
> ilde states, I can see these things being in modules.
> 
> Regardless, it's bad programming practise to be using something after
> the final remove is called, it certainly violates the principle of least
> surprise and would usually eventually cause problems.
> 
> James
> 

I absolutely defer to you in this question. But there is no
pm_qos_remove at the moment, as far as I see? Should I add one? When
and how would it be called?

Maybe I'm not understanding you right at the moment. 

Cheers,
Flo

Re: [linux-pm] [PATCH v4] pm_qos: make update_request non blocking

[James Bottomley]

On Mon, 2010-06-14 at 16:49 +0200, Florian Mickler wrote:
> On Mon, 14 Jun 2010 09:44:06 -0500
> James Bottomley <James.Bottomley@suse.de> wrote:
> 
> > On Mon, 2010-06-14 at 16:33 +0200, Florian Mickler wrote:
> > > On Fri, 11 Jun 2010 09:25:52 -0500
> > > James Bottomley <James.Bottomley@suse.de> wrote:
> > > 
> > > > 
> > > > Actually, pm_qos_remove now needs a flush_scheduled work since you don't
> > > > want to return until the list is clear (since the next action may be to
> > > > free the object).
> > > 
> > > The work-items are allocated in the pm_qos objects (which get never
> > > freed), so we should be fine there.
> > 
> > That's not a safe assumption.  Once we get into drivers, timers and cpu
> > ilde states, I can see these things being in modules.
> > 
> > Regardless, it's bad programming practise to be using something after
> > the final remove is called, it certainly violates the principle of least
> > surprise and would usually eventually cause problems.
> > 
> > James
> > 
> 
> I absolutely defer to you in this question. But there is no
> pm_qos_remove at the moment, as far as I see? Should I add one? When
> and how would it be called?
> 
> Maybe I'm not understanding you right at the moment. 

You need to flush in pm_qos_remove_notifier() to make sure you aren't
racing deferred work in the removed notifier with a return from remove,
so that the outcome is always consistent (i.e. the notifier sees all
pending events before removal).  On closer inspection, it looks like the
notifier mutexes are sufficient to make sure it doesn't return running
the notifier to be removed ... but that does mean that add and remove
require user context.

James

Re: [PATCH v4] pm_qos: make update_request non blocking

[James Bottomley]

On Mon, 2010-06-14 at 16:49 +0200, Florian Mickler wrote:
> On Mon, 14 Jun 2010 09:44:06 -0500
> James Bottomley <James.Bottomley@suse.de> wrote:
> 
> > On Mon, 2010-06-14 at 16:33 +0200, Florian Mickler wrote:
> > > On Fri, 11 Jun 2010 09:25:52 -0500
> > > James Bottomley <James.Bottomley@suse.de> wrote:
> > > 
> > > > 
> > > > Actually, pm_qos_remove now needs a flush_scheduled work since you don't
> > > > want to return until the list is clear (since the next action may be to
> > > > free the object).
> > > 
> > > The work-items are allocated in the pm_qos objects (which get never
> > > freed), so we should be fine there.
> > 
> > That's not a safe assumption.  Once we get into drivers, timers and cpu
> > ilde states, I can see these things being in modules.
> > 
> > Regardless, it's bad programming practise to be using something after
> > the final remove is called, it certainly violates the principle of least
> > surprise and would usually eventually cause problems.
> > 
> > James
> > 
> 
> I absolutely defer to you in this question. But there is no
> pm_qos_remove at the moment, as far as I see? Should I add one? When
> and how would it be called?
> 
> Maybe I'm not understanding you right at the moment. 

You need to flush in pm_qos_remove_notifier() to make sure you aren't
racing deferred work in the removed notifier with a return from remove,
so that the outcome is always consistent (i.e. the notifier sees all
pending events before removal).  On closer inspection, it looks like the
notifier mutexes are sufficient to make sure it doesn't return running
the notifier to be removed ... but that does mean that add and remove
require user context.

James

Re: [linux-pm] [PATCH v4] pm_qos: make update_request non blocking

[Florian Mickler]

On Mon, 14 Jun 2010 10:10:06 -0500
James Bottomley <James.Bottomley@suse.de> wrote:
 
> On closer inspection, it looks like the
> notifier mutexes are sufficient to make sure it doesn't return running
> the notifier to be removed ... but that does mean that add and remove
> require user context.
> 
> James
> 

Yes. I don't think we need to make the add and remove callable from
interrupt context?

Cheers,
Flo

Re: [PATCH v4] pm_qos: make update_request non blocking

[Florian Mickler]

On Mon, 14 Jun 2010 10:10:06 -0500
James Bottomley <James.Bottomley@suse.de> wrote:
 
> On closer inspection, it looks like the
> notifier mutexes are sufficient to make sure it doesn't return running
> the notifier to be removed ... but that does mean that add and remove
> require user context.
> 
> James
> 

Yes. I don't think we need to make the add and remove callable from
interrupt context?

Cheers,
Flo

Re: [PATCH 3/3] pm_qos: only schedule work when in interrupt context

[Florian Mickler]

Hi James!

On Mon, 14 Jun 2010 16:46:40 +0200
florian@mickler.org wrote:

> With this patch we only schedule the work when in interrupt context.
> 
> Before update_request was callable from interrupt-context there was a
> 1:1 relation between a change in the request-value and a notification.
> This patch restores that behaviour for all constraints that have update_request
> never called from interrupt context.
> 
> The notifier mutex serializes calls to blocking_notifier_call_chain, so
> that we are serialized against any pending or currently executing notification.
> 
> Signed-off-by: Florian Mickler <florian@mickler.org>
> ---
>  kernel/pm_qos_params.c |   10 +++++++---
>  1 files changed, 7 insertions(+), 3 deletions(-)
> 
> diff --git a/kernel/pm_qos_params.c b/kernel/pm_qos_params.c
> index 9346906..c06cae9 100644
> --- a/kernel/pm_qos_params.c
> +++ b/kernel/pm_qos_params.c
> @@ -152,11 +152,15 @@ static s32 min_compare(s32 v1, s32 v2)
>  static void pm_qos_call_notifiers(struct pm_qos_object *o,
>  				  unsigned long curr_value)
>  {
> -	schedule_work(&o->notify);
> -
>  	if (o->atomic_notifiers)
>  		atomic_notifier_call_chain(o->atomic_notifiers,
> -					   curr_value, NULL);
> +				(unsigned long) curr_value, NULL);
> +
> +	if (in_interrupt()) 
> +		schedule_work(&o->notify);
> +	else 
> +		blocking_notifier_call_chain(o->blocking_notifiers, 
> +				(unsigned long) curr_value, NULL);
>  }
>  
>  static void update_notify(struct work_struct *work)

What about this? Is this ok? I don't know if it is benign to use
in_interrupt() here. I took this idea from the
execute_in_process_context() implementation. 


If this is ok, should I rebase them on your two pm_qos patches (plists
and the kzalloc removal)? 

Did you already thought about some debugging stuff that would suffice
the android needs? I kind of thought about either registerieng some
notifier callback or using the perf/tracing infrastructure. 
But I have not looked into it yet.

Cheers,
Flo

Re: [PATCH 3/3] pm_qos: only schedule work when in interrupt context

[Florian Mickler]

Hi James!

On Mon, 14 Jun 2010 16:46:40 +0200
florian@mickler.org wrote:

> With this patch we only schedule the work when in interrupt context.
> 
> Before update_request was callable from interrupt-context there was a
> 1:1 relation between a change in the request-value and a notification.
> This patch restores that behaviour for all constraints that have update_request
> never called from interrupt context.
> 
> The notifier mutex serializes calls to blocking_notifier_call_chain, so
> that we are serialized against any pending or currently executing notification.
> 
> Signed-off-by: Florian Mickler <florian@mickler.org>
> ---
>  kernel/pm_qos_params.c |   10 +++++++---
>  1 files changed, 7 insertions(+), 3 deletions(-)
> 
> diff --git a/kernel/pm_qos_params.c b/kernel/pm_qos_params.c
> index 9346906..c06cae9 100644
> --- a/kernel/pm_qos_params.c
> +++ b/kernel/pm_qos_params.c
> @@ -152,11 +152,15 @@ static s32 min_compare(s32 v1, s32 v2)
>  static void pm_qos_call_notifiers(struct pm_qos_object *o,
>  				  unsigned long curr_value)
>  {
> -	schedule_work(&o->notify);
> -
>  	if (o->atomic_notifiers)
>  		atomic_notifier_call_chain(o->atomic_notifiers,
> -					   curr_value, NULL);
> +				(unsigned long) curr_value, NULL);
> +
> +	if (in_interrupt()) 
> +		schedule_work(&o->notify);
> +	else 
> +		blocking_notifier_call_chain(o->blocking_notifiers, 
> +				(unsigned long) curr_value, NULL);
>  }
>  
>  static void update_notify(struct work_struct *work)

What about this? Is this ok? I don't know if it is benign to use
in_interrupt() here. I took this idea from the
execute_in_process_context() implementation. 


If this is ok, should I rebase them on your two pm_qos patches (plists
and the kzalloc removal)? 

Did you already thought about some debugging stuff that would suffice
the android needs? I kind of thought about either registerieng some
notifier callback or using the perf/tracing infrastructure. 
But I have not looked into it yet.

Cheers,
Flo

Re: [PATCH 3/3] pm_qos: only schedule work when in interrupt context

[James Bottomley]

On Tue, 2010-06-15 at 19:23 +0200, Florian Mickler wrote:
> Hi James!
> 
> On Mon, 14 Jun 2010 16:46:40 +0200
> florian@mickler.org wrote:
> 
> > With this patch we only schedule the work when in interrupt context.
> > 
> > Before update_request was callable from interrupt-context there was a
> > 1:1 relation between a change in the request-value and a notification.
> > This patch restores that behaviour for all constraints that have update_request
> > never called from interrupt context.
> > 
> > The notifier mutex serializes calls to blocking_notifier_call_chain, so
> > that we are serialized against any pending or currently executing notification.
> > 
> > Signed-off-by: Florian Mickler <florian@mickler.org>
> > ---
> >  kernel/pm_qos_params.c |   10 +++++++---
> >  1 files changed, 7 insertions(+), 3 deletions(-)
> > 
> > diff --git a/kernel/pm_qos_params.c b/kernel/pm_qos_params.c
> > index 9346906..c06cae9 100644
> > --- a/kernel/pm_qos_params.c
> > +++ b/kernel/pm_qos_params.c
> > @@ -152,11 +152,15 @@ static s32 min_compare(s32 v1, s32 v2)
> >  static void pm_qos_call_notifiers(struct pm_qos_object *o,
> >  				  unsigned long curr_value)
> >  {
> > -	schedule_work(&o->notify);
> > -
> >  	if (o->atomic_notifiers)
> >  		atomic_notifier_call_chain(o->atomic_notifiers,
> > -					   curr_value, NULL);
> > +				(unsigned long) curr_value, NULL);
> > +
> > +	if (in_interrupt()) 
> > +		schedule_work(&o->notify);
> > +	else 
> > +		blocking_notifier_call_chain(o->blocking_notifiers, 
> > +				(unsigned long) curr_value, NULL);
> >  }
> >  
> >  static void update_notify(struct work_struct *work)
> 
> What about this? Is this ok? I don't know if it is benign to use
> in_interrupt() here. I took this idea from the
> execute_in_process_context() implementation. 

I think it will work ... but I still think it's over complex given the
listed requirements (android seems to only want atomic notifiers from
atomic contexts).

> If this is ok, should I rebase them on your two pm_qos patches (plists
> and the kzalloc removal)? 

Well, I would say yes.  However, for more impartial advice, I'd wait and
see what the pm maintainers want.

> Did you already thought about some debugging stuff that would suffice
> the android needs? I kind of thought about either registerieng some
> notifier callback or using the perf/tracing infrastructure. 
> But I have not looked into it yet.

I was just going to try the conversion when the wakelocks stuff was
finally in and see if it worked in an android kernel.

James

Re: [PATCH 3/3] pm_qos: only schedule work when in interrupt context

[James Bottomley]

On Tue, 2010-06-15 at 19:23 +0200, Florian Mickler wrote:
> Hi James!
> 
> On Mon, 14 Jun 2010 16:46:40 +0200
> florian@mickler.org wrote:
> 
> > With this patch we only schedule the work when in interrupt context.
> > 
> > Before update_request was callable from interrupt-context there was a
> > 1:1 relation between a change in the request-value and a notification.
> > This patch restores that behaviour for all constraints that have update_request
> > never called from interrupt context.
> > 
> > The notifier mutex serializes calls to blocking_notifier_call_chain, so
> > that we are serialized against any pending or currently executing notification.
> > 
> > Signed-off-by: Florian Mickler <florian@mickler.org>
> > ---
> >  kernel/pm_qos_params.c |   10 +++++++---
> >  1 files changed, 7 insertions(+), 3 deletions(-)
> > 
> > diff --git a/kernel/pm_qos_params.c b/kernel/pm_qos_params.c
> > index 9346906..c06cae9 100644
> > --- a/kernel/pm_qos_params.c
> > +++ b/kernel/pm_qos_params.c
> > @@ -152,11 +152,15 @@ static s32 min_compare(s32 v1, s32 v2)
> >  static void pm_qos_call_notifiers(struct pm_qos_object *o,
> >  				  unsigned long curr_value)
> >  {
> > -	schedule_work(&o->notify);
> > -
> >  	if (o->atomic_notifiers)
> >  		atomic_notifier_call_chain(o->atomic_notifiers,
> > -					   curr_value, NULL);
> > +				(unsigned long) curr_value, NULL);
> > +
> > +	if (in_interrupt()) 
> > +		schedule_work(&o->notify);
> > +	else 
> > +		blocking_notifier_call_chain(o->blocking_notifiers, 
> > +				(unsigned long) curr_value, NULL);
> >  }
> >  
> >  static void update_notify(struct work_struct *work)
> 
> What about this? Is this ok? I don't know if it is benign to use
> in_interrupt() here. I took this idea from the
> execute_in_process_context() implementation. 

I think it will work ... but I still think it's over complex given the
listed requirements (android seems to only want atomic notifiers from
atomic contexts).

> If this is ok, should I rebase them on your two pm_qos patches (plists
> and the kzalloc removal)? 

Well, I would say yes.  However, for more impartial advice, I'd wait and
see what the pm maintainers want.

> Did you already thought about some debugging stuff that would suffice
> the android needs? I kind of thought about either registerieng some
> notifier callback or using the perf/tracing infrastructure. 
> But I have not looked into it yet.

I was just going to try the conversion when the wakelocks stuff was
finally in and see if it worked in an android kernel.

James