[PATCH 2.6.25-rc1] cpufreq: fix cpufreq policy refcount imbalance

[PATCH 2.6.25-rc1] cpufreq: fix cpufreq policy refcount imbalance

[Yi Yang]

When one cpu is set to offline, the caller process will hang, according to
the trace data, the problem lies in the refcount error in cpufreq driver,
cpufreq_cpu_callback will wait for completion policy->kobj_unregister
which is nerver completed because a refcount error in function
__cpufreq_remove_dev in file driver/cpufreq/cpufreq.c results in not
calling kobject release method.

In driver/cpufreq/cpufreq.c, the refcount of data->kobj isn't 1 when it
will be unregistered, this problem didn't exist in 2.6.24 and earlier.

The root cause is kobject API switch, kobject_init_and_add and kobject_put
replace older kobject_register and kobject_unregister in 2.6.25-rc1,
compared to 2.6.24, kobject_unregister is deleted in function
__cpufreq_remove_dev but it isn't replaced with kobject_put.

This patch adds kobject_put to balance refcount. I noticed Greg suggests
it will fix a power-off issue to remove kobject_get statement block, but i
think that isn't the best way because those code block has existed very long
and it is helpful because the successive statements are invoking relevant
data.


Signed-off-by: Yi Yang <yi.y.yang@intel.com>
---
 cpufreq.c |    5 +++++
 1 file changed, 5 insertions(+)

