[RFC] pm_qos: reimplement using plists

[RFC] pm_qos: reimplement using plists

[James Bottomley]

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*/
 
 #include <linux/pm_qos_params.h>
+#include <linux/plist.h>
 #include <linux/sched.h>
 #include <linux/spinlock.h>
 #include <linux/slab.h>
@@ -49,58 +50,51 @@
  * held, taken with _irqsave.  One lock to rule them all
  */
 struct pm_qos_request_list {
-	struct list_head list;
-	union {
-		s32 value;
-		s32 usec;
-		s32 kbps;
-	};
+	struct plist_node list;
 	int pm_qos_class;
 };
 
-static s32 max_compare(s32 v1, s32 v2);
-static s32 min_compare(s32 v1, s32 v2);
+enum pm_qos_type {
+	PM_QOS_MAX,		/* return the largest value */
+	PM_QOS_MIN,		/* return the smallest value */
+};
 
 struct pm_qos_object {
-	struct pm_qos_request_list requests;
+	struct plist_head requests;
 	struct blocking_notifier_head *notifiers;
 	struct miscdevice pm_qos_power_miscdev;
 	char *name;
 	s32 default_value;
-	atomic_t target_value;
-	s32 (*comparitor)(s32, s32);
+	enum pm_qos_type type;
 };
 
 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)},
+	.requests = {_PLIST_HEAD_INIT(cpu_dma_pm_qos.requests)},
 	.notifiers = &cpu_dma_lat_notifier,
 	.name = "cpu_dma_latency",
 	.default_value = 2000 * USEC_PER_SEC,
-	.target_value = ATOMIC_INIT(2000 * USEC_PER_SEC),
-	.comparitor = min_compare
+	.type = PM_QOS_MIN,
 };
 
 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)},
+	.requests = {_PLIST_HEAD_INIT(network_lat_pm_qos.requests)},
 	.notifiers = &network_lat_notifier,
 	.name = "network_latency",
 	.default_value = 2000 * USEC_PER_SEC,
-	.target_value = ATOMIC_INIT(2000 * USEC_PER_SEC),
-	.comparitor = min_compare
+	.type = PM_QOS_MIN
 };
 
 
 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)},
+	.requests = {_PLIST_HEAD_INIT(network_throughput_pm_qos.requests)},
 	.notifiers = &network_throughput_notifier,
 	.name = "network_throughput",
 	.default_value = 0,
-	.target_value = ATOMIC_INIT(0),
-	.comparitor = max_compare
+	.type = PM_QOS_MAX,
 };
 
 
@@ -124,46 +118,40 @@ static const struct file_operations pm_qos_power_fops = {
 	.release = pm_qos_power_release,
 };
 
-/* static helper functions */
-static s32 max_compare(s32 v1, s32 v2)
+/* unlocked internal variant */
+static inline int pm_qos_get_value(struct pm_qos_object *o)
 {
-	return max(v1, v2);
-}
+	if (plist_head_empty(&o->requests))
+		return o->default_value;
 
-static s32 min_compare(s32 v1, s32 v2)
-{
-	return min(v1, v2);
-}
+	switch (o->type) {
+	case PM_QOS_MIN:
+		return list_entry(o->requests.node_list.prev, struct plist_node, plist.node_list)->prio;
 
+	case PM_QOS_MAX:
+		return plist_first(&o->requests)->prio;
+	}
+}
 
-static void update_target(int pm_qos_class)
+static void update_target(struct pm_qos_object *o, struct plist_node *node,
+			  int del)
 {
-	s32 extreme_value;
-	struct pm_qos_request_list *node;
 	unsigned long flags;
-	int call_notifier = 0;
+	int prev_value, curr_value;
 
 	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) {
-		call_notifier = 1;
-		atomic_set(&pm_qos_array[pm_qos_class]->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));
-	}
+	prev_value = pm_qos_get_value(o);
+	if (del)
+		plist_del(node, &o->requests);
+	else
+		plist_add(node, &o->requests);
+	curr_value = pm_qos_get_value(o);
 	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 (prev_value != curr_value)
+		blocking_notifier_call_chain(o->notifiers,
+					     (unsigned long)curr_value,
+					     NULL);
 }
 
 static int register_pm_qos_misc(struct pm_qos_object *qos)
@@ -196,7 +184,14 @@ static int find_pm_qos_object_by_minor(int minor)
  */
 int pm_qos_request(int pm_qos_class)
 {
-	return atomic_read(&pm_qos_array[pm_qos_class]->target_value);
+	unsigned long flags;
+	int value;
+
+	spin_lock_irqsave(&pm_qos_lock, flags);
+	value = pm_qos_get_value(pm_qos_array[pm_qos_class]);
+	spin_unlock_irqrestore(&pm_qos_lock, flags);
+
+	return value;
 }
 EXPORT_SYMBOL_GPL(pm_qos_request);
 
