adding handles to pm_qos?

adding handles to pm_qos?

[Ai Li]

We are calling pm_qos from some of our drivers.  One concern is that
each call of pm_qos_update_requirement() iterates through the client
list and strcmp the client names.  It could be slow.  A proposal is
that pm_qos provides handles that can be used on
pm_qos_update_requirement().

For measurement purposes, I added get/put interfaces to
acquire/release the handles and a new pm_qos_update_requirement
function that bypasses the iteration and strcmp.  Here are some
collected data:

How many clock cycles does pm_qos_update_requirement take?
when there is one client on this qos_class:
    using handle        using name      using handle/using name
avg 252.4               400.4           63%

when there are 5 clients on this qos_class:
    using handle        using name      using handle/using name
avg 407.6               644.8           63%

when there are 10 clients on this qos_class:
    using handle        using name      using handle/using name
avg 582.4               938.4           62%

Given the time differences, it seems worthwhile to add handles.

~Ai

Re: adding handles to pm_qos?

[mgross]

On Wed, Oct 14, 2009 at 11:24:19AM -0600, Ai Li wrote:
> We are calling pm_qos from some of our drivers.  One concern is that
> each call of pm_qos_update_requirement() iterates through the client
> list and strcmp the client names.  It could be slow.  A proposal is
> that pm_qos provides handles that can be used on
> pm_qos_update_requirement().
> 
> For measurement purposes, I added get/put interfaces to
> acquire/release the handles and a new pm_qos_update_requirement
> function that bypasses the iteration and strcmp.  Here are some
> collected data:
> 
> How many clock cycles does pm_qos_update_requirement take?
> when there is one client on this qos_class:
>     using handle        using name      using handle/using name
> avg 252.4               400.4           63%
> 
> when there are 5 clients on this qos_class:
>     using handle        using name      using handle/using name
> avg 407.6               644.8           63%
> 
> when there are 10 clients on this qos_class:
>     using handle        using name      using handle/using name
> avg 582.4               938.4           62%
> 
> Given the time differences, it seems worthwhile to add handles.

How often are you calling pm_qos_update_requirement?  

I think calling pm_qos_ interfaces too often makes me wonder about my
assumptions and your sanity.

Can you explain why the pm_qos_update_requirement is getting hit often
enough to bother with this change?

Other than that I don't have a problem with moving to handles, if its a
practical change made for reasons other than making api abuse less
painful.

Further, If the implicit assumption that pmqos calls are on cold paths
is wrong, then perhaps more thought is needed than just changing things
to handle based searches.

--mgross




 
> ~Ai
> 
> _______________________________________________
> linux-pm mailing list
> linux-pm@lists.linux-foundation.org
> https://lists.linux-foundation.org/mailman/listinfo/linux-pm

Re: adding handles to pm_qos?

[Ai Li]

> How often are you calling pm_qos_update_requirement?
> 
> I think calling pm_qos_ interfaces too often makes me wonder
> about my
> assumptions and your sanity.
> 
> Can you explain why the pm_qos_update_requirement is getting hit
> often
> enough to bother with this change?
> 
> Other than that I don't have a problem with moving to handles,
> if its a
> practical change made for reasons other than making api abuse
> less
> painful.
> 
> Further, If the implicit assumption that pmqos calls are on cold
> paths
> is wrong, then perhaps more thought is needed than just changing
> things
> to handle based searches.
> 

Our embedded platforms support different low power modes.  With the
modes, the deeper the sleep, the more the power savings, and the
larger the interrupt latency coming out of the low power mode.

To help the platform achieving greatest power savings, some of our
device drivers set lateny qos only when there is a service request to
the driver or a device transaction.  When the transaction or request
is done, the drivers cancel the QoS with
pm_qos_update_requirement(PM_QOS_DEFAULT_VALUE), allowing the
platform to reach a deeper sleep.  

The approach gives us good power savings.  However when there are
lots of transactions, pm_qos_update_requirement() gets called a lot
of times.

~Ai

Re: adding handles to pm_qos?

[mgross]

On Tue, Oct 27, 2009 at 06:37:58PM -0600, Ai Li wrote:
> > How often are you calling pm_qos_update_requirement?
> > 
> > I think calling pm_qos_ interfaces too often makes me wonder
> > about my
> > assumptions and your sanity.
> > 
> > Can you explain why the pm_qos_update_requirement is getting hit
> > often
> > enough to bother with this change?
> > 
> > Other than that I don't have a problem with moving to handles,
> > if its a
> > practical change made for reasons other than making api abuse
> > less
> > painful.
> > 
> > Further, If the implicit assumption that pmqos calls are on cold
> > paths
> > is wrong, then perhaps more thought is needed than just changing
> > things
> > to handle based searches.
> > 
> 
> Our embedded platforms support different low power modes.  With the
> modes, the deeper the sleep, the more the power savings, and the
> larger the interrupt latency coming out of the low power mode.
> 
> To help the platform achieving greatest power savings, some of our
> device drivers set lateny qos only when there is a service request to
> the driver or a device transaction.  When the transaction or request
> is done, the drivers cancel the QoS with
> pm_qos_update_requirement(PM_QOS_DEFAULT_VALUE), allowing the
> platform to reach a deeper sleep.  
> 
> The approach gives us good power savings.  However when there are
> lots of transactions, pm_qos_update_requirement() gets called a lot
> of times.

