[PATCH] pm_qos: make update_request non blocking

[PATCH] pm_qos: make update_request non blocking

[florian]

First we use atomic_notifier to get the spinlocked variant.
Secondly we call the notifiers via schedule_work.

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

Please advise.

 kernel/pm_qos_params.c |   33 ++++++++++++++++++++++-----------
 1 files changed, 22 insertions(+), 11 deletions(-)

diff --git a/kernel/pm_qos_params.c b/kernel/pm_qos_params.c
index f42d3f7..7335c27 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>
 
@@ -63,7 +64,7 @@ static s32 min_compare(s32 v1, s32 v2);
 
 struct pm_qos_object {
 	struct pm_qos_request_list requests;
-	struct blocking_notifier_head *notifiers;
+	struct atomic_notifier_head *notifiers;
 	struct miscdevice pm_qos_power_miscdev;
 	char *name;
 	s32 default_value;
@@ -72,7 +73,7 @@ struct pm_qos_object {
 };
 
 static struct pm_qos_object null_pm_qos;
-static BLOCKING_NOTIFIER_HEAD(cpu_dma_lat_notifier);
+static ATOMIC_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,
@@ -82,7 +83,7 @@ static struct pm_qos_object cpu_dma_pm_qos = {
 	.comparitor = min_compare
 };
 
-static BLOCKING_NOTIFIER_HEAD(network_lat_notifier);
+static ATOMIC_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,
@@ -93,7 +94,7 @@ static struct pm_qos_object network_lat_pm_qos = {
 };
 
 
-static BLOCKING_NOTIFIER_HEAD(network_throughput_notifier);
+static ATOMIC_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,
@@ -103,7 +104,6 @@ static struct pm_qos_object network_throughput_pm_qos = {
 	.comparitor = max_compare
 };
 
-
 static struct pm_qos_object *pm_qos_array[] = {
 	&null_pm_qos,
 	&cpu_dma_pm_qos,
@@ -135,6 +135,15 @@ static s32 min_compare(s32 v1, s32 v2)
 	return min(v1, v2);
 }
 
+static void update_notify(struct work_struct* work){
+	int pm_qos_class = atomic_long_read(&work->data);
+	
+	s32 extreme_value = atomic_read(&pm_qos_array[pm_qos_class]->target_value);
+	atomic_notifier_call_chain(
+		pm_qos_array[pm_qos_class]->notifiers,
+			(unsigned long) extreme_value, NULL);
+}
+struct work_struct pm_qos_update_notify_work;
 
 static void update_target(int pm_qos_class)
 {
@@ -160,10 +169,10 @@ static void update_target(int pm_qos_class)
 	}
 	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);
+	if (call_notifier){
+		atomic_long_set(&pm_qos_update_notify_work.data, pm_qos_class);
+		schedule_work(&pm_qos_update_notify_work);
+	}
 }
 
 static int register_pm_qos_misc(struct pm_qos_object *qos)