@@ -214,21 +209,19 @@ EXPORT_SYMBOL_GPL(pm_qos_request);
 struct pm_qos_request_list *pm_qos_add_request(int pm_qos_class, s32 value)
 {
 	struct pm_qos_request_list *dep;
-	unsigned long flags;
 
 	dep = kzalloc(sizeof(struct pm_qos_request_list), GFP_KERNEL);
 	if (dep) {
+		struct pm_qos_object *o =  pm_qos_array[pm_qos_class];
+		int new_value;
+
 		if (value == PM_QOS_DEFAULT_VALUE)
-			dep->value = pm_qos_array[pm_qos_class]->default_value;
+			new_value = o->default_value;
 		else
-			dep->value = value;
+			new_value = value;
+		plist_node_init(&dep->list, new_value);
 		dep->pm_qos_class = pm_qos_class;
-
-		spin_lock_irqsave(&pm_qos_lock, flags);
-		list_add(&dep->list,
-			&pm_qos_array[pm_qos_class]->requests.list);
-		spin_unlock_irqrestore(&pm_qos_lock, flags);
-		update_target(pm_qos_class);
+		update_target(o, &dep->list, 0);
 	}
 
 	return dep;
@@ -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;
 
-		if (temp != pm_qos_req->value) {
-			pending_update = 1;
-			pm_qos_req->value = temp;
-		}
+	o = pm_qos_array[pm_qos_req->pm_qos_class];
+
+	if (new_value == PM_QOS_DEFAULT_VALUE)
+		temp = o->default_value;
+	else
+		temp = new_value;
+
+	if (temp != pm_qos_req->list.prio) {
+		pending_update = 1;
+		spin_lock_irqsave(&pm_qos_lock, flags);
+		plist_del(&pm_qos_req->list, &o->requests);
+		plist_node_init(&pm_qos_req->list, temp);
 		spin_unlock_irqrestore(&pm_qos_lock, flags);
-		if (pending_update)
-			update_target(pm_qos_req->pm_qos_class);
 	}
+	if (pending_update)
+		update_target(o, &pm_qos_req->list, 0);
 }
 EXPORT_SYMBOL_GPL(pm_qos_update_request);
 
@@ -280,19 +278,15 @@ EXPORT_SYMBOL_GPL(pm_qos_update_request);
  */
 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 */
 
-	qos_class = pm_qos_req->pm_qos_class;
-	spin_lock_irqsave(&pm_qos_lock, flags);
-	list_del(&pm_qos_req->list);
+	o = pm_qos_array[pm_qos_req->pm_qos_class];
+	update_target(o, &pm_qos_req->list, 1);
 	kfree(pm_qos_req);
-	spin_unlock_irqrestore(&pm_qos_lock, flags);
-	update_target(qos_class);
 }
 EXPORT_SYMBOL_GPL(pm_qos_remove_request);
 

Re: [RFC] pm_qos: reimplement using plists

[Rafael J. Wysocki]

On Saturday 05 June 2010, 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.

I like it.

> 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.

I assume you'll send a signed-off version at one point.

Thanks,
Rafael


