[PATCH, RESEND 0/2] pm_qos: remove BKL and de-race

[PATCH, RESEND 0/2] pm_qos: remove BKL and de-race

[Jonathan Corbet]

Here's a repost of my two-patch set getting rid of the BKL and some
racy code in pm_qos_params.c.  There's been no changes since the last
time around other than the addition of a couple of tags.  Patches
follow, or they can be pulled from:

	git://git.lwn.net/linux-2.6.git bkl-removal

Ingo, you didn't say whether these could find a home in your
violent-death-to-the-bkl tree until the merge window opens.  So they're
still looking for a place to rest; I guess I could ask Stephen to take
the bkl-removal tree back into linux-next, but that seems like
overkill.

Thanks,

jon

[PATCH 1/2] pm_qos: remove the BKL

[Jonathan Corbet]

pm_qos_power_open got its lock_kernel() calls from the open() pushdown.  A
look at the code shows that the only global resources accessed are
pm_qos_array and "name".  pm_qos_array doesn't change (things pointed to
therein do change, but they are atomics and/or are protected by
pm_qos_lock).  Accesses to "name" are totally unprotected with or without
the BKL; that will be fixed shortly.  The BKL is not helpful here; take it
out.

Signed-off-by: Jonathan Corbet <corbet@lwn.net>
Cc: Mark Gross <mgross@linux.intel.com>
---
 kernel/pm_qos_params.c |    8 +-------
 1 files changed, 1 insertions(+), 7 deletions(-)

diff --git a/kernel/pm_qos_params.c b/kernel/pm_qos_params.c
index dfdec52..d96b83e 100644
--- a/kernel/pm_qos_params.c
+++ b/kernel/pm_qos_params.c
@@ -29,7 +29,6 @@
 
 #include <linux/pm_qos_params.h>
 #include <linux/sched.h>
-#include <linux/smp_lock.h>
 #include <linux/spinlock.h>
 #include <linux/slab.h>
 #include <linux/time.h>
@@ -352,20 +351,15 @@ static int pm_qos_power_open(struct inode *inode, struct file *filp)
 	int ret;
 	long pm_qos_class;
 
-	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();
+		if (ret >= 0)
 			return 0;
-		}
 	}
-	unlock_kernel();
-
 	return -EPERM;
 }
 
-- 
1.6.2.5

[PATCH 2/2] pm_qos: clean up racy file-global variable

[Jonathan Corbet]

"name" is a poor name for a file-global variable.  It was used in three
different functions, with no mutual exclusion.  But it's just a tiny,
temporary string; let's just move it onto the stack in the functions that
need it.  Also use snprintf() just in case.

Signed-off-by: Jonathan Corbet <corbet@lwn.net>
Cc: Mark Gross <mgross@linux.intel.com>
Reviewed-by: Frederic Weisbecker <fweisbec@gmail.com>
---
 kernel/pm_qos_params.c |   12 +++++++-----
 1 files changed, 7 insertions(+), 5 deletions(-)

diff --git a/kernel/pm_qos_params.c b/kernel/pm_qos_params.c
index d96b83e..3db49b9 100644
--- a/kernel/pm_qos_params.c
+++ b/kernel/pm_qos_params.c
@@ -343,18 +343,18 @@ 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];
+#define PID_NAME_LEN 32
 
 static int pm_qos_power_open(struct inode *inode, struct file *filp)
 {
 	int ret;
 	long pm_qos_class;
+	char name[PID_NAME_LEN];
 
 	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);
+		snprintf(name, PID_NAME_LEN, "process_%d", current->pid);
 		ret = pm_qos_add_requirement(pm_qos_class, name,
 					PM_QOS_DEFAULT_VALUE);
 		if (ret >= 0)
