Re: [RFC] pm_qos: reimplement using plists

[mark gross <640e9920@gmail.com>]

On Tue, Jun 08, 2010 at 11:52:16AM -0400, James Bottomley wrote:
> On Tue, 2010-06-08 at 06:31 -0700, mark gross wrote:
> > On Sun, Jun 06, 2010 at 02:39:54PM -0700, mark gross wrote:
> > > On Sat, Jun 05, 2010 at 12:58:08PM -0500, James Bottomley wrote:
> > > > A lot of the pm_qos extremal value handling is really duplicating what a
> > > > priority ordered list does, just in a less efficient fashion.  Simply
> > > > redoing the implementation in terms of a plist gets rid of a lot of this
> > > > junk (although there are several other strange things that could do with
> > > > tidying up, like pm_qos_request_list has to carry the pm_qos_class with
> > > > every node, simply because it doesn't get passed in to
> > > > pm_qos_update_request even though every caller knows full well what
> > > > parameter it's updating).
> > > > 
> > > > I think this redo is a win independent of android, so we should do
> > > > something like this now.
> > > > 
> > > > There is one nasty that should probably be fixed in plists not open
> > > > coded here: plist_first gives the highest priority value, but there's no
> > > > corresponding API to give the lowest (even though you can get it from
> > > > the head.nodes_list.prev) ... if the sched people are OK, I'll correct
> > > > this with the final patch set.
> > > > 
> > > > James
> > > > 
> > > > ---
> > > > 
> > > >  kernel/pm_qos_params.c |  152 +++++++++++++++++++++++-------------------------
> > > >  1 files changed, 73 insertions(+), 79 deletions(-)
> > > > 
> > > > diff --git a/kernel/pm_qos_params.c b/kernel/pm_qos_params.c
> > > > index f42d3f7..241fa79 100644
> > > > --- a/kernel/pm_qos_params.c
> > > > +++ b/kernel/pm_qos_params.c
> > > > @@ -30,6 +30,7 @@
> > > >  /*#define DEBUG*/
> > 
> > snip
> > 
> > > > @@ -251,22 +244,27 @@ void pm_qos_update_request(struct pm_qos_request_list *pm_qos_req,
> > > >  	unsigned long flags;
> > > >  	int pending_update = 0;
> > > >  	s32 temp;
> > > > +	struct pm_qos_object *o;
> > > >  
> > > > -	if (pm_qos_req) { /*guard against callers passing in null */
> > > > -		spin_lock_irqsave(&pm_qos_lock, flags);
> > > > -		if (new_value == PM_QOS_DEFAULT_VALUE)
> > > > -			temp = pm_qos_array[pm_qos_req->pm_qos_class]->default_value;
> > > > -		else
> > > > -			temp = new_value;
> > > > +	if (!pm_qos_req) /*guard against callers passing in null */
> > > > +		return;
> > > 
> > > need a better test to see if the pm_qos_req is in the plist or not as we
> > > move to a caller allocated design.
> > >
> > 
> > snip  
> > > >  void pm_qos_remove_request(struct pm_qos_request_list *pm_qos_req)
> > > >  {
> > > > -	unsigned long flags;
> > > > -	int qos_class;
> > > > +	struct pm_qos_object *o;
> > > >  
> > > >  	if (pm_qos_req == NULL)
> > > >  		return;
> > > >  		/* silent return to keep pcm code cleaner */
> > > 
> > > need a way to tell if the request is in the list or not so we don't
> > > crater removing a plist node that isn't in the list.
> > > 
> > snip
> > >
> > 
> > I found that e1000e will panic on rmmod because of it attempting to
> > removing of a pm_qos request that it never added.
> > 
> > This is an ugly patch, but I think its needed for a while to clean out
> > the abusers, then it can be updated to not be so noisy.
> > 
> > 
> > --mgross
> > 
> > --Signed-off-by: mark gross <markgross@thegnar.org>
> > 
> > 
> > 
> > >From fb713f95b83ea3744c31917cfd019bf3e32349b3 Mon Sep 17 00:00:00 2001
> > From: markgross <markgross@thegnar.org>
> > Date: Tue, 8 Jun 2010 06:22:01 -0700
> > Subject: [PATCH] check and complain about abuse of the api to avoid panics
> > 
> > ---
> >  kernel/pm_qos_params.c |   12 +++++++++++-
> >  1 files changed, 11 insertions(+), 1 deletions(-)
> > 
> > diff --git a/kernel/pm_qos_params.c b/kernel/pm_qos_params.c
> > index f1d3d23..4bded27 100644
> > --- a/kernel/pm_qos_params.c
> > +++ b/kernel/pm_qos_params.c
> > @@ -212,7 +212,7 @@ void pm_qos_add_request(struct pm_qos_request_list *dep,
> >  	int new_value;
> >  
> >  	if (pm_qos_request_active(dep))
> > -		return;
> > +		return; /* already in the list */
> >  
> >  	if (value == PM_QOS_DEFAULT_VALUE)
> >  		new_value = o->default_value;
> > @@ -244,6 +244,11 @@ void pm_qos_update_request(struct pm_qos_request_list *pm_qos_req,
> >  
> >  	if (!pm_qos_req) /*guard against callers passing in null */
> >  		return;
> > +	if (!pm_qos_request_active(pm_qos_req)) {
> > +		WARN(true, "pm_qos: update to an unregistered request");
> > +		dump_stack();
> > +		return;
> > +	}
> >  
> >  	o = pm_qos_array[pm_qos_req->pm_qos_class];
> >  
> > @@ -279,6 +284,11 @@ void pm_qos_remove_request(struct pm_qos_request_list *pm_qos_req)
> >  	if (pm_qos_req == NULL)
> >  		return;
> >  		/* silent return to keep pcm code cleaner */
> > +	if (!pm_qos_request_active(pm_qos_req)) {
> > +		WARN(true, "pm_qos: removal an unregistered request");
> > +		dump_stack();
> > +		return;
> > +	}
> 
> Yes, that would more or less reflect current functionality.  If it's the
> intention of the API to silently ignore update and removal of
> unregistered requests, then it should probably be done silently,
> though ... otherwise we'll start to make noise where previously there
> was none.
> 
> James
The intention of the API was not to silently ignore bogus updates and
removals, even though the initial implementation implemented that :(

I recommend we start making the noise and fix the API to return failure
when updating an un-registered qos request.  

Silently failing on removal may be still needed, but a Warn-once would
be in order.

I had planned to start making that API change before this plist stuff,
but I think for now adding the noise and getting the plist stuff good,
then tackling the slight API change is the order I would like to see
things happen.

what do you think?

--mgross