Re: [PATCH] pm_qos: make update_request non blocking

[James Bottomley <James.Bottomley@suse.de>]

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