> ---
> 
>  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*/
>  
>  #include <linux/pm_qos_params.h>
> +#include <linux/plist.h>
>  #include <linux/sched.h>
>  #include <linux/spinlock.h>
>  #include <linux/slab.h>
> @@ -49,58 +50,51 @@
>   * held, taken with _irqsave.  One lock to rule them all
>   */
>  struct pm_qos_request_list {
> -	struct list_head list;
> -	union {
> -		s32 value;
> -		s32 usec;
> -		s32 kbps;
> -	};
> +	struct plist_node list;
>  	int pm_qos_class;
>  };
>  
> -static s32 max_compare(s32 v1, s32 v2);
> -static s32 min_compare(s32 v1, s32 v2);
> +enum pm_qos_type {
> +	PM_QOS_MAX,		/* return the largest value */
> +	PM_QOS_MIN,		/* return the smallest value */
> +};
>  
>  struct pm_qos_object {
> -	struct pm_qos_request_list requests;
> +	struct plist_head requests;
>  	struct blocking_notifier_head *notifiers;
>  	struct miscdevice pm_qos_power_miscdev;
>  	char *name;
>  	s32 default_value;
> -	atomic_t target_value;
> -	s32 (*comparitor)(s32, s32);
> +	enum pm_qos_type type;
>  };
>  
>  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)},
> +	.requests = {_PLIST_HEAD_INIT(cpu_dma_pm_qos.requests)},
>  	.notifiers = &cpu_dma_lat_notifier,
>  	.name = "cpu_dma_latency",
>  	.default_value = 2000 * USEC_PER_SEC,
> -	.target_value = ATOMIC_INIT(2000 * USEC_PER_SEC),
> -	.comparitor = min_compare
> +	.type = PM_QOS_MIN,
>  };
>  
>  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)},
> +	.requests = {_PLIST_HEAD_INIT(network_lat_pm_qos.requests)},
>  	.notifiers = &network_lat_notifier,
>  	.name = "network_latency",
>  	.default_value = 2000 * USEC_PER_SEC,
> -	.target_value = ATOMIC_INIT(2000 * USEC_PER_SEC),
> -	.comparitor = min_compare
> +	.type = PM_QOS_MIN
>  };
>  
>  
>  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)},
> +	.requests = {_PLIST_HEAD_INIT(network_throughput_pm_qos.requests)},
>  	.notifiers = &network_throughput_notifier,
>  	.name = "network_throughput",
>  	.default_value = 0,
> -	.target_value = ATOMIC_INIT(0),
> -	.comparitor = max_compare
> +	.type = PM_QOS_MAX,
>  };
>  
>  
> @@ -124,46 +118,40 @@ static const struct file_operations pm_qos_power_fops = {
>  	.release = pm_qos_power_release,
>  };
>  
> -/* static helper functions */
> -static s32 max_compare(s32 v1, s32 v2)
> +/* unlocked internal variant */
> +static inline int pm_qos_get_value(struct pm_qos_object *o)
>  {
> -	return max(v1, v2);
> -}
> +	if (plist_head_empty(&o->requests))
> +		return o->default_value;
>  
> -static s32 min_compare(s32 v1, s32 v2)
> -{
> -	return min(v1, v2);
> -}
> +	switch (o->type) {
> +	case PM_QOS_MIN:
> +		return list_entry(o->requests.node_list.prev, struct plist_node, plist.node_list)->prio;
>  
> +	case PM_QOS_MAX:
> +		return plist_first(&o->requests)->prio;
> +	}
> +}
>  
> -static void update_target(int pm_qos_class)
> +static void update_target(struct pm_qos_object *o, struct plist_node *node,
> +			  int del)
>  {
> -	s32 extreme_value;
> -	struct pm_qos_request_list *node;
>  	unsigned long flags;
> -	int call_notifier = 0;
> +	int prev_value, curr_value;
>  
>  	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) {
> -		call_notifier = 1;
> -		atomic_set(&pm_qos_array[pm_qos_class]->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));
> -	}
> +	prev_value = pm_qos_get_value(o);
> +	if (del)
> +		plist_del(node, &o->requests);
> +	else
> +		plist_add(node, &o->requests);
> +	curr_value = pm_qos_get_value(o);
>  	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 (prev_value != curr_value)
> +		blocking_notifier_call_chain(o->notifiers,
> +					     (unsigned long)curr_value,
> +					     NULL);
>  }
>  
>  static int register_pm_qos_misc(struct pm_qos_object *qos)
> @@ -196,7 +184,14 @@ static int find_pm_qos_object_by_minor(int minor)
>   */
>  int pm_qos_request(int pm_qos_class)
>  {
> -	return atomic_read(&pm_qos_array[pm_qos_class]->target_value);
> +	unsigned long flags;
> +	int value;
> +
> +	spin_lock_irqsave(&pm_qos_lock, flags);
> +	value = pm_qos_get_value(pm_qos_array[pm_qos_class]);
> +	spin_unlock_irqrestore(&pm_qos_lock, flags);
> +
> +	return value;
>  }
>  EXPORT_SYMBOL_GPL(pm_qos_request);
>  
> @@ -214,21 +209,19 @@ EXPORT_SYMBOL_GPL(pm_qos_request);
>  struct pm_qos_request_list *pm_qos_add_request(int pm_qos_class, s32 value)
>  {
>  	struct pm_qos_request_list *dep;
> -	unsigned long flags;
>  
>  	dep = kzalloc(sizeof(struct pm_qos_request_list), GFP_KERNEL);
>  	if (dep) {
> +		struct pm_qos_object *o =  pm_qos_array[pm_qos_class];
> +		int new_value;
> +
>  		if (value == PM_QOS_DEFAULT_VALUE)
> -			dep->value = pm_qos_array[pm_qos_class]->default_value;
> +			new_value = o->default_value;
>  		else
> -			dep->value = value;
> +			new_value = value;
> +		plist_node_init(&dep->list, new_value);
>  		dep->pm_qos_class = pm_qos_class;
> -
> -		spin_lock_irqsave(&pm_qos_lock, flags);
> -		list_add(&dep->list,
> -			&pm_qos_array[pm_qos_class]->requests.list);
> -		spin_unlock_irqrestore(&pm_qos_lock, flags);
> -		update_target(pm_qos_class);
> +		update_target(o, &dep->list, 0);
>  	}
>  
>  	return dep;
> @@ -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;
>  
> -		if (temp != pm_qos_req->value) {
> -			pending_update = 1;
> -			pm_qos_req->value = temp;
> -		}
> +	o = pm_qos_array[pm_qos_req->pm_qos_class];
> +
> +	if (new_value == PM_QOS_DEFAULT_VALUE)
> +		temp = o->default_value;
> +	else
> +		temp = new_value;
> +
> +	if (temp != pm_qos_req->list.prio) {
> +		pending_update = 1;
> +		spin_lock_irqsave(&pm_qos_lock, flags);
> +		plist_del(&pm_qos_req->list, &o->requests);
> +		plist_node_init(&pm_qos_req->list, temp);
>  		spin_unlock_irqrestore(&pm_qos_lock, flags);
> -		if (pending_update)
> -			update_target(pm_qos_req->pm_qos_class);
>  	}
> +	if (pending_update)
> +		update_target(o, &pm_qos_req->list, 0);
>  }
>  EXPORT_SYMBOL_GPL(pm_qos_update_request);
>  
> @@ -280,19 +278,15 @@ EXPORT_SYMBOL_GPL(pm_qos_update_request);
>   */
>  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 */
>  
> -	qos_class = pm_qos_req->pm_qos_class;
> -	spin_lock_irqsave(&pm_qos_lock, flags);
> -	list_del(&pm_qos_req->list);
> +	o = pm_qos_array[pm_qos_req->pm_qos_class];
> +	update_target(o, &pm_qos_req->list, 1);
>  	kfree(pm_qos_req);
> -	spin_unlock_irqrestore(&pm_qos_lock, flags);
> -	update_target(qos_class);
>  }
>  EXPORT_SYMBOL_GPL(pm_qos_remove_request);
>  
> 
> 
> _______________________________________________
> linux-pm mailing list
> linux-pm@lists.linux-foundation.org
> https://lists.linux-foundation.org/mailman/listinfo/linux-pm
> 
> 