Oh. 

This will not scale with the aggregation logic very well at all if
pm_qos update requirement gets hit per transaction through a driver code
path, then I think some thought on the scalability is needed and perhaps
a change to the aggregation design for such uses.

Do you have a patch for the handle implementation I could look at?

--mgross


.  

> 
> ~Ai
> 

Re: adding handles to pm_qos?

[Ai Li]

> Oh.
> 
> This will not scale with the aggregation logic very well at all
> if
> pm_qos update requirement gets hit per transaction through a
> driver code
> path, then I think some thought on the scalability is needed and
> perhaps
> a change to the aggregation design for such uses.
> 
> Do you have a patch for the handle implementation I could look
> at?

Here is the patch that I use with the test code.  There are three new
functions:

void *pm_qos_get(int qos, char *name);
int pm_qos_put(void *handle);
int pm_qos_update_requirement_direct(void *handle, s32 new_value);

In the test, I wanted to keep the existing interface intact so I
could compare them at the same time.  For the formal code submission,
a different approach may work better.


--- include/linux/pm_qos_params.h.orig
+++ include/linux/pm_qos_params.h
@@ -16,7 +16,11 @@
 
 int pm_qos_add_requirement(int qos, char *name, s32 value);
 int pm_qos_update_requirement(int qos, char *name, s32 new_value);
-void pm_qos_remove_requirement(int qos, char *name);
+int pm_qos_remove_requirement(int qos, char *name);
+
+void *pm_qos_get(int qos, char *name);
+int pm_qos_put(void *handle);
+int pm_qos_update_requirement_direct(void *handle, s32 new_value);
 
 int pm_qos_requirement(int qos);
 
--- kernel/pm_qos_params.c.orig
+++ kernel/pm_qos_params.c
@@ -55,6 +55,8 @@ struct requirement_list {
 		s32 kbps;
 	};
 	char *name;
+	int pm_qos_class;
+	int handle_count;
 };
 
 static s32 max_compare(s32 v1, s32 v2);
@@ -210,7 +212,9 @@ EXPORT_SYMBOL_GPL(pm_qos_requirement);
 int pm_qos_add_requirement(int pm_qos_class, char *name, s32 value)
 {
 	struct requirement_list *dep;
+	struct requirement_list *node;
 	unsigned long flags;
+	int retval;
 
 	dep = kzalloc(sizeof(struct requirement_list), GFP_KERNEL);
 	if (dep) {
@@ -219,10 +223,25 @@ int pm_qos_add_requirement(int pm_qos_class,
char *name, s32 value)
 		else
 			dep->value = value;
 		dep->name = kstrdup(name, GFP_KERNEL);
-		if (!dep->name)
+		if (!dep->name) {
+			retval = -ENOMEM;
 			goto cleanup;
+		}
+		dep->pm_qos_class = pm_qos_class;
 
 		spin_lock_irqsave(&pm_qos_lock, flags);
+		/* make sure the new name is unique */
+		list_for_each_entry(node,
+
&pm_qos_array[pm_qos_class]->requirements.list, list) {
+			if (strcmp(node->name, name) == 0) {
+				spin_unlock_irqrestore(&pm_qos_lock,
flags);
+
+				kfree(dep->name);
+				retval = -EINVAL;
+				goto cleanup;
+			}
+		}
+
 		list_add(&dep->list,
 
&pm_qos_array[pm_qos_class]->requirements.list);
 		spin_unlock_irqrestore(&pm_qos_lock, flags);
@@ -231,9 +250,11 @@ int pm_qos_add_requirement(int pm_qos_class,
char *name, s32 value)
 		return 0;
 	}
 
+	retval = -ENOMEM;
+
 cleanup:
 	kfree(dep);
-	return -ENOMEM;
+	return retval;
 }
 EXPORT_SYMBOL_GPL(pm_qos_add_requirement);
 
@@ -282,8 +303,10 @@ EXPORT_SYMBOL_GPL(pm_qos_update_requirement);
  *
  * Will remove named qos request from pm_qos_class list of
parameters and
  * recompute the current target value for the pm_qos_class.
+ *
+ * If there are outstanding handles to the request, -EBUSY is
returned.
  */