--- a/drivers/cpufreq/cpufreq.c	2008-02-15 04:41:29.000000000 +0800
+++ b/drivers/cpufreq/cpufreq.c	2008-02-15 06:56:56.000000000 +0800
@@ -1057,12 +1057,17 @@ static int __cpufreq_remove_dev (struct 
 
 	unlock_policy_rwsem_write(cpu);
 
+	/* it matches the previous kobject_get */
 	kobject_put(&data->kobj);
 
 	/* we need to make sure that the underlying kobj is actually
 	 * not referenced anymore by anybody before we proceed with
 	 * unloading.
 	 */
+
+	/* unregister data->kobj, it matches kobject_init_and_add */
+	kobject_put(&data->kobj);
+
 	dprintk("waiting for dropping of refcount\n");
 	wait_for_completion(&data->kobj_unregister);
 	dprintk("wait complete\n");

[PATCH 2.6.25-rc1] cpufreq: fix cpufreq policy refcount imbalance

[Yi Yang]

When one cpu is set to offline, the caller process will hang, according to
the trace data, the problem lies in the refcount error in cpufreq driver,
cpufreq_cpu_callback will wait for completion policy->kobj_unregister
which is nerver completed because a refcount error in function
__cpufreq_remove_dev in file driver/cpufreq/cpufreq.c results in not
calling kobject release method.

In driver/cpufreq/cpufreq.c, the refcount of data->kobj isn't 1 when it
will be unregistered, this problem didn't exist in 2.6.24 and earlier.

The root cause is kobject API switch, kobject_init_and_add and kobject_put
replace older kobject_register and kobject_unregister in 2.6.25-rc1,
compared to 2.6.24, kobject_unregister is deleted in function
__cpufreq_remove_dev but it isn't replaced with kobject_put.

This patch adds kobject_put to balance refcount. I noticed Greg suggests
it will fix a power-off issue to remove kobject_get statement block, but i
think that isn't the best way because those code block has existed very long
and it is helpful because the successive statements are invoking relevant
data.


Signed-off-by: Yi Yang <yi.y.yang@intel.com>
---
 cpufreq.c |    5 +++++
 1 file changed, 5 insertions(+)

--- a/drivers/cpufreq/cpufreq.c	2008-02-15 04:41:29.000000000 +0800
+++ b/drivers/cpufreq/cpufreq.c	2008-02-15 06:56:56.000000000 +0800
@@ -1057,12 +1057,17 @@ static int __cpufreq_remove_dev (struct 
 
 	unlock_policy_rwsem_write(cpu);
 
+	/* it matches the previous kobject_get */
 	kobject_put(&data->kobj);
 
 	/* we need to make sure that the underlying kobj is actually
 	 * not referenced anymore by anybody before we proceed with
 	 * unloading.
 	 */
+
+	/* unregister data->kobj, it matches kobject_init_and_add */
+	kobject_put(&data->kobj);
+
 	dprintk("waiting for dropping of refcount\n");
 	wait_for_completion(&data->kobj_unregister);
 	dprintk("wait complete\n");

Re: [linux-pm] [PATCH 2.6.25-rc1] cpufreq: fix cpufreq policy refcount imbalance

[Alan Stern]

On Fri, 15 Feb 2008, Yi Yang wrote:

> This patch adds kobject_put to balance refcount. I noticed Greg suggests
> it will fix a power-off issue to remove kobject_get statement block, but i
> think that isn't the best way because those code block has existed very long
> and it is helpful because the successive statements are invoking relevant
> data.

Are you referring to this section of code (before the region affected 
by your patch)?

	if (!kobject_get(&data->kobj)) {
		spin_unlock_irqrestore(&cpufreq_driver_lock, flags);
		cpufreq_debug_enable_ratelimit();
		unlock_policy_rwsem_write(cpu);
		return -EFAULT;
	}

Greg is correct that the kobject_get() here is useless and should be
removed.  kobject_get() never returns NULL unless its argument is NULL.  
Since &data->kobj can never be NULL, the "if" test will never fail.  
Hence there's no point in making the test at all.

The fact that a section of code has existed for a long time doesn't 
mean that it is right.  :-)

Furthermore, there's no reason to do the kobject_get().  Holding 2 
references to a kobject is no better than holding just 1 reference.  
Assuming you know that the kobject is still registered, then you also 
know that there is already a reference to it.  So you have no reason to 
take an additional reference.

Alan Stern

Re: [linux-pm] [PATCH 2.6.25-rc1] cpufreq: fix cpufreq policy refcount imbalance

[Greg KH]

On Fri, Feb 15, 2008 at 10:52:51AM -0500, Alan Stern wrote:
> On Fri, 15 Feb 2008, Yi Yang wrote:
> 
> > This patch adds kobject_put to balance refcount. I noticed Greg suggests
> > it will fix a power-off issue to remove kobject_get statement block, but i
> > think that isn't the best way because those code block has existed very long
> > and it is helpful because the successive statements are invoking relevant
> > data.
> 
> Are you referring to this section of code (before the region affected 
> by your patch)?
> 
> 	if (!kobject_get(&data->kobj)) {
> 		spin_unlock_irqrestore(&cpufreq_driver_lock, flags);
> 		cpufreq_debug_enable_ratelimit();
> 		unlock_policy_rwsem_write(cpu);
> 		return -EFAULT;
> 	}
> 
> Greg is correct that the kobject_get() here is useless and should be
> removed.  kobject_get() never returns NULL unless its argument is NULL.  
> Since &data->kobj can never be NULL, the "if" test will never fail.  
> Hence there's no point in making the test at all.
> 
> The fact that a section of code has existed for a long time doesn't 
> mean that it is right.  :-)
> 
> Furthermore, there's no reason to do the kobject_get().  Holding 2 
> references to a kobject is no better than holding just 1 reference.  
> Assuming you know that the kobject is still registered, then you also 
> know that there is already a reference to it.  So you have no reason to 
> take an additional reference.

There's the additional problem that this second reference count is never
dropped, causing a bug :)

thanks,

greg k-h

Re: [PATCH 2.6.25-rc1] cpufreq: fix cpufreq policy refcount imbalance

[Greg KH]

On Fri, Feb 15, 2008 at 07:48:41AM +0800, Yi Yang wrote:
> When one cpu is set to offline, the caller process will hang, according to
> the trace data, the problem lies in the refcount error in cpufreq driver,
> cpufreq_cpu_callback will wait for completion policy->kobj_unregister
> which is nerver completed because a refcount error in function
> __cpufreq_remove_dev in file driver/cpufreq/cpufreq.c results in not
> calling kobject release method.
> 
> In driver/cpufreq/cpufreq.c, the refcount of data->kobj isn't 1 when it
> will be unregistered, this problem didn't exist in 2.6.24 and earlier.
> 
> The root cause is kobject API switch, kobject_init_and_add and kobject_put
> replace older kobject_register and kobject_unregister in 2.6.25-rc1,
> compared to 2.6.24, kobject_unregister is deleted in function
> __cpufreq_remove_dev but it isn't replaced with kobject_put.
> 
> This patch adds kobject_put to balance refcount. I noticed Greg suggests
> it will fix a power-off issue to remove kobject_get statement block, but i
> think that isn't the best way because those code block has existed very long
> and it is helpful because the successive statements are invoking relevant
> data.
> 
> 
> Signed-off-by: Yi Yang <yi.y.yang@intel.com>