Re: [RFC] pm_qos: reimplement using plists

[James Bottomley]

On Sat, 2010-06-05 at 20:17 +0200, Rafael J. Wysocki wrote:
> On Saturday 05 June 2010, 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.
> 
> I like it.
> 
> > 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.
> 
> I assume you'll send a signed-off version at one point.

Yes ... probably as a two parter with the plist changes.

James

Re: [RFC] pm_qos: reimplement using plists

[mark gross]

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*/
>  
>  #include <linux/pm_qos_params.h>
> +#include <linux/plist.h>
>  #include <linux/sched.h>
>  #include <linux/spinlock.h>
>  #include <linux/slab.h>
> @@ -49,58 +50,51 @@
>   * held, taken with _irqsave.  One lock to rule them all
>   */
>  struct pm_qos_request_list {
> -	struct list_head list;
> -	union {
> -		s32 value;
> -		s32 usec;
> -		s32 kbps;
> -	};
> +	struct plist_node list;
>  	int pm_qos_class;
>  };

what is the scaling charactoristics of plist's for things that have a
large dynamic range and a significant utilization of it?  (say 16 bits
of it.  Which is very unlikely to ever happen)

I think in practice this will not be an issue as most users only ever
call the pm_qos API with 2 values (at least kernel users of the API) 

I've only had a quick look so far but it looks really good to me.

--mgross

