Re: [PATCH] pm_qos: make update_request non blocking

[Florian Mickler <florian@mickler.org>]

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!