-void pm_qos_remove_requirement(int pm_qos_class, char *name)
+int pm_qos_remove_requirement(int pm_qos_class, char *name)
 {
 	unsigned long flags;
 	struct requirement_list *node;
@@ -293,6 +316,11 @@ void pm_qos_remove_requirement(int pm_qos_class,
char *name)
 	list_for_each_entry(node,
 		&pm_qos_array[pm_qos_class]->requirements.list, list)
{
 		if (strcmp(node->name, name) == 0) {
+			if (node->handle_count > 0) {
+				spin_unlock_irqrestore(&pm_qos_lock,
flags);
+				return -EBUSY;
+			}
+
 			kfree(node->name);
 			list_del(&node->list);
 			kfree(node);
@@ -303,9 +331,83 @@ void pm_qos_remove_requirement(int pm_qos_class,
char *name)
 	spin_unlock_irqrestore(&pm_qos_lock, flags);
 	if (pending_update)
 		update_target(pm_qos_class);
+
+	return 0;
 }
 EXPORT_SYMBOL_GPL(pm_qos_remove_requirement);
 
+/* pm_qos_get - returns an opaque handle to the existing qos request
+ * @pm_qos_class: identifies which list of qos request to us
+ * @name: identifies the request
+ *
+ * Will increment the reference count as well.
+ *
+ * If the named request isn't in the list then NULL is returned.
+ */
+void *pm_qos_get(int pm_qos_class, char *name)
+{
+	unsigned long flags;
+	struct requirement_list *node;
+	struct requirement_list *handle = NULL;
+
+	spin_lock_irqsave(&pm_qos_lock, flags);
+	list_for_each_entry(node,
+		&pm_qos_array[pm_qos_class]->requirements.list, list)
{
+		if (strcmp(node->name, name) == 0) {
+			node->handle_count++;
+			handle = node;
+			break;
+		}
+	}
+	spin_unlock_irqrestore(&pm_qos_lock, flags);
+	return handle;
+}
+EXPORT_SYMBOL_GPL(pm_qos_get);
+
+/* pm_qos_put - release the handle of the existing qos request
+ * @handle: previously acquired through pm_qos_get
+ *
+ * Will decrement the reference count as well. Caller should no
+ * longer use the handle after this call.
+ */
+int pm_qos_put(void *handle)
+{
+	unsigned long flags;
+	struct requirement_list *node =	(struct requirement_list *)
handle;
+
+	spin_lock_irqsave(&pm_qos_lock, flags);
+	BUG_ON(node->handle_count == 0);
+	node->handle_count--;
+	spin_unlock_irqrestore(&pm_qos_lock, flags);
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(pm_qos_put);
+
+/* pm_qos_update_requirement_direct - modifies an existing qos
request
+ * @handle: previously acquired through pm_qos_get
+ * @value: defines the qos request
+ *
+ * Updates an existing qos requirement for the pm_qos_class of
parameters along
+ * with updating the target pm_qos_class value.
+ */
+int pm_qos_update_requirement_direct(void *handle, s32 new_value)
+{
+	unsigned long flags;
+	struct requirement_list *node = (struct requirement_list *)
handle;
+
+	spin_lock_irqsave(&pm_qos_lock, flags);
+	if (new_value == PM_QOS_DEFAULT_VALUE)
+		node->value =
pm_qos_array[node->pm_qos_class]->default_value;
+	else
+		node->value = new_value;
+	spin_unlock_irqrestore(&pm_qos_lock, flags);
+	update_target(node->pm_qos_class);
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(pm_qos_update_requirement_direct);
+
 /**
  * pm_qos_add_notifier - sets notification entry for changes to
target value
  * @pm_qos_class: identifies which qos target changes should be
notified.


~Ai

Re: adding handles to pm_qos?

[mgross]

Thanks, I'll look at it over the next few days.

--mgross

On Fri, Oct 30, 2009 at 07:53:42PM -0600, Ai Li wrote:
> > Oh.
> > 
> > This will not scale with the aggregation logic very well at all
> > if
> > pm_qos update requirement gets hit per transaction through a
> > driver code
> > path, then I think some thought on the scalability is needed and
> > perhaps
> > a change to the aggregation design for such uses.
> > 
> > Do you have a patch for the handle implementation I could look
> > at?
> 
> Here is the patch that I use with the test code.  There are three new
> functions:
> 
> void *pm_qos_get(int qos, char *name);
> int pm_qos_put(void *handle);
> int pm_qos_update_requirement_direct(void *handle, s32 new_value);
> 
> In the test, I wanted to keep the existing interface intact so I
> could compare them at the same time.  For the formal code submission,
> a different approach may work better.
> 
> 
> --- include/linux/pm_qos_params.h.orig
> +++ include/linux/pm_qos_params.h
> @@ -16,7 +16,11 @@
>  
>  int pm_qos_add_requirement(int qos, char *name, s32 value);
>  int pm_qos_update_requirement(int qos, char *name, s32 new_value);
> -void pm_qos_remove_requirement(int qos, char *name);
> +int pm_qos_remove_requirement(int qos, char *name);
> +
> +void *pm_qos_get(int qos, char *name);
> +int pm_qos_put(void *handle);
> +int pm_qos_update_requirement_direct(void *handle, s32 new_value);
>  
>  int pm_qos_requirement(int qos);
>  
> --- kernel/pm_qos_params.c.orig
> +++ kernel/pm_qos_params.c
> @@ -55,6 +55,8 @@ struct requirement_list {
>  		s32 kbps;
>  	};
>  	char *name;
> +	int pm_qos_class;
> +	int handle_count;
>  };
>  
>  static s32 max_compare(s32 v1, s32 v2);
> @@ -210,7 +212,9 @@ EXPORT_SYMBOL_GPL(pm_qos_requirement);
>  int pm_qos_add_requirement(int pm_qos_class, char *name, s32 value)
>  {
>  	struct requirement_list *dep;
> +	struct requirement_list *node;
>  	unsigned long flags;
> +	int retval;
>  
>  	dep = kzalloc(sizeof(struct requirement_list), GFP_KERNEL);
>  	if (dep) {
> @@ -219,10 +223,25 @@ int pm_qos_add_requirement(int pm_qos_class,
> char *name, s32 value)
>  		else
>  			dep->value = value;
>  		dep->name = kstrdup(name, GFP_KERNEL);
> -		if (!dep->name)
> +		if (!dep->name) {
> +			retval = -ENOMEM;
>  			goto cleanup;
> +		}
> +		dep->pm_qos_class = pm_qos_class;
>  
>  		spin_lock_irqsave(&pm_qos_lock, flags);
> +		/* make sure the new name is unique */
> +		list_for_each_entry(node,
> +
> &pm_qos_array[pm_qos_class]->requirements.list, list) {
> +			if (strcmp(node->name, name) == 0) {
> +				spin_unlock_irqrestore(&pm_qos_lock,
> flags);
> +
> +				kfree(dep->name);
> +				retval = -EINVAL;
> +				goto cleanup;
> +			}
> +		}
> +
>  		list_add(&dep->list,
>  
> &pm_qos_array[pm_qos_class]->requirements.list);
>  		spin_unlock_irqrestore(&pm_qos_lock, flags);
> @@ -231,9 +250,11 @@ int pm_qos_add_requirement(int pm_qos_class,
> char *name, s32 value)
>  		return 0;
>  	}
>  
> +	retval = -ENOMEM;
> +
>  cleanup:
>  	kfree(dep);
> -	return -ENOMEM;
> +	return retval;
>  }
>  EXPORT_SYMBOL_GPL(pm_qos_add_requirement);
>  
> @@ -282,8 +303,10 @@ EXPORT_SYMBOL_GPL(pm_qos_update_requirement);
>   *
>   * Will remove named qos request from pm_qos_class list of
> parameters and
>   * recompute the current target value for the pm_qos_class.
> + *
> + * If there are outstanding handles to the request, -EBUSY is
> returned.
>   */
> -void pm_qos_remove_requirement(int pm_qos_class, char *name)
> +int pm_qos_remove_requirement(int pm_qos_class, char *name)
>  {
>  	unsigned long flags;
>  	struct requirement_list *node;
> @@ -293,6 +316,11 @@ void pm_qos_remove_requirement(int pm_qos_class,
> char *name)
>  	list_for_each_entry(node,
>  		&pm_qos_array[pm_qos_class]->requirements.list, list)
> {
>  		if (strcmp(node->name, name) == 0) {
> +			if (node->handle_count > 0) {
> +				spin_unlock_irqrestore(&pm_qos_lock,
> flags);
> +				return -EBUSY;
> +			}
> +
>  			kfree(node->name);
>  			list_del(&node->list);
>  			kfree(node);
> @@ -303,9 +331,83 @@ void pm_qos_remove_requirement(int pm_qos_class,
> char *name)
>  	spin_unlock_irqrestore(&pm_qos_lock, flags);
>  	if (pending_update)
>  		update_target(pm_qos_class);
> +
> +	return 0;
>  }
>  EXPORT_SYMBOL_GPL(pm_qos_remove_requirement);
>  
> +/* pm_qos_get - returns an opaque handle to the existing qos request
> + * @pm_qos_class: identifies which list of qos request to us
> + * @name: identifies the request
> + *
> + * Will increment the reference count as well.
> + *
> + * If the named request isn't in the list then NULL is returned.
> + */
> +void *pm_qos_get(int pm_qos_class, char *name)
> +{
> +	unsigned long flags;
> +	struct requirement_list *node;
> +	struct requirement_list *handle = NULL;
> +
> +	spin_lock_irqsave(&pm_qos_lock, flags);
> +	list_for_each_entry(node,
> +		&pm_qos_array[pm_qos_class]->requirements.list, list)
> {
> +		if (strcmp(node->name, name) == 0) {
> +			node->handle_count++;
> +			handle = node;
> +			break;
> +		}
> +	}
> +	spin_unlock_irqrestore(&pm_qos_lock, flags);
> +	return handle;
> +}
> +EXPORT_SYMBOL_GPL(pm_qos_get);
> +
> +/* pm_qos_put - release the handle of the existing qos request
> + * @handle: previously acquired through pm_qos_get
> + *
> + * Will decrement the reference count as well. Caller should no
> + * longer use the handle after this call.
> + */
> +int pm_qos_put(void *handle)
> +{
> +	unsigned long flags;
> +	struct requirement_list *node =	(struct requirement_list *)
> handle;
> +
> +	spin_lock_irqsave(&pm_qos_lock, flags);
> +	BUG_ON(node->handle_count == 0);
> +	node->handle_count--;
> +	spin_unlock_irqrestore(&pm_qos_lock, flags);
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(pm_qos_put);
> +
> +/* pm_qos_update_requirement_direct - modifies an existing qos
> request
> + * @handle: previously acquired through pm_qos_get
> + * @value: defines the qos request
> + *
> + * Updates an existing qos requirement for the pm_qos_class of
> parameters along
> + * with updating the target pm_qos_class value.
> + */
> +int pm_qos_update_requirement_direct(void *handle, s32 new_value)
> +{
> +	unsigned long flags;
> +	struct requirement_list *node = (struct requirement_list *)
> handle;
> +
> +	spin_lock_irqsave(&pm_qos_lock, flags);
> +	if (new_value == PM_QOS_DEFAULT_VALUE)
> +		node->value =
> pm_qos_array[node->pm_qos_class]->default_value;
> +	else
> +		node->value = new_value;
> +	spin_unlock_irqrestore(&pm_qos_lock, flags);
> +	update_target(node->pm_qos_class);
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(pm_qos_update_requirement_direct);
> +
>  /**
>   * pm_qos_add_notifier - sets notification entry for changes to
> target value
>   * @pm_qos_class: identifies which qos target changes should be
> notified.
> 
> 
> ~Ai

Re: adding handles to pm_qos?

[Ai Li]

> Thanks, I'll look at it over the next few days.
> 
> --mgross
> 
> > Here is the patch that I use with the test code.  There are
> three new
> > functions:
> >
> > void *pm_qos_get(int qos, char *name);
> > int pm_qos_put(void *handle);
> > int pm_qos_update_requirement_direct(void *handle, s32
> new_value);
> >
> > In the test, I wanted to keep the existing interface intact so
> I
> > could compare them at the same time.  For the formal code
> submission,
> > a different approach may work better.


Here is an alternate way of adding handles to pm_qos_params that I
was alluding to.  This approach may be more preferable because it
does not bloat the API and the handles become an integral part of
pm_qos_params.  In my previous patch, handles are kind of bolted onto
pm_qos_params and it needs separate calls (pm_qos_get, pm_qos_put) to
acquire and release the handle.  In this patch,
pm_qos_add_requirement and pm_qos_remove_requirement automatically
take care of the handles.


--- include/linux/pm_qos_params.h.orig
+++ include/linux/pm_qos_params.h
@@ -14,9 +14,11 @@
 #define PM_QOS_NUM_CLASSES 4
 #define PM_QOS_DEFAULT_VALUE -1
 
-int pm_qos_add_requirement(int qos, char *name, s32 value);
-int pm_qos_update_requirement(int qos, char *name, s32 new_value);
-void pm_qos_remove_requirement(int qos, char *name);
+struct requirement_list;
+
+struct requirement_list *pm_qos_add_requirement(int qos, char *name,
s32 value);
+int pm_qos_update_requirement(struct requirement_list *qos, s32
new_value);
+void pm_qos_remove_requirement(struct requirement_list *qos);
 
 int pm_qos_requirement(int qos);
 
--- kernel/pm_qos_params.c.orig
+++ kernel/pm_qos_params.c
@@ -49,6 +49,7 @@
  */
 struct requirement_list {
 	struct list_head list;
+	int pm_qos_class;
 	union {
 		s32 value;
 		s32 usec;
@@ -207,13 +208,16 @@ EXPORT_SYMBOL_GPL(pm_qos_requirement);
  * performance characteristics.  It recomputes the aggregate QoS
expectations
  * for the pm_qos_class of parameters.
  */
-int pm_qos_add_requirement(int pm_qos_class, char *name, s32 value)
+struct requirement_list *pm_qos_add_requirement(int pm_qos_class,
char *name,
+		s32 value)
 {
 	struct requirement_list *dep;
 	unsigned long flags;
 
 	dep = kzalloc(sizeof(struct requirement_list), GFP_KERNEL);
 	if (dep) {
+		dep->pm_qos_class = pm_qos_class;
+
 		if (value == PM_QOS_DEFAULT_VALUE)
 			dep->value =
pm_qos_array[pm_qos_class]->default_value;
 		else
@@ -228,48 +232,37 @@ int pm_qos_add_requirement(int pm_qos_class,
char *name, s32 value)
 		spin_unlock_irqrestore(&pm_qos_lock, flags);
 		update_target(pm_qos_class);
 
-		return 0;
+		return dep;
 	}
 
 cleanup:
 	kfree(dep);
-	return -ENOMEM;
+	return ERR_PTR(-ENOMEM);
 }
 EXPORT_SYMBOL_GPL(pm_qos_add_requirement);
 
 /**
  * pm_qos_update_requirement - modifies an existing qos request
- * @pm_qos_class: identifies which list of qos request to us
- * @name: identifies the request
+ * @qos: identifies the qos request to us
  * @value: defines the qos request
  *
- * Updates an existing qos requirement for the pm_qos_class of
parameters along
+ * Updates an existing qos requirement along
  * with updating the target pm_qos_class value.
- *
- * If the named request isn't in the list then no change is made.
  */
-int pm_qos_update_requirement(int pm_qos_class, char *name, s32
new_value)
+int pm_qos_update_requirement(struct requirement_list *qos, s32
new_value)
 {
 	unsigned long flags;
-	struct requirement_list *node;
 	int pending_update = 0;
 
 	spin_lock_irqsave(&pm_qos_lock, flags);
-	list_for_each_entry(node,
-		&pm_qos_array[pm_qos_class]->requirements.list, list)
{
-		if (strcmp(node->name, name) == 0) {
-			if (new_value == PM_QOS_DEFAULT_VALUE)
-				node->value =
-
pm_qos_array[pm_qos_class]->default_value;
-			else
-				node->value = new_value;
-			pending_update = 1;
-			break;
-		}
-	}
+	if (new_value == PM_QOS_DEFAULT_VALUE)
+		qos->value =
pm_qos_array[qos->pm_qos_class]->default_value;
+	else
+		qos->value = new_value;
+	pending_update = 1;
 	spin_unlock_irqrestore(&pm_qos_lock, flags);
 	if (pending_update)
-		update_target(pm_qos_class);
+		update_target(qos->pm_qos_class);
 
 	return 0;
 }
@@ -277,32 +270,26 @@ EXPORT_SYMBOL_GPL(pm_qos_update_requirement);
 
 /**
  * pm_qos_remove_requirement - modifies an existing qos request
- * @pm_qos_class: identifies which list of qos request to us
+ * @qos: identifies the qos request to us
  * @name: identifies the request
  *
- * Will remove named qos request from pm_qos_class list of
parameters and
+ * Will remove qos request from pm_qos_class list and
  * recompute the current target value for the pm_qos_class.
  */
-void pm_qos_remove_requirement(int pm_qos_class, char *name)
+void pm_qos_remove_requirement(struct requirement_list *qos)
 {
 	unsigned long flags;
-	struct requirement_list *node;
 	int pending_update = 0;
 
 	spin_lock_irqsave(&pm_qos_lock, flags);
-	list_for_each_entry(node,
-		&pm_qos_array[pm_qos_class]->requirements.list, list)
{
-		if (strcmp(node->name, name) == 0) {
-			kfree(node->name);
-			list_del(&node->list);
-			kfree(node);
-			pending_update = 1;
-			break;
-		}
-	}
+	list_del(&qos->list);
+	pending_update = 1;
 	spin_unlock_irqrestore(&pm_qos_lock, flags);
 	if (pending_update)
-		update_target(pm_qos_class);
+		update_target(qos->pm_qos_class);
+
+	kfree(qos->name);
+	kfree(qos);
 }
 EXPORT_SYMBOL_GPL(pm_qos_remove_requirement);
 
@@ -345,37 +332,42 @@ int pm_qos_remove_notifier(int pm_qos_class,
struct notifier_block *notifier)
 EXPORT_SYMBOL_GPL(pm_qos_remove_notifier);
 
 #define PID_NAME_LEN sizeof("process_1234567890")
-static char name[PID_NAME_LEN];
 
 static int pm_qos_power_open(struct inode *inode, struct file *filp)
 {
 	int ret;
 	long pm_qos_class;
+	char name[PID_NAME_LEN];
+	struct requirement_list *qos;
 
 	lock_kernel();
 	pm_qos_class = find_pm_qos_object_by_minor(iminor(inode));
-	if (pm_qos_class >= 0) {
-		filp->private_data = (void *)pm_qos_class;
-		sprintf(name, "process_%d", current->pid);
-		ret = pm_qos_add_requirement(pm_qos_class, name,
-					PM_QOS_DEFAULT_VALUE);
-		if (ret >= 0) {
-			unlock_kernel();
-			return 0;
-		}
+	if (pm_qos_class < 0) {
+		ret = -EPERM;
+		goto power_open_exit;
 	}
-	unlock_kernel();
 
-	return -EPERM;
+	sprintf(name, "process_%d", current->pid);
+	qos = pm_qos_add_requirement(pm_qos_class, name,
PM_QOS_DEFAULT_VALUE);
+	if (IS_ERR(qos)) {
+		ret = PTR_ERR(qos);
+		goto power_open_exit;
+	}
+
+	filp->private_data = qos;
+	ret = 0;
+
+power_open_exit:
+	unlock_kernel();
+	return ret;
 }
 
 static int pm_qos_power_release(struct inode *inode, struct file
*filp)
 {
-	int pm_qos_class;
+	struct requirement_list *qos;
 
-	pm_qos_class = (long)filp->private_data;
-	sprintf(name, "process_%d", current->pid);
-	pm_qos_remove_requirement(pm_qos_class, name);
+	qos = (struct requirement_list *)filp->private_data;
+	pm_qos_remove_requirement(qos);
 
 	return 0;
 }
@@ -384,15 +376,15 @@ static ssize_t pm_qos_power_write(struct file
*filp, const char __user *buf,
 		size_t count, loff_t *f_pos)
 {
 	s32 value;
-	int pm_qos_class;
+	struct requirement_list *qos;
 
-	pm_qos_class = (long)filp->private_data;
 	if (count != sizeof(s32))
 		return -EINVAL;
 	if (copy_from_user(&value, buf, sizeof(s32)))
 		return -EFAULT;
-	sprintf(name, "process_%d", current->pid);
-	pm_qos_update_requirement(pm_qos_class, name, value);
+
+	qos = (struct requirement_list *)filp->private_data;
+	pm_qos_update_requirement(qos, value);
 
 	return  sizeof(s32);
 }


~Ai

Re: adding handles to pm_qos?

[640E9920]

[-- Attachment #1.1: Type: text/plain, Size: 9079 bytes --]

On Tue, Nov 17, 2009 at 06:06:56PM -0700, Ai Li wrote:
> Date: Tue, 17 Nov 2009 18:06:56 -0700
> From: Ai Li <aili@codeaurora.org>
> To: mgross@linux.intel.com
> Cc: linux-pm@lists.linux-foundation.org
> Subject: Re: [linux-pm] adding handles to pm_qos?
> 
> > Thanks, I'll look at it over the next few days.
> > 
> > --mgross
> > 
> > > Here is the patch that I use with the test code.  There are
> > three new
> > > functions:
> > >
> > > void *pm_qos_get(int qos, char *name);
> > > int pm_qos_put(void *handle);
> > > int pm_qos_update_requirement_direct(void *handle, s32
> > new_value);
> > >
> > > In the test, I wanted to keep the existing interface intact so
> > I
> > > could compare them at the same time.  For the formal code
> > submission,
> > > a different approach may work better.
> 
> 
> Here is an alternate way of adding handles to pm_qos_params that I
> was alluding to.  This approach may be more preferable because it
> does not bloat the API and the handles become an integral part of
> pm_qos_params.  In my previous patch, handles are kind of bolted onto
> pm_qos_params and it needs separate calls (pm_qos_get, pm_qos_put) to
> acquire and release the handle.  In this patch,
> pm_qos_add_requirement and pm_qos_remove_requirement automatically
> take care of the handles.
> 
> 
> --- include/linux/pm_qos_params.h.orig
> +++ include/linux/pm_qos_params.h
> @@ -14,9 +14,11 @@
>  #define PM_QOS_NUM_CLASSES 4
>  #define PM_QOS_DEFAULT_VALUE -1
>  
> -int pm_qos_add_requirement(int qos, char *name, s32 value);
> -int pm_qos_update_requirement(int qos, char *name, s32 new_value);
> -void pm_qos_remove_requirement(int qos, char *name);
> +struct requirement_list;
> +
> +struct requirement_list *pm_qos_add_requirement(int qos, char *name,
> s32 value);
> +int pm_qos_update_requirement(struct requirement_list *qos, s32
> new_value);
> +void pm_qos_remove_requirement(struct requirement_list *qos);
>  
>  int pm_qos_requirement(int qos);
>  
> --- kernel/pm_qos_params.c.orig
> +++ kernel/pm_qos_params.c
> @@ -49,6 +49,7 @@
>   */
>  struct requirement_list {
>  	struct list_head list;
> +	int pm_qos_class;
>  	union {
>  		s32 value;
>  		s32 usec;
> @@ -207,13 +208,16 @@ EXPORT_SYMBOL_GPL(pm_qos_requirement);
>   * performance characteristics.  It recomputes the aggregate QoS
> expectations
>   * for the pm_qos_class of parameters.
>   */
> -int pm_qos_add_requirement(int pm_qos_class, char *name, s32 value)
> +struct requirement_list *pm_qos_add_requirement(int pm_qos_class,
> char *name,

looks like your email client buggered the your patch.

mgross@mgross-laptop:~/marks/Linux/linux-2.6$ patch -p0 < /home/mgross/pm_qos_handle.patch
patching file include/linux/pm_qos_params.h
patch: **** malformed patch at line 43: s32 value);

Please resend 

--mgross



> +		s32 value)
>  {
>  	struct requirement_list *dep;
>  	unsigned long flags;
>  
>  	dep = kzalloc(sizeof(struct requirement_list), GFP_KERNEL);
>  	if (dep) {
> +		dep->pm_qos_class = pm_qos_class;
> +
>  		if (value == PM_QOS_DEFAULT_VALUE)
>  			dep->value =
> pm_qos_array[pm_qos_class]->default_value;
>  		else
> @@ -228,48 +232,37 @@ int pm_qos_add_requirement(int pm_qos_class,
> char *name, s32 value)
>  		spin_unlock_irqrestore(&pm_qos_lock, flags);
>  		update_target(pm_qos_class);
>  
> -		return 0;
> +		return dep;
>  	}
>  
>  cleanup:
>  	kfree(dep);
> -	return -ENOMEM;
> +	return ERR_PTR(-ENOMEM);
>  }
>  EXPORT_SYMBOL_GPL(pm_qos_add_requirement);
>  
>  /**
>   * pm_qos_update_requirement - modifies an existing qos request
> - * @pm_qos_class: identifies which list of qos request to us
> - * @name: identifies the request
> + * @qos: identifies the qos request to us
>   * @value: defines the qos request
>   *
> - * Updates an existing qos requirement for the pm_qos_class of
> parameters along
> + * Updates an existing qos requirement along
>   * with updating the target pm_qos_class value.
> - *
> - * If the named request isn't in the list then no change is made.
>   */
> -int pm_qos_update_requirement(int pm_qos_class, char *name, s32
> new_value)
> +int pm_qos_update_requirement(struct requirement_list *qos, s32
> new_value)
>  {
>  	unsigned long flags;
> -	struct requirement_list *node;
>  	int pending_update = 0;
>  
>  	spin_lock_irqsave(&pm_qos_lock, flags);
> -	list_for_each_entry(node,
> -		&pm_qos_array[pm_qos_class]->requirements.list, list)
> {
> -		if (strcmp(node->name, name) == 0) {
> -			if (new_value == PM_QOS_DEFAULT_VALUE)
> -				node->value =
> -
> pm_qos_array[pm_qos_class]->default_value;
> -			else
> -				node->value = new_value;
> -			pending_update = 1;
> -			break;
> -		}
> -	}
> +	if (new_value == PM_QOS_DEFAULT_VALUE)
> +		qos->value =
> pm_qos_array[qos->pm_qos_class]->default_value;
> +	else
> +		qos->value = new_value;
> +	pending_update = 1;
>  	spin_unlock_irqrestore(&pm_qos_lock, flags);
>  	if (pending_update)
> -		update_target(pm_qos_class);
> +		update_target(qos->pm_qos_class);
>  
>  	return 0;
>  }
> @@ -277,32 +270,26 @@ EXPORT_SYMBOL_GPL(pm_qos_update_requirement);
>  
>  /**
>   * pm_qos_remove_requirement - modifies an existing qos request
> - * @pm_qos_class: identifies which list of qos request to us
> + * @qos: identifies the qos request to us
>   * @name: identifies the request
>   *
> - * Will remove named qos request from pm_qos_class list of
> parameters and
> + * Will remove qos request from pm_qos_class list and
>   * recompute the current target value for the pm_qos_class.
>   */
> -void pm_qos_remove_requirement(int pm_qos_class, char *name)
> +void pm_qos_remove_requirement(struct requirement_list *qos)
>  {
>  	unsigned long flags;
> -	struct requirement_list *node;
>  	int pending_update = 0;
>  
>  	spin_lock_irqsave(&pm_qos_lock, flags);
> -	list_for_each_entry(node,
> -		&pm_qos_array[pm_qos_class]->requirements.list, list)
> {
> -		if (strcmp(node->name, name) == 0) {
> -			kfree(node->name);
> -			list_del(&node->list);
> -			kfree(node);
> -			pending_update = 1;
> -			break;
> -		}
> -	}
> +	list_del(&qos->list);
> +	pending_update = 1;
>  	spin_unlock_irqrestore(&pm_qos_lock, flags);
>  	if (pending_update)
> -		update_target(pm_qos_class);
> +		update_target(qos->pm_qos_class);
> +
> +	kfree(qos->name);
> +	kfree(qos);
>  }
>  EXPORT_SYMBOL_GPL(pm_qos_remove_requirement);
>  
> @@ -345,37 +332,42 @@ int pm_qos_remove_notifier(int pm_qos_class,
> struct notifier_block *notifier)
>  EXPORT_SYMBOL_GPL(pm_qos_remove_notifier);
>  
>  #define PID_NAME_LEN sizeof("process_1234567890")
> -static char name[PID_NAME_LEN];
>  
>  static int pm_qos_power_open(struct inode *inode, struct file *filp)
>  {
>  	int ret;
>  	long pm_qos_class;
> +	char name[PID_NAME_LEN];
> +	struct requirement_list *qos;
>  
>  	lock_kernel();
>  	pm_qos_class = find_pm_qos_object_by_minor(iminor(inode));
> -	if (pm_qos_class >= 0) {
> -		filp->private_data = (void *)pm_qos_class;
> -		sprintf(name, "process_%d", current->pid);
> -		ret = pm_qos_add_requirement(pm_qos_class, name,
> -					PM_QOS_DEFAULT_VALUE);
> -		if (ret >= 0) {
> -			unlock_kernel();
> -			return 0;
> -		}
> +	if (pm_qos_class < 0) {
> +		ret = -EPERM;
> +		goto power_open_exit;
>  	}
> -	unlock_kernel();
>  
> -	return -EPERM;
> +	sprintf(name, "process_%d", current->pid);
> +	qos = pm_qos_add_requirement(pm_qos_class, name,
> PM_QOS_DEFAULT_VALUE);
> +	if (IS_ERR(qos)) {
> +		ret = PTR_ERR(qos);
> +		goto power_open_exit;
> +	}
> +
> +	filp->private_data = qos;
> +	ret = 0;
> +
> +power_open_exit:
> +	unlock_kernel();
> +	return ret;
>  }
>  
>  static int pm_qos_power_release(struct inode *inode, struct file
> *filp)
>  {
> -	int pm_qos_class;
> +	struct requirement_list *qos;
>  
> -	pm_qos_class = (long)filp->private_data;
> -	sprintf(name, "process_%d", current->pid);
> -	pm_qos_remove_requirement(pm_qos_class, name);
> +	qos = (struct requirement_list *)filp->private_data;
> +	pm_qos_remove_requirement(qos);
>  
>  	return 0;
>  }
> @@ -384,15 +376,15 @@ static ssize_t pm_qos_power_write(struct file
> *filp, const char __user *buf,
>  		size_t count, loff_t *f_pos)
>  {
>  	s32 value;
> -	int pm_qos_class;
> +	struct requirement_list *qos;
>  
> -	pm_qos_class = (long)filp->private_data;
>  	if (count != sizeof(s32))
>  		return -EINVAL;
>  	if (copy_from_user(&value, buf, sizeof(s32)))
>  		return -EFAULT;
> -	sprintf(name, "process_%d", current->pid);
> -	pm_qos_update_requirement(pm_qos_class, name, value);
> +
> +	qos = (struct requirement_list *)filp->private_data;
> +	pm_qos_update_requirement(qos, value);
>  
>  	return  sizeof(s32);
>  }
> 
> 
> ~Ai
> 
> _______________________________________________
> linux-pm mailing list
> linux-pm@lists.linux-foundation.org
> https://lists.linux-foundation.org/mailman/listinfo/linux-pm

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 197 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]