>  
> -static s32 max_compare(s32 v1, s32 v2);
> -static s32 min_compare(s32 v1, s32 v2);
> +enum pm_qos_type {
> +	PM_QOS_MAX,		/* return the largest value */
> +	PM_QOS_MIN,		/* return the smallest value */
> +};
>  
>  struct pm_qos_object {
> -	struct pm_qos_request_list requests;
> +	struct plist_head requests;
>  	struct blocking_notifier_head *notifiers;
>  	struct miscdevice pm_qos_power_miscdev;
>  	char *name;
>  	s32 default_value;
> -	atomic_t target_value;
> -	s32 (*comparitor)(s32, s32);
> +	enum pm_qos_type type;
>  };
>  
>  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)},
> +	.requests = {_PLIST_HEAD_INIT(cpu_dma_pm_qos.requests)},
>  	.notifiers = &cpu_dma_lat_notifier,
>  	.name = "cpu_dma_latency",
>  	.default_value = 2000 * USEC_PER_SEC,
> -	.target_value = ATOMIC_INIT(2000 * USEC_PER_SEC),
> -	.comparitor = min_compare
> +	.type = PM_QOS_MIN,
>  };
>  
>  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)},
> +	.requests = {_PLIST_HEAD_INIT(network_lat_pm_qos.requests)},
>  	.notifiers = &network_lat_notifier,
>  	.name = "network_latency",
>  	.default_value = 2000 * USEC_PER_SEC,
> -	.target_value = ATOMIC_INIT(2000 * USEC_PER_SEC),
> -	.comparitor = min_compare
> +	.type = PM_QOS_MIN
>  };
>  
>  
>  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)},
> +	.requests = {_PLIST_HEAD_INIT(network_throughput_pm_qos.requests)},
>  	.notifiers = &network_throughput_notifier,
>  	.name = "network_throughput",
>  	.default_value = 0,
> -	.target_value = ATOMIC_INIT(0),
> -	.comparitor = max_compare
> +	.type = PM_QOS_MAX,
>  };
>  
>  
> @@ -124,46 +118,40 @@ static const struct file_operations pm_qos_power_fops = {
>  	.release = pm_qos_power_release,
>  };
>  
> -/* static helper functions */
> -static s32 max_compare(s32 v1, s32 v2)
> +/* unlocked internal variant */
> +static inline int pm_qos_get_value(struct pm_qos_object *o)
>  {
> -	return max(v1, v2);
> -}
> +	if (plist_head_empty(&o->requests))
> +		return o->default_value;
>  
> -static s32 min_compare(s32 v1, s32 v2)
> -{
> -	return min(v1, v2);
> -}
> +	switch (o->type) {
> +	case PM_QOS_MIN:
> +		return list_entry(o->requests.node_list.prev, struct plist_node, plist.node_list)->prio;
>  
> +	case PM_QOS_MAX:
> +		return plist_first(&o->requests)->prio;
> +	}
> +}
>  
> -static void update_target(int pm_qos_class)
> +static void update_target(struct pm_qos_object *o, struct plist_node *node,
> +			  int del)
>  {
> -	s32 extreme_value;
> -	struct pm_qos_request_list *node;
>  	unsigned long flags;
> -	int call_notifier = 0;
> +	int prev_value, curr_value;
>  
>  	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) {
> -		call_notifier = 1;
> -		atomic_set(&pm_qos_array[pm_qos_class]->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));
> -	}
> +	prev_value = pm_qos_get_value(o);
> +	if (del)
> +		plist_del(node, &o->requests);
> +	else
> +		plist_add(node, &o->requests);
> +	curr_value = pm_qos_get_value(o);
>  	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 (prev_value != curr_value)
> +		blocking_notifier_call_chain(o->notifiers,
> +					     (unsigned long)curr_value,
> +					     NULL);
>  }
>  
>  static int register_pm_qos_misc(struct pm_qos_object *qos)
> @@ -196,7 +184,14 @@ static int find_pm_qos_object_by_minor(int minor)
>   */
>  int pm_qos_request(int pm_qos_class)
>  {
> -	return atomic_read(&pm_qos_array[pm_qos_class]->target_value);
> +	unsigned long flags;
> +	int value;
> +
> +	spin_lock_irqsave(&pm_qos_lock, flags);
> +	value = pm_qos_get_value(pm_qos_array[pm_qos_class]);
> +	spin_unlock_irqrestore(&pm_qos_lock, flags);
> +
> +	return value;
>  }
>  EXPORT_SYMBOL_GPL(pm_qos_request);
>  
> @@ -214,21 +209,19 @@ EXPORT_SYMBOL_GPL(pm_qos_request);
>  struct pm_qos_request_list *pm_qos_add_request(int pm_qos_class, s32 value)
>  {
>  	struct pm_qos_request_list *dep;
> -	unsigned long flags;
>  
>  	dep = kzalloc(sizeof(struct pm_qos_request_list), GFP_KERNEL);
>  	if (dep) {
> +		struct pm_qos_object *o =  pm_qos_array[pm_qos_class];
> +		int new_value;
> +
>  		if (value == PM_QOS_DEFAULT_VALUE)
> -			dep->value = pm_qos_array[pm_qos_class]->default_value;
> +			new_value = o->default_value;
>  		else
> -			dep->value = value;
> +			new_value = value;
> +		plist_node_init(&dep->list, new_value);
>  		dep->pm_qos_class = pm_qos_class;
> -
> -		spin_lock_irqsave(&pm_qos_lock, flags);
> -		list_add(&dep->list,
> -			&pm_qos_array[pm_qos_class]->requests.list);
> -		spin_unlock_irqrestore(&pm_qos_lock, flags);
> -		update_target(pm_qos_class);
> +		update_target(o, &dep->list, 0);
>  	}
>  
>  	return dep;
> @@ -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;
>  
> -		if (temp != pm_qos_req->value) {
> -			pending_update = 1;
> -			pm_qos_req->value = temp;
> -		}
> +	o = pm_qos_array[pm_qos_req->pm_qos_class];
> +
> +	if (new_value == PM_QOS_DEFAULT_VALUE)
> +		temp = o->default_value;
> +	else
> +		temp = new_value;
> +
> +	if (temp != pm_qos_req->list.prio) {
> +		pending_update = 1;
> +		spin_lock_irqsave(&pm_qos_lock, flags);
> +		plist_del(&pm_qos_req->list, &o->requests);
> +		plist_node_init(&pm_qos_req->list, temp);
>  		spin_unlock_irqrestore(&pm_qos_lock, flags);
> -		if (pending_update)
> -			update_target(pm_qos_req->pm_qos_class);
>  	}
> +	if (pending_update)
> +		update_target(o, &pm_qos_req->list, 0);
>  }
>  EXPORT_SYMBOL_GPL(pm_qos_update_request);
>  
> @@ -280,19 +278,15 @@ EXPORT_SYMBOL_GPL(pm_qos_update_request);
>   */
>  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 */
>  
> -	qos_class = pm_qos_req->pm_qos_class;
> -	spin_lock_irqsave(&pm_qos_lock, flags);
> -	list_del(&pm_qos_req->list);
> +	o = pm_qos_array[pm_qos_req->pm_qos_class];
> +	update_target(o, &pm_qos_req->list, 1);
>  	kfree(pm_qos_req);
> -	spin_unlock_irqrestore(&pm_qos_lock, flags);
> -	update_target(qos_class);
>  }
>  EXPORT_SYMBOL_GPL(pm_qos_remove_request);
>  
> 
> 