No, the additional kobject_get() needs to be removed.  I posted a patch
for this last night, and so did someone else earlier at:
		http://lkml.org/lkml/2008/2/8/342

this patch should not be added, I'll get the other one in instead.

thanks,

greg k-h

[PATCH 2.6.25-rc3] cpuidle: fix cpuidle time and usage overflow

[Yi Yang]

cpuidle C-state sysfs node time and usage are very easy to overflow because
they are all of unsigned int type, time will overflow within about two hours,
usage will take longer time to overflow, but they are increasing for ever.

This patch will convert them to unsigned long long.


Signed-off-by: Yi Yang <yi.y.yang@intel.com>
---
 drivers/cpuidle/cpuidle.c |    2 +-
 drivers/cpuidle/sysfs.c   |   10 ++++++++--
 include/linux/cpuidle.h   |    4 ++--

--- a/include/linux/cpuidle.h	2008-02-25 02:31:26.000000000 -0500
+++ b/include/linux/cpuidle.h	2008-02-25 04:30:24.000000000 -0500
@@ -38,8 +38,8 @@ struct cpuidle_state {
 	unsigned int	power_usage; /* in mW */
 	unsigned int	target_residency; /* in US */
 
-	unsigned int	usage;
-	unsigned int	time; /* in US */
+	unsigned long long	usage;
+	unsigned long long	time; /* in US */
 
 	int (*enter)	(struct cpuidle_device *dev,
 			 struct cpuidle_state *state);
--- a/drivers/cpuidle/cpuidle.c	2008-02-25 02:37:14.000000000 -0500
+++ b/drivers/cpuidle/cpuidle.c	2008-02-25 04:29:19.000000000 -0500
@@ -67,7 +67,7 @@ static void cpuidle_idle_call(void)
 	/* enter the state and update stats */
 	dev->last_residency = target_state->enter(dev, target_state);
 	dev->last_state = target_state;
-	target_state->time += dev->last_residency;
+	target_state->time += (unsigned long long)dev->last_residency;
 	target_state->usage++;
 
 	/* give the governor an opportunity to reflect on the outcome */
--- a/drivers/cpuidle/sysfs.c	2008-02-25 02:33:14.000000000 -0500
+++ b/drivers/cpuidle/sysfs.c	2008-02-25 03:10:50.000000000 -0500
@@ -218,6 +218,12 @@ static ssize_t show_state_##_name(struct
 	return sprintf(buf, "%u\n", state->_name);\
 }
 
+#define define_show_state_ull_function(_name) \
+static ssize_t show_state_##_name(struct cpuidle_state *state, char *buf) \
+{ \
+	return sprintf(buf, "%llu\n", state->_name);\
+}
+
 #define define_show_state_str_function(_name) \
 static ssize_t show_state_##_name(struct cpuidle_state *state, char *buf) \
 { \
@@ -228,8 +234,8 @@ static ssize_t show_state_##_name(struct
 
 define_show_state_function(exit_latency)
 define_show_state_function(power_usage)
-define_show_state_function(usage)
-define_show_state_function(time)
+define_show_state_ull_function(usage)
+define_show_state_ull_function(time)
 define_show_state_str_function(name)
 define_show_state_str_function(desc)
 

Re: [PATCH 2.6.25-rc3] cpuidle: fix cpuidle time and usage overflow

[Yi Yang]

On Mon, 2008-02-25 at 11:15 +0100, Ingo Molnar wrote:
> * Yi Yang <yi.y.yang@intel.com> wrote:
> 
> > cpuidle C-state sysfs node time and usage are very easy to overflow 
> > because they are all of unsigned int type, time will overflow within 
> > about two hours, usage will take longer time to overflow, but they are 
> > increasing for ever.
> > 
> > This patch will convert them to unsigned long long.
> 
> what happens if such an overflow happens - any particular regression or 
> other misbehavior, or just funny looking stats in sysfs?
They are just stats info in sysfs, cpuidle's behaviors don't depend on
them. I didn't notice any regression or misbehaviors.

> 
> 	Ingo
> -
> To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2.6.25-rc3] cpuidle: fix cpuidle time and usage overflow