@@ -308,7 +317,7 @@ int pm_qos_add_notifier(int pm_qos_class, struct notifier_block *notifier)
 {
 	int retval;
 
-	retval = blocking_notifier_chain_register(
+	retval = atomic_notifier_chain_register(
 			pm_qos_array[pm_qos_class]->notifiers, notifier);
 
 	return retval;
@@ -327,7 +336,7 @@ int pm_qos_remove_notifier(int pm_qos_class, struct notifier_block *notifier)
 {
 	int retval;
 
-	retval = blocking_notifier_chain_unregister(
+	retval = atomic_notifier_chain_unregister(
 			pm_qos_array[pm_qos_class]->notifiers, notifier);
 
 	return retval;
@@ -380,6 +389,8 @@ static ssize_t pm_qos_power_write(struct file *filp, const char __user *buf,
 		pr_debug(KERN_ERR "%s, %d, 0x%x\n", ascii_value, x, value);
 	} else
 		return -EINVAL;
+	
+	INIT_WORK(&pm_qos_update_notify_work,update_notify);
 
 	pm_qos_req = (struct pm_qos_request_list *)filp->private_data;
 	pm_qos_update_request(pm_qos_req, value);
-- 
1.7.1

Re: [PATCH] pm_qos: make update_request non blocking

[James Bottomley]

On Sat, 2010-06-05 at 22:46 +0200, florian@mickler.org wrote:
> First we use atomic_notifier to get the spinlocked variant.
> Secondly we call the notifiers via schedule_work.
> 
> Signed-off-by: Florian Mickler <florian@mickler.org>
> ---
> 
> Please advise.
> 
>  kernel/pm_qos_params.c |   33 ++++++++++++++++++++++-----------
>  1 files changed, 22 insertions(+), 11 deletions(-)
> 
> diff --git a/kernel/pm_qos_params.c b/kernel/pm_qos_params.c
> index f42d3f7..7335c27 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>
>  
> @@ -63,7 +64,7 @@ static s32 min_compare(s32 v1, s32 v2);
>  
>  struct pm_qos_object {
>  	struct pm_qos_request_list requests;
> -	struct blocking_notifier_head *notifiers;
> +	struct atomic_notifier_head *notifiers;
>  	struct miscdevice pm_qos_power_miscdev;
>  	char *name;
>  	s32 default_value;
> @@ -72,7 +73,7 @@ struct pm_qos_object {
>  };
>  
>  static struct pm_qos_object null_pm_qos;
> -static BLOCKING_NOTIFIER_HEAD(cpu_dma_lat_notifier);
> +static ATOMIC_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,
> @@ -82,7 +83,7 @@ static struct pm_qos_object cpu_dma_pm_qos = {
>  	.comparitor = min_compare
>  };
>  
> -static BLOCKING_NOTIFIER_HEAD(network_lat_notifier);
> +static ATOMIC_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,
> @@ -93,7 +94,7 @@ static struct pm_qos_object network_lat_pm_qos = {
>  };
>  
> 
> -static BLOCKING_NOTIFIER_HEAD(network_throughput_notifier);
> +static ATOMIC_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,
> @@ -103,7 +104,6 @@ static struct pm_qos_object network_throughput_pm_qos = {
>  	.comparitor = max_compare
>  };
>  
> -
>  static struct pm_qos_object *pm_qos_array[] = {
>  	&null_pm_qos,
>  	&cpu_dma_pm_qos,
> @@ -135,6 +135,15 @@ static s32 min_compare(s32 v1, s32 v2)
>  	return min(v1, v2);
>  }
>  
> +static void update_notify(struct work_struct* work){
> +	int pm_qos_class = atomic_long_read(&work->data);
> +	
> +	s32 extreme_value = atomic_read(&pm_qos_array[pm_qos_class]->target_value);
> +	atomic_notifier_call_chain(
> +		pm_qos_array[pm_qos_class]->notifiers,
> +			(unsigned long) extreme_value, NULL);
> +}
> +struct work_struct pm_qos_update_notify_work;
>  
>  static void update_target(int pm_qos_class)
>  {
> @@ -160,10 +169,10 @@ static void update_target(int pm_qos_class)
>  	}
>  	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);
> +	if (call_notifier){
> +		atomic_long_set(&pm_qos_update_notify_work.data, pm_qos_class);
> +		schedule_work(&pm_qos_update_notify_work);

Actually, you should use execute_in_process_context for this ... that
way any current call from user context won't change semantics (i.e. the
notifier will be executed before the call finishes).

I also suspect we might need an atomic notifier chain to be called
in-line ... things like the dma latency notifier need in-line
notification, I can see that some android ones would at all.  However,
this question can be deferred until an actual use case is seen.

Also, your implementation is racy: you can't have a single
pm_qos_update_notify_work because that means that only one thread can
use it at once ... and that means the work it is scheduling has to be
begun before it can be re-used.  Currently, any thread can blow away the
queued work item, which will likely oops the system.

James

Re: [PATCH] pm_qos: make update_request non blocking

[Florian Mickler]

On Sat, 05 Jun 2010 15:57:10 -0500
James Bottomley <James.Bottomley@suse.de> wrote:

> On Sat, 2010-06-05 at 22:46 +0200, florian@mickler.org wrote:
> > First we use atomic_notifier to get the spinlocked variant.
> > Secondly we call the notifiers via schedule_work.
> > 
> > Signed-off-by: Florian Mickler <florian@mickler.org>
> > ---
> > 
> > Please advise.
> > 
> >  kernel/pm_qos_params.c |   33 ++++++++++++++++++++++-----------
> >  1 files changed, 22 insertions(+), 11 deletions(-)
> > 
> > diff --git a/kernel/pm_qos_params.c b/kernel/pm_qos_params.c
> > index f42d3f7..7335c27 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>
> >  
> > @@ -63,7 +64,7 @@ static s32 min_compare(s32 v1, s32 v2);
> >  
> >  struct pm_qos_object {
> >  	struct pm_qos_request_list requests;
> > -	struct blocking_notifier_head *notifiers;
> > +	struct atomic_notifier_head *notifiers;
> >  	struct miscdevice pm_qos_power_miscdev;
> >  	char *name;
> >  	s32 default_value;
> > @@ -72,7 +73,7 @@ struct pm_qos_object {
> >  };
> >  
> >  static struct pm_qos_object null_pm_qos;
> > -static BLOCKING_NOTIFIER_HEAD(cpu_dma_lat_notifier);
> > +static ATOMIC_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,
> > @@ -82,7 +83,7 @@ static struct pm_qos_object cpu_dma_pm_qos = {
> >  	.comparitor = min_compare
> >  };
> >  
> > -static BLOCKING_NOTIFIER_HEAD(network_lat_notifier);
> > +static ATOMIC_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,
> > @@ -93,7 +94,7 @@ static struct pm_qos_object network_lat_pm_qos = {
> >  };
> >  
> > 
> > -static BLOCKING_NOTIFIER_HEAD(network_throughput_notifier);
> > +static ATOMIC_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,
> > @@ -103,7 +104,6 @@ static struct pm_qos_object network_throughput_pm_qos = {
> >  	.comparitor = max_compare
> >  };
> >  
> > -
> >  static struct pm_qos_object *pm_qos_array[] = {
> >  	&null_pm_qos,
> >  	&cpu_dma_pm_qos,
> > @@ -135,6 +135,15 @@ static s32 min_compare(s32 v1, s32 v2)
> >  	return min(v1, v2);
> >  }
> >  
> > +static void update_notify(struct work_struct* work){
> > +	int pm_qos_class = atomic_long_read(&work->data);
> > +	
> > +	s32 extreme_value = atomic_read(&pm_qos_array[pm_qos_class]->target_value);
> > +	atomic_notifier_call_chain(
> > +		pm_qos_array[pm_qos_class]->notifiers,
> > +			(unsigned long) extreme_value, NULL);
> > +}
> > +struct work_struct pm_qos_update_notify_work;
> >  
> >  static void update_target(int pm_qos_class)
> >  {
> > @@ -160,10 +169,10 @@ static void update_target(int pm_qos_class)
> >  	}
> >  	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);
> > +	if (call_notifier){
> > +		atomic_long_set(&pm_qos_update_notify_work.data, pm_qos_class);
> > +		schedule_work(&pm_qos_update_notify_work);
> 
> Actually, you should use execute_in_process_context for this ... that
> way any current call from user context won't change semantics (i.e. the
> notifier will be executed before the call finishes).

Ah, ok. Didn't know of its existence. 
After thinking a bit, I think it is crucial for the qos-infrastructure
to provide direct notifiers. For example, any missed change in latency
could impact the system.

> 
> I also suspect we might need an atomic notifier chain to be called
> in-line ... things like the dma latency notifier need in-line
> notification, I can see that some android ones would at all.  However,
> this question can be deferred until an actual use case is seen.

We should probably only provide an atomic queue and have all users
either schedule work or be quick, as it can be crucial to get the
information out there.

What do you think?

> 
> Also, your implementation is racy: you can't have a single
> pm_qos_update_notify_work because that means that only one thread can
> use it at once ... and that means the work it is scheduling has to be
> begun before it can be re-used.  Currently, any thread can blow away the
> queued work item, which will likely oops the system.

Indeed. I read the part about scheduling an work item that is on the
queue but didn't realize what it would mean in this case.


> 
> James
> 

Thanks!

Re: [PATCH] pm_qos: make update_request non blocking

[James Bottomley]

On Sat, 2010-06-05 at 23:21 +0200, Florian Mickler wrote:
> On Sat, 05 Jun 2010 15:57:10 -0500
> James Bottomley <James.Bottomley@suse.de> wrote:
> 
> > On Sat, 2010-06-05 at 22:46 +0200, florian@mickler.org wrote:
> > > First we use atomic_notifier to get the spinlocked variant.
> > > Secondly we call the notifiers via schedule_work.
> > > 
> > > Signed-off-by: Florian Mickler <florian@mickler.org>
> > > ---
> > > 
> > > Please advise.
> > > 
> > >  kernel/pm_qos_params.c |   33 ++++++++++++++++++++++-----------
> > >  1 files changed, 22 insertions(+), 11 deletions(-)
> > > 
> > > diff --git a/kernel/pm_qos_params.c b/kernel/pm_qos_params.c
> > > index f42d3f7..7335c27 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>
> > >  
> > > @@ -63,7 +64,7 @@ static s32 min_compare(s32 v1, s32 v2);
> > >  
> > >  struct pm_qos_object {
> > >  	struct pm_qos_request_list requests;
> > > -	struct blocking_notifier_head *notifiers;
> > > +	struct atomic_notifier_head *notifiers;
> > >  	struct miscdevice pm_qos_power_miscdev;
> > >  	char *name;
> > >  	s32 default_value;
> > > @@ -72,7 +73,7 @@ struct pm_qos_object {
> > >  };
> > >  
> > >  static struct pm_qos_object null_pm_qos;
> > > -static BLOCKING_NOTIFIER_HEAD(cpu_dma_lat_notifier);
> > > +static ATOMIC_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,
> > > @@ -82,7 +83,7 @@ static struct pm_qos_object cpu_dma_pm_qos = {
> > >  	.comparitor = min_compare
> > >  };
> > >  
> > > -static BLOCKING_NOTIFIER_HEAD(network_lat_notifier);
> > > +static ATOMIC_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,
> > > @@ -93,7 +94,7 @@ static struct pm_qos_object network_lat_pm_qos = {
> > >  };
> > >  
> > > 
> > > -static BLOCKING_NOTIFIER_HEAD(network_throughput_notifier);
> > > +static ATOMIC_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,
> > > @@ -103,7 +104,6 @@ static struct pm_qos_object network_throughput_pm_qos = {
> > >  	.comparitor = max_compare
> > >  };
> > >  
> > > -
> > >  static struct pm_qos_object *pm_qos_array[] = {
> > >  	&null_pm_qos,
> > >  	&cpu_dma_pm_qos,
> > > @@ -135,6 +135,15 @@ static s32 min_compare(s32 v1, s32 v2)
> > >  	return min(v1, v2);
> > >  }
> > >  
> > > +static void update_notify(struct work_struct* work){
> > > +	int pm_qos_class = atomic_long_read(&work->data);
> > > +	
> > > +	s32 extreme_value = atomic_read(&pm_qos_array[pm_qos_class]->target_value);
> > > +	atomic_notifier_call_chain(
> > > +		pm_qos_array[pm_qos_class]->notifiers,
> > > +			(unsigned long) extreme_value, NULL);
> > > +}
> > > +struct work_struct pm_qos_update_notify_work;
> > >  
> > >  static void update_target(int pm_qos_class)
> > >  {
> > > @@ -160,10 +169,10 @@ static void update_target(int pm_qos_class)
> > >  	}
> > >  	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);
> > > +	if (call_notifier){
> > > +		atomic_long_set(&pm_qos_update_notify_work.data, pm_qos_class);
> > > +		schedule_work(&pm_qos_update_notify_work);
> > 
> > Actually, you should use execute_in_process_context for this ... that
> > way any current call from user context won't change semantics (i.e. the
> > notifier will be executed before the call finishes).
> 
> Ah, ok. Didn't know of its existence. 
> After thinking a bit, I think it is crucial for the qos-infrastructure
> to provide direct notifiers. For example, any missed change in latency
> could impact the system.
> 
> > 
> > I also suspect we might need an atomic notifier chain to be called
> > in-line ... things like the dma latency notifier need in-line
> > notification, I can see that some android ones would at all.  However,
> > this question can be deferred until an actual use case is seen.
> 
> We should probably only provide an atomic queue and have all users
> either schedule work or be quick, as it can be crucial to get the
> information out there.
> 
> What do you think?

Yes, considering the use cases, I don't think we want to muck with
workqueues at all, so just do two queues per constraint, one blocking
and one atomic.  For a constraint  that has atomic update sites,
registering a blocking notifier would be illegal and we'd give a runtime
warning (we can even code a __might_sleep in there before the call).
That way we preserve current semantics and permit blocking notifiers to
be registered, but only if all the call sites have user context.

James

Re: [PATCH] pm_qos: make update_request non blocking

[mark gross]

On Sat, Jun 05, 2010 at 05:44:09PM -0500, James Bottomley wrote:
> On Sat, 2010-06-05 at 23:21 +0200, Florian Mickler wrote:
> > On Sat, 05 Jun 2010 15:57:10 -0500
> > James Bottomley <James.Bottomley@suse.de> wrote:
> > 
> > > On Sat, 2010-06-05 at 22:46 +0200, florian@mickler.org wrote:
> > > > First we use atomic_notifier to get the spinlocked variant.
> > > > Secondly we call the notifiers via schedule_work.
> > > > 
> > > > Signed-off-by: Florian Mickler <florian@mickler.org>
> > > > ---
> > > > 
> > > > Please advise.
> > > > 
> > > >  kernel/pm_qos_params.c |   33 ++++++++++++++++++++++-----------
> > > >  1 files changed, 22 insertions(+), 11 deletions(-)
> > > > 
> > > > diff --git a/kernel/pm_qos_params.c b/kernel/pm_qos_params.c
> > > > index f42d3f7..7335c27 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>
> > > >  
> > > > @@ -63,7 +64,7 @@ static s32 min_compare(s32 v1, s32 v2);
> > > >  
> > > >  struct pm_qos_object {
> > > >  	struct pm_qos_request_list requests;
> > > > -	struct blocking_notifier_head *notifiers;
> > > > +	struct atomic_notifier_head *notifiers;
> > > >  	struct miscdevice pm_qos_power_miscdev;
> > > >  	char *name;
> > > >  	s32 default_value;
> > > > @@ -72,7 +73,7 @@ struct pm_qos_object {
> > > >  };
> > > >  
> > > >  static struct pm_qos_object null_pm_qos;
> > > > -static BLOCKING_NOTIFIER_HEAD(cpu_dma_lat_notifier);
> > > > +static ATOMIC_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,
> > > > @@ -82,7 +83,7 @@ static struct pm_qos_object cpu_dma_pm_qos = {
> > > >  	.comparitor = min_compare
> > > >  };
> > > >  
> > > > -static BLOCKING_NOTIFIER_HEAD(network_lat_notifier);
> > > > +static ATOMIC_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,
> > > > @@ -93,7 +94,7 @@ static struct pm_qos_object network_lat_pm_qos = {
> > > >  };
> > > >  
> > > > 
> > > > -static BLOCKING_NOTIFIER_HEAD(network_throughput_notifier);
> > > > +static ATOMIC_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,
> > > > @@ -103,7 +104,6 @@ static struct pm_qos_object network_throughput_pm_qos = {
> > > >  	.comparitor = max_compare
> > > >  };
> > > >  
> > > > -
> > > >  static struct pm_qos_object *pm_qos_array[] = {
> > > >  	&null_pm_qos,
> > > >  	&cpu_dma_pm_qos,
> > > > @@ -135,6 +135,15 @@ static s32 min_compare(s32 v1, s32 v2)
> > > >  	return min(v1, v2);
> > > >  }
> > > >  
> > > > +static void update_notify(struct work_struct* work){
> > > > +	int pm_qos_class = atomic_long_read(&work->data);
> > > > +	
> > > > +	s32 extreme_value = atomic_read(&pm_qos_array[pm_qos_class]->target_value);
> > > > +	atomic_notifier_call_chain(
> > > > +		pm_qos_array[pm_qos_class]->notifiers,
> > > > +			(unsigned long) extreme_value, NULL);
> > > > +}
> > > > +struct work_struct pm_qos_update_notify_work;
> > > >  
> > > >  static void update_target(int pm_qos_class)
> > > >  {
> > > > @@ -160,10 +169,10 @@ static void update_target(int pm_qos_class)
> > > >  	}
> > > >  	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);
> > > > +	if (call_notifier){
> > > > +		atomic_long_set(&pm_qos_update_notify_work.data, pm_qos_class);
> > > > +		schedule_work(&pm_qos_update_notify_work);
> > > 
> > > Actually, you should use execute_in_process_context for this ... that
> > > way any current call from user context won't change semantics (i.e. the
> > > notifier will be executed before the call finishes).
> > 
> > Ah, ok. Didn't know of its existence. 
> > After thinking a bit, I think it is crucial for the qos-infrastructure
> > to provide direct notifiers. For example, any missed change in latency
> > could impact the system.
> > 
> > > 
> > > I also suspect we might need an atomic notifier chain to be called
> > > in-line ... things like the dma latency notifier need in-line
> > > notification, I can see that some android ones would at all.  However,
> > > this question can be deferred until an actual use case is seen.
> > 
> > We should probably only provide an atomic queue and have all users
> > either schedule work or be quick, as it can be crucial to get the
> > information out there.
> > 
> > What do you think?
> 
> Yes, considering the use cases, I don't think we want to muck with
> workqueues at all, so just do two queues per constraint, one blocking
> and one atomic.  For a constraint  that has atomic update sites,
> registering a blocking notifier would be illegal and we'd give a runtime
> warning (we can even code a __might_sleep in there before the call).
> That way we preserve current semantics and permit blocking notifiers to
> be registered, but only if all the call sites have user context.
> 
I like the idea of having only atomic safe notifiers.  Is there an easy
way to enforce that?

--mgross
 
> 

Re: [PATCH] pm_qos: make update_request non blocking

[James Bottomley]

On Sun, 2010-06-06 at 14:50 -0700, mark gross wrote:
> On Sat, Jun 05, 2010 at 05:44:09PM -0500, James Bottomley wrote:
> > On Sat, 2010-06-05 at 23:21 +0200, Florian Mickler wrote:
> > > On Sat, 05 Jun 2010 15:57:10 -0500
> > > James Bottomley <James.Bottomley@suse.de> wrote:
> > > 
> > > > On Sat, 2010-06-05 at 22:46 +0200, florian@mickler.org wrote:
> > > > > First we use atomic_notifier to get the spinlocked variant.
> > > > > Secondly we call the notifiers via schedule_work.
> > > > > 
> > > > > Signed-off-by: Florian Mickler <florian@mickler.org>
> > > > > ---
> > > > > 
> > > > > Please advise.
> > > > > 
> > > > >  kernel/pm_qos_params.c |   33 ++++++++++++++++++++++-----------
> > > > >  1 files changed, 22 insertions(+), 11 deletions(-)
> > > > > 
> > > > > diff --git a/kernel/pm_qos_params.c b/kernel/pm_qos_params.c
> > > > > index f42d3f7..7335c27 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>
> > > > >  
> > > > > @@ -63,7 +64,7 @@ static s32 min_compare(s32 v1, s32 v2);
> > > > >  
> > > > >  struct pm_qos_object {
> > > > >  	struct pm_qos_request_list requests;
> > > > > -	struct blocking_notifier_head *notifiers;
> > > > > +	struct atomic_notifier_head *notifiers;
> > > > >  	struct miscdevice pm_qos_power_miscdev;
> > > > >  	char *name;
> > > > >  	s32 default_value;
> > > > > @@ -72,7 +73,7 @@ struct pm_qos_object {
> > > > >  };
> > > > >  
> > > > >  static struct pm_qos_object null_pm_qos;
> > > > > -static BLOCKING_NOTIFIER_HEAD(cpu_dma_lat_notifier);
> > > > > +static ATOMIC_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,
> > > > > @@ -82,7 +83,7 @@ static struct pm_qos_object cpu_dma_pm_qos = {
> > > > >  	.comparitor = min_compare
> > > > >  };
> > > > >  
> > > > > -static BLOCKING_NOTIFIER_HEAD(network_lat_notifier);
> > > > > +static ATOMIC_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,
> > > > > @@ -93,7 +94,7 @@ static struct pm_qos_object network_lat_pm_qos = {
> > > > >  };
> > > > >  
> > > > > 
> > > > > -static BLOCKING_NOTIFIER_HEAD(network_throughput_notifier);
> > > > > +static ATOMIC_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,
> > > > > @@ -103,7 +104,6 @@ static struct pm_qos_object network_throughput_pm_qos = {
> > > > >  	.comparitor = max_compare
> > > > >  };
> > > > >  
> > > > > -
> > > > >  static struct pm_qos_object *pm_qos_array[] = {
> > > > >  	&null_pm_qos,
> > > > >  	&cpu_dma_pm_qos,
> > > > > @@ -135,6 +135,15 @@ static s32 min_compare(s32 v1, s32 v2)
> > > > >  	return min(v1, v2);
> > > > >  }
> > > > >  
> > > > > +static void update_notify(struct work_struct* work){
> > > > > +	int pm_qos_class = atomic_long_read(&work->data);
> > > > > +	
> > > > > +	s32 extreme_value = atomic_read(&pm_qos_array[pm_qos_class]->target_value);
> > > > > +	atomic_notifier_call_chain(
> > > > > +		pm_qos_array[pm_qos_class]->notifiers,
> > > > > +			(unsigned long) extreme_value, NULL);
> > > > > +}
> > > > > +struct work_struct pm_qos_update_notify_work;
> > > > >  
> > > > >  static void update_target(int pm_qos_class)
> > > > >  {
> > > > > @@ -160,10 +169,10 @@ static void update_target(int pm_qos_class)
> > > > >  	}
> > > > >  	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);
> > > > > +	if (call_notifier){
> > > > > +		atomic_long_set(&pm_qos_update_notify_work.data, pm_qos_class);
> > > > > +		schedule_work(&pm_qos_update_notify_work);
> > > > 
> > > > Actually, you should use execute_in_process_context for this ... that
> > > > way any current call from user context won't change semantics (i.e. the
> > > > notifier will be executed before the call finishes).
> > > 
> > > Ah, ok. Didn't know of its existence. 
> > > After thinking a bit, I think it is crucial for the qos-infrastructure
> > > to provide direct notifiers. For example, any missed change in latency
> > > could impact the system.
> > > 
> > > > 
> > > > I also suspect we might need an atomic notifier chain to be called
> > > > in-line ... things like the dma latency notifier need in-line
> > > > notification, I can see that some android ones would at all.  However,
> > > > this question can be deferred until an actual use case is seen.
> > > 
> > > We should probably only provide an atomic queue and have all users
> > > either schedule work or be quick, as it can be crucial to get the
> > > information out there.
> > > 
> > > What do you think?
> > 
> > Yes, considering the use cases, I don't think we want to muck with
> > workqueues at all, so just do two queues per constraint, one blocking
> > and one atomic.  For a constraint  that has atomic update sites,
> > registering a blocking notifier would be illegal and we'd give a runtime
> > warning (we can even code a __might_sleep in there before the call).
> > That way we preserve current semantics and permit blocking notifiers to
> > be registered, but only if all the call sites have user context.
> > 
> I like the idea of having only atomic safe notifiers.  Is there an easy
> way to enforce that?

There's a runtime way: you put a __might_sleep() into the code if
someone registers a blocking notifier chain.

James