Re: [RFC] pm_qos: reimplement using plists

[mark gross]

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*/
>  
>  #include <linux/pm_qos_params.h>
> +#include <linux/plist.h>
>  #include <linux/sched.h>
>  #include <linux/spinlock.h>
>  #include <linux/slab.h>
> @@ -49,58 +50,51 @@
>   * held, taken with _irqsave.  One lock to rule them all
>   */
>  struct pm_qos_request_list {
> -	struct list_head list;
> -	union {
> -		s32 value;
> -		s32 usec;
> -		s32 kbps;
> -	};
> +	struct plist_node list;
>  	int pm_qos_class;
>  };
>  
> -static s32 max_compare(s32 v1, s32 v2);
> -static s32 min_compare(s32 v1, s32 v2);
> +enum pm_qos_type {
> +	PM_QOS_MAX,		/* return the largest value */
> +	PM_QOS_MIN,		/* return the smallest value */
> +};
>  
>  struct pm_qos_object {
> -	struct pm_qos_request_list requests;
> +	struct plist_head requests;
>  	struct blocking_notifier_head *notifiers;
>  	struct miscdevice pm_qos_power_miscdev;
>  	char *name;
>  	s32 default_value;
> -	atomic_t target_value;
> -	s32 (*comparitor)(s32, s32);
> +	enum pm_qos_type type;
>  };
>  
>  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)},
> +	.requests = {_PLIST_HEAD_INIT(cpu_dma_pm_qos.requests)},
>  	.notifiers = &cpu_dma_lat_notifier,
>  	.name = "cpu_dma_latency",
>  	.default_value = 2000 * USEC_PER_SEC,
> -	.target_value = ATOMIC_INIT(2000 * USEC_PER_SEC),
> -	.comparitor = min_compare
> +	.type = PM_QOS_MIN,
>  };
>  
>  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)},
> +	.requests = {_PLIST_HEAD_INIT(network_lat_pm_qos.requests)},
>  	.notifiers = &network_lat_notifier,
>  	.name = "network_latency",
>  	.default_value = 2000 * USEC_PER_SEC,
> -	.target_value = ATOMIC_INIT(2000 * USEC_PER_SEC),
> -	.comparitor = min_compare
> +	.type = PM_QOS_MIN
>  };
>  
>  
>  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)},
> +	.requests = {_PLIST_HEAD_INIT(network_throughput_pm_qos.requests)},
>  	.notifiers = &network_throughput_notifier,
>  	.name = "network_throughput",
>  	.default_value = 0,
> -	.target_value = ATOMIC_INIT(0),
> -	.comparitor = max_compare
> +	.type = PM_QOS_MAX,
>  };
>  
>  
> @@ -124,46 +118,40 @@ static const struct file_operations pm_qos_power_fops = {
>  	.release = pm_qos_power_release,
>  };
>  
> -/* static helper functions */
> -static s32 max_compare(s32 v1, s32 v2)
> +/* unlocked internal variant */
> +static inline int pm_qos_get_value(struct pm_qos_object *o)
>  {
> -	return max(v1, v2);
> -}
> +	if (plist_head_empty(&o->requests))
> +		return o->default_value;
>  
> -static s32 min_compare(s32 v1, s32 v2)
> -{
> -	return min(v1, v2);
> -}
> +	switch (o->type) {
> +	case PM_QOS_MIN:
> +		return list_entry(o->requests.node_list.prev, struct plist_node, plist.node_list)->prio;
>  
> +	case PM_QOS_MAX:
> +		return plist_first(&o->requests)->prio;
> +	}
> +}
>  
> -static void update_target(int pm_qos_class)
> +static void update_target(struct pm_qos_object *o, struct plist_node *node,
> +			  int del)
>  {
> -	s32 extreme_value;
> -	struct pm_qos_request_list *node;
>  	unsigned long flags;
> -	int call_notifier = 0;
> +	int prev_value, curr_value;
>  
>  	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) {
> -		call_notifier = 1;
> -		atomic_set(&pm_qos_array[pm_qos_class]->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));
> -	}
> +	prev_value = pm_qos_get_value(o);
> +	if (del)
> +		plist_del(node, &o->requests);
> +	else
> +		plist_add(node, &o->requests);
> +	curr_value = pm_qos_get_value(o);
>  	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 (prev_value != curr_value)
> +		blocking_notifier_call_chain(o->notifiers,
> +					     (unsigned long)curr_value,
> +					     NULL);
>  }
>  
>  static int register_pm_qos_misc(struct pm_qos_object *qos)
> @@ -196,7 +184,14 @@ static int find_pm_qos_object_by_minor(int minor)
>   */
>  int pm_qos_request(int pm_qos_class)
>  {
> -	return atomic_read(&pm_qos_array[pm_qos_class]->target_value);
> +	unsigned long flags;
> +	int value;
> +
> +	spin_lock_irqsave(&pm_qos_lock, flags);
> +	value = pm_qos_get_value(pm_qos_array[pm_qos_class]);
> +	spin_unlock_irqrestore(&pm_qos_lock, flags);
> +
> +	return value;
>  }
>  EXPORT_SYMBOL_GPL(pm_qos_request);
>  
> @@ -214,21 +209,19 @@ EXPORT_SYMBOL_GPL(pm_qos_request);
>  struct pm_qos_request_list *pm_qos_add_request(int pm_qos_class, s32 value)
>  {
>  	struct pm_qos_request_list *dep;
> -	unsigned long flags;
>  
>  	dep = kzalloc(sizeof(struct pm_qos_request_list), GFP_KERNEL);
>  	if (dep) {
> +		struct pm_qos_object *o =  pm_qos_array[pm_qos_class];
> +		int new_value;
> +
>  		if (value == PM_QOS_DEFAULT_VALUE)
> -			dep->value = pm_qos_array[pm_qos_class]->default_value;
> +			new_value = o->default_value;
>  		else
> -			dep->value = value;
> +			new_value = value;
> +		plist_node_init(&dep->list, new_value);
>  		dep->pm_qos_class = pm_qos_class;
> -
> -		spin_lock_irqsave(&pm_qos_lock, flags);
> -		list_add(&dep->list,
> -			&pm_qos_array[pm_qos_class]->requests.list);
> -		spin_unlock_irqrestore(&pm_qos_lock, flags);
> -		update_target(pm_qos_class);
> +		update_target(o, &dep->list, 0);
>  	}
>  
>  	return dep;
> @@ -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.

>  
> -		if (temp != pm_qos_req->value) {
> -			pending_update = 1;
> -			pm_qos_req->value = temp;
> -		}
> +	o = pm_qos_array[pm_qos_req->pm_qos_class];
> +
> +	if (new_value == PM_QOS_DEFAULT_VALUE)
> +		temp = o->default_value;
> +	else
> +		temp = new_value;
> +
> +	if (temp != pm_qos_req->list.prio) {
> +		pending_update = 1;
> +		spin_lock_irqsave(&pm_qos_lock, flags);
> +		plist_del(&pm_qos_req->list, &o->requests);
> +		plist_node_init(&pm_qos_req->list, temp);
>  		spin_unlock_irqrestore(&pm_qos_lock, flags);
> -		if (pending_update)
> -			update_target(pm_qos_req->pm_qos_class);
>  	}
> +	if (pending_update)
> +		update_target(o, &pm_qos_req->list, 0);
>  }
>  EXPORT_SYMBOL_GPL(pm_qos_update_request);
>  
> @@ -280,19 +278,15 @@ EXPORT_SYMBOL_GPL(pm_qos_update_request);
>   */
>  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.