[Ingo Molnar]

* Yi Yang <yi.y.yang@intel.com> wrote:

> cpuidle C-state sysfs node time and usage are very easy to overflow 
> because they are all of unsigned int type, time will overflow within 
> about two hours, usage will take longer time to overflow, but they are 
> increasing for ever.
> 
> This patch will convert them to unsigned long long.

what happens if such an overflow happens - any particular regression or 
other misbehavior, or just funny looking stats in sysfs?

	Ingo

Re: [PATCH 2.6.25-rc3] cpuidle: fix cpuidle time and usage overflow

[Len Brown]

applied

thanks,
-len

On Sunday 24 February 2008, Yi Yang wrote:
> cpuidle C-state sysfs node time and usage are very easy to overflow because
> they are all of unsigned int type, time will overflow within about two hours,
> usage will take longer time to overflow, but they are increasing for ever.
> 
> This patch will convert them to unsigned long long.
> 
> 
> Signed-off-by: Yi Yang <yi.y.yang@intel.com>
> ---
>  drivers/cpuidle/cpuidle.c |    2 +-
>  drivers/cpuidle/sysfs.c   |   10 ++++++++--
>  include/linux/cpuidle.h   |    4 ++--
> 
> --- a/include/linux/cpuidle.h	2008-02-25 02:31:26.000000000 -0500
> +++ b/include/linux/cpuidle.h	2008-02-25 04:30:24.000000000 -0500
> @@ -38,8 +38,8 @@ struct cpuidle_state {
>  	unsigned int	power_usage; /* in mW */
>  	unsigned int	target_residency; /* in US */
>  
> -	unsigned int	usage;
> -	unsigned int	time; /* in US */
> +	unsigned long long	usage;
> +	unsigned long long	time; /* in US */
>  
>  	int (*enter)	(struct cpuidle_device *dev,
>  			 struct cpuidle_state *state);
> --- a/drivers/cpuidle/cpuidle.c	2008-02-25 02:37:14.000000000 -0500
> +++ b/drivers/cpuidle/cpuidle.c	2008-02-25 04:29:19.000000000 -0500
> @@ -67,7 +67,7 @@ static void cpuidle_idle_call(void)
>  	/* enter the state and update stats */
>  	dev->last_residency = target_state->enter(dev, target_state);
>  	dev->last_state = target_state;
> -	target_state->time += dev->last_residency;
> +	target_state->time += (unsigned long long)dev->last_residency;
>  	target_state->usage++;
>  
>  	/* give the governor an opportunity to reflect on the outcome */
> --- a/drivers/cpuidle/sysfs.c	2008-02-25 02:33:14.000000000 -0500
> +++ b/drivers/cpuidle/sysfs.c	2008-02-25 03:10:50.000000000 -0500
> @@ -218,6 +218,12 @@ static ssize_t show_state_##_name(struct
>  	return sprintf(buf, "%u\n", state->_name);\
>  }
>  
> +#define define_show_state_ull_function(_name) \
> +static ssize_t show_state_##_name(struct cpuidle_state *state, char *buf) \
> +{ \
> +	return sprintf(buf, "%llu\n", state->_name);\
> +}
> +
>  #define define_show_state_str_function(_name) \
>  static ssize_t show_state_##_name(struct cpuidle_state *state, char *buf) \
>  { \
> @@ -228,8 +234,8 @@ static ssize_t show_state_##_name(struct
>  
>  define_show_state_function(exit_latency)
>  define_show_state_function(power_usage)
> -define_show_state_function(usage)
> -define_show_state_function(time)
> +define_show_state_ull_function(usage)
> +define_show_state_ull_function(time)
>  define_show_state_str_function(name)
>  define_show_state_str_function(desc)
>  
> 
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
>