@@ -366,9 +366,10 @@ static int pm_qos_power_open(struct inode *inode, struct file *filp)
 static int pm_qos_power_release(struct inode *inode, struct file *filp)
 {
 	int pm_qos_class;
+	char name[PID_NAME_LEN];
 
 	pm_qos_class = (long)filp->private_data;
-	sprintf(name, "process_%d", current->pid);
+	snprintf(name, PID_NAME_LEN, "process_%d", current->pid);
 	pm_qos_remove_requirement(pm_qos_class, name);
 
 	return 0;
@@ -379,13 +380,14 @@ static ssize_t pm_qos_power_write(struct file *filp, const char __user *buf,
 {
 	s32 value;
 	int pm_qos_class;
+	char name[PID_NAME_LEN];
 
 	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);
+	snprintf(name, PID_NAME_LEN, "process_%d", current->pid);
 	pm_qos_update_requirement(pm_qos_class, name, value);
 
 	return  sizeof(s32);
-- 
1.6.2.5

Re: [PATCH, RESEND 0/2] pm_qos: remove BKL and de-race

[Thomas Gleixner]

Jonathan,

On Thu, 13 Aug 2009, Jonathan Corbet wrote:

> Here's a repost of my two-patch set getting rid of the BKL and some
> racy code in pm_qos_params.c.  There's been no changes since the last
> time around other than the addition of a couple of tags.  Patches
> follow, or they can be pulled from:
> 
> 	git://git.lwn.net/linux-2.6.git bkl-removal
> 
> Ingo, you didn't say whether these could find a home in your
> violent-death-to-the-bkl tree until the merge window opens.  So they're
> still looking for a place to rest; I guess I could ask Stephen to take
> the bkl-removal tree back into linux-next, but that seems like
> overkill.

Pulled it into

 git://git.kernel.org/pub/scm/linux/kernel/git/tip/linux-2.6-tip.git kill-the-bkl-really

Thanks,

	tglx

Re: [PATCH, RESEND 0/2] pm_qos: remove BKL and de-race

[mgross]

On Thu, Aug 13, 2009 at 02:05:54PM -0600, Jonathan Corbet wrote:

Sorry for the late reply.  I'll look over the patches shortly.

--mgross

> Here's a repost of my two-patch set getting rid of the BKL and some
> racy code in pm_qos_params.c.  There's been no changes since the last
> time around other than the addition of a couple of tags.  Patches
> follow, or they can be pulled from:
> 
> 	git://git.lwn.net/linux-2.6.git bkl-removal
> 
> Ingo, you didn't say whether these could find a home in your
> violent-death-to-the-bkl tree until the merge window opens.  So they're
> still looking for a place to rest; I guess I could ask Stephen to take
> the bkl-removal tree back into linux-next, but that seems like
> overkill.
> 
> Thanks,
> 
> jon

Re: [PATCH 1/2] pm_qos: remove the BKL

[mgross]

On Thu, Aug 13, 2009 at 02:07:16PM -0600, Jonathan Corbet wrote:
> pm_qos_power_open got its lock_kernel() calls from the open() pushdown.  A
> look at the code shows that the only global resources accessed are
> pm_qos_array and "name".  pm_qos_array doesn't change (things pointed to
> therein do change, but they are atomics and/or are protected by
> pm_qos_lock).  Accesses to "name" are totally unprotected with or without
> the BKL; that will be fixed shortly.  The BKL is not helpful here; take it
> out.



Signed-off-by: Mark Gross <mgross@linux.intel.com>

--mgross

> 
> Signed-off-by: Jonathan Corbet <corbet@lwn.net>
> Cc: Mark Gross <mgross@linux.intel.com>
> ---
>  kernel/pm_qos_params.c |    8 +-------
>  1 files changed, 1 insertions(+), 7 deletions(-)
> 
> diff --git a/kernel/pm_qos_params.c b/kernel/pm_qos_params.c
> index dfdec52..d96b83e 100644
> --- a/kernel/pm_qos_params.c
> +++ b/kernel/pm_qos_params.c
> @@ -29,7 +29,6 @@
>  
>  #include <linux/pm_qos_params.h>
>  #include <linux/sched.h>
> -#include <linux/smp_lock.h>
>  #include <linux/spinlock.h>
>  #include <linux/slab.h>
>  #include <linux/time.h>
> @@ -352,20 +351,15 @@ static int pm_qos_power_open(struct inode *inode, struct file *filp)
>  	int ret;
>  	long pm_qos_class;
>  
> -	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();
> +		if (ret >= 0)
>  			return 0;
> -		}
>  	}
> -	unlock_kernel();
> -
>  	return -EPERM;
>  }
>  
> -- 
> 1.6.2.5

Re: [PATCH 2/2] pm_qos: clean up racy file-global variable

[mgross]

On Thu, Aug 13, 2009 at 02:08:25PM -0600, Jonathan Corbet wrote:
> "name" is a poor name for a file-global variable.  It was used in three
True. 

> different functions, with no mutual exclusion.  But it's just a tiny,
> temporary string; let's just move it onto the stack in the functions that
> need it.  Also use snprintf() just in case.
> 

Signed-off-by: Mark Gross <mgross@intel.com>


> Signed-off-by: Jonathan Corbet <corbet@lwn.net>
> Cc: Mark Gross <mgross@linux.intel.com>
> Reviewed-by: Frederic Weisbecker <fweisbec@gmail.com>
> ---
>  kernel/pm_qos_params.c |   12 +++++++-----
>  1 files changed, 7 insertions(+), 5 deletions(-)
> 
> diff --git a/kernel/pm_qos_params.c b/kernel/pm_qos_params.c
> index d96b83e..3db49b9 100644
> --- a/kernel/pm_qos_params.c
> +++ b/kernel/pm_qos_params.c
> @@ -343,18 +343,18 @@ 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];
> +#define PID_NAME_LEN 32
>  
>  static int pm_qos_power_open(struct inode *inode, struct file *filp)
>  {
>  	int ret;
>  	long pm_qos_class;
> +	char name[PID_NAME_LEN];
>  
>  	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);
> +		snprintf(name, PID_NAME_LEN, "process_%d", current->pid);
>  		ret = pm_qos_add_requirement(pm_qos_class, name,
>  					PM_QOS_DEFAULT_VALUE);
>  		if (ret >= 0)
> @@ -366,9 +366,10 @@ static int pm_qos_power_open(struct inode *inode, struct file *filp)
>  static int pm_qos_power_release(struct inode *inode, struct file *filp)
>  {
>  	int pm_qos_class;
> +	char name[PID_NAME_LEN];
>  
>  	pm_qos_class = (long)filp->private_data;
> -	sprintf(name, "process_%d", current->pid);
> +	snprintf(name, PID_NAME_LEN, "process_%d", current->pid);
>  	pm_qos_remove_requirement(pm_qos_class, name);
>  
>  	return 0;
> @@ -379,13 +380,14 @@ static ssize_t pm_qos_power_write(struct file *filp, const char __user *buf,
>  {
>  	s32 value;
>  	int pm_qos_class;
> +	char name[PID_NAME_LEN];
>  
>  	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);
> +	snprintf(name, PID_NAME_LEN, "process_%d", current->pid);
>  	pm_qos_update_requirement(pm_qos_class, name, value);
>  
>  	return  sizeof(s32);
> -- 
> 1.6.2.5