>  
> -	qos_class = pm_qos_req->pm_qos_class;
> -	spin_lock_irqsave(&pm_qos_lock, flags);
> -	list_del(&pm_qos_req->list);
> +	o = pm_qos_array[pm_qos_req->pm_qos_class];
> +	update_target(o, &pm_qos_req->list, 1);
>  	kfree(pm_qos_req);
> -	spin_unlock_irqrestore(&pm_qos_lock, flags);
> -	update_target(qos_class);
>  }
>  EXPORT_SYMBOL_GPL(pm_qos_remove_request);
>

I like it.

--mgross
  

Re: [RFC] pm_qos: reimplement using plists

[James Bottomley]

On Sun, 2010-06-06 at 14:05 -0700, mark gross wrote:
> what is the scaling charactoristics of plist's for things that have a
> large dynamic range and a significant utilization of it?  (say 16 bits
> of it.  Which is very unlikely to ever happen)

well O(1) as the document says ... actually it's O(n) where n is the
number of different values in the list.  However, it's always better
than the manual search of a single list that pm_qos currently does. 

> I think in practice this will not be an issue as most users only ever
> call the pm_qos API with 2 values (at least kernel users of the API) 

Well, it's better in every case, not just the low range cases.

> I've only had a quick look so far but it looks really good to me.

Thanks.

James

Re: [RFC] pm_qos: reimplement using plists

[James Bottomley]

On Sun, 2010-06-06 at 14:39 -0700, mark gross wrote:
> > @@ -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.

This is a guard against callers passing in NULL ... which is probably
unnecessary ... I think oopsing on a NULL deref would be just fine for
that, since it would represent a programming error.

> >  
> > -		if (temp != pm_qos_req->value) {
> > -			pending_update = 1;
> > -			pm_qos_req->value = temp;
> > -		}
> > +	o = pm_qos_array[pm_qos_req->pm_qos_class];
> > +
> > +	if (new_value == PM_QOS_DEFAULT_VALUE)
> > +		temp = o->default_value;
> > +	else
> > +		temp = new_value;
> > +
> > +	if (temp != pm_qos_req->list.prio) {
> > +		pending_update = 1;
> > +		spin_lock_irqsave(&pm_qos_lock, flags);
> > +		plist_del(&pm_qos_req->list, &o->requests);
> > +		plist_node_init(&pm_qos_req->list, temp);
> >  		spin_unlock_irqrestore(&pm_qos_lock, flags);
> > -		if (pending_update)
> > -			update_target(pm_qos_req->pm_qos_class);
> >  	}
> > +	if (pending_update)
> > +		update_target(o, &pm_qos_req->list, 0);
> >  }
> >  EXPORT_SYMBOL_GPL(pm_qos_update_request);
> >  
> > @@ -280,19 +278,15 @@ EXPORT_SYMBOL_GPL(pm_qos_update_request);
> >   */
> >  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.

It won't: plist_del is idempotent, like list_del.  Of course, such a
programming error represents a bug and probably *should* be reported.

James

Re: [RFC] pm_qos: reimplement using plists

[mark gross]

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;
+	}
 
 	o = pm_qos_array[pm_qos_req->pm_qos_class];
 	update_target(o, &pm_qos_req->list, 1);
-- 
1.6.3.3



 

Re: [RFC] pm_qos: reimplement using plists

[James Bottomley]

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

Re: [RFC] pm_qos: reimplement using plists

[mark gross]

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

Re: [RFC] pm_qos: reimplement using plists

[Florian Mickler]

On Sun, 06 Jun 2010 23:05:06 -0400
James Bottomley <James.Bottomley@suse.de> wrote:

> On Sun, 2010-06-06 at 14:39 -0700, mark gross wrote:
> > > @@ -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.
> 
> This is a guard against callers passing in NULL ... which is probably
> unnecessary ... I think oopsing on a NULL deref would be just fine for
> that, since it would represent a programming error.

That is bad in the general case because you then depend on
mmap_min_addr to be sufficiently large.

I would advocate for not making the attack surface that big.

Cheers,
Flo

Re: [RFC] pm_qos: reimplement using plists

[James Bottomley]

On Wed, 2010-06-09 at 08:38 +0200, Florian Mickler wrote:
> On Sun, 06 Jun 2010 23:05:06 -0400
> James Bottomley <James.Bottomley@suse.de> wrote:
> 
> > On Sun, 2010-06-06 at 14:39 -0700, mark gross wrote:
> > > > @@ -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.
> > 
> > This is a guard against callers passing in NULL ... which is probably
> > unnecessary ... I think oopsing on a NULL deref would be just fine for
> > that, since it would represent a programming error.
> 
> That is bad in the general case because you then depend on
> mmap_min_addr to be sufficiently large.
> 
> I would advocate for not making the attack surface that big.

I think you're thinking this is the userspace API?  It isn't, it's the
kernel, so we want to catch abusers hard usually with either warn on or
bug on ... ideally at build time, but that's not entirely possible in
this case.

James

Re: [RFC] pm_qos: reimplement using plists

[James Bottomley]

On Tue, 2010-06-08 at 19:42 -0700, mark gross wrote:
> 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.  

OK ... Since I've effectively audited all callers not to pass in null, I
think we can now BUG on that, but the update after non reigstration can
be a warn and be nice.

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

I'd have to revisit some of the conversions then.  I didn't do a check
before unregister in one of them because we did it ...

> 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?

I think clarity on the API is always good.

James