[PATCH V2 00/14] cpufreq: stats: cleanups

[PATCH V2 00/14] cpufreq: stats: cleanups

[Viresh Kumar]

This is a resend of cpufreq stats cleanups I had for sometime. Few issues were
reported from Fenguang's bot and are all fixed now and are stable..

V1->V2:
- Reviewed-by Prarit
- Replaced lock with mutex as we may sleep from within critical region and so a
  new patch "cpufreq: stats: replace spinlock with mutex".

Pushed here:
git://git.linaro.org/people/viresh.kumar/linux.git cpufreq/stats/cleanups

Viresh Kumar (14):
  cpufreq: stats: don't break strings into multiple lines
  cpufreq: stats: return -EEXIST when stats are already allocated
  cpufreq: stats: don't check for freq table while freeing stats
  cpufreq: stats: pass 'stat' to cpufreq_stats_update()
  cpufreq: stats: get rid of per-cpu cpufreq_stats_table
  cpufreq: Remove (now) unused 'last_cpu' from struct cpufreq_policy
  cpufreq: stats: remove cpufreq_stats_update_policy_cpu()
  cpufreq: remove CPUFREQ_UPDATE_POLICY_CPU notifications
  cpufreq: stats: create sysfs group once we are ready
  cpufreq: stats: don't update stats from show_trans_table()
  cpufreq: stats: don't update stats on false notifiers
  cpufreq: stats: replace spinlock with mutex
  cpufreq: stats: Fix locking
  cpufreq: stats: call cpufreq_stats_update() with locks held

 drivers/cpufreq/cpufreq.c       |   6 --
 drivers/cpufreq/cpufreq_stats.c | 148 ++++++++++++++++++----------------------
 include/linux/cpufreq.h         |  10 +--
 3 files changed, 73 insertions(+), 91 deletions(-)

-- 
2.2.0

[PATCH V2 01/14] cpufreq: stats: don't break strings into multiple lines

[Viresh Kumar]

The MODULE_DESCRIPTION() string is broken into multiple lines just to make
checkpatch happy. Its less readable now and prone to errors. Fix it.

Reviewed-by: Prarit Bhargava <prarit@redhat.com>
Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 drivers/cpufreq/cpufreq_stats.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/cpufreq/cpufreq_stats.c b/drivers/cpufreq/cpufreq_stats.c
index 0cd9b4dcef99..81be4d637ab4 100644
--- a/drivers/cpufreq/cpufreq_stats.c
+++ b/drivers/cpufreq/cpufreq_stats.c
@@ -374,8 +374,7 @@ static void __exit cpufreq_stats_exit(void)
 }
 
 MODULE_AUTHOR("Zou Nan hai <nanhai.zou@intel.com>");
-MODULE_DESCRIPTION("'cpufreq_stats' - A driver to export cpufreq stats "
-				"through sysfs filesystem");
+MODULE_DESCRIPTION("'cpufreq_stats' - exports cpufreq stats through sysfs filesystem");
 MODULE_LICENSE("GPL");
 
 module_init(cpufreq_stats_init);
-- 
2.2.0

Re: [PATCH V2 01/14] cpufreq: stats: don't break strings into multiple lines

[Rafael J. Wysocki]

On Friday, January 02, 2015 11:16:38 AM Viresh Kumar wrote:
> The MODULE_DESCRIPTION() string is broken into multiple lines just to make
> checkpatch happy. Its less readable now and prone to errors. Fix it.
> 
> Reviewed-by: Prarit Bhargava <prarit@redhat.com>
> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
> ---
>  drivers/cpufreq/cpufreq_stats.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/drivers/cpufreq/cpufreq_stats.c b/drivers/cpufreq/cpufreq_stats.c
> index 0cd9b4dcef99..81be4d637ab4 100644
> --- a/drivers/cpufreq/cpufreq_stats.c
> +++ b/drivers/cpufreq/cpufreq_stats.c
> @@ -374,8 +374,7 @@ static void __exit cpufreq_stats_exit(void)
>  }
>  
>  MODULE_AUTHOR("Zou Nan hai <nanhai.zou@intel.com>");
> -MODULE_DESCRIPTION("'cpufreq_stats' - A driver to export cpufreq stats "
> -				"through sysfs filesystem");
> +MODULE_DESCRIPTION("'cpufreq_stats' - exports cpufreq stats through sysfs filesystem");

What about 

+MODULE_DESCRIPTION("Export cpufreq stats via sysfs");

This doesn't seem to be substantially less information than the original to me.

>  MODULE_LICENSE("GPL");
>  
>  module_init(cpufreq_stats_init);
> 

-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

Re: [PATCH V2 01/14] cpufreq: stats: don't break strings into multiple lines

[Viresh Kumar]

On 4 January 2015 at 03:27, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> What about
>
> +MODULE_DESCRIPTION("Export cpufreq stats via sysfs");
>
> This doesn't seem to be substantially less information than the original to me.

Ack.

[PATCH V2 02/14] cpufreq: stats: return -EEXIST when stats are already allocated

[Viresh Kumar]

__cpufreq_stats_create_table() is called from:
- policy create notifier: stats will always be NULL here
- cpufreq_stats_init() calls it for all CPUs as cpufreq-stats can be initialized
  after cpufreq driver. Because CPUs share clock lines, 'stats' will be NULL
  here for the first cpu only and will return back for others.

While we return for other CPUs, we don't return the right error value. We must
return -EEXIST, as that is the case here.

Reviewed-by: Prarit Bhargava <prarit@redhat.com>
Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 drivers/cpufreq/cpufreq_stats.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/cpufreq/cpufreq_stats.c b/drivers/cpufreq/cpufreq_stats.c
index 81be4d637ab4..403671b1a5ee 100644
--- a/drivers/cpufreq/cpufreq_stats.c
+++ b/drivers/cpufreq/cpufreq_stats.c
@@ -192,8 +192,10 @@ static int __cpufreq_stats_create_table(struct cpufreq_policy *policy)
 	if (unlikely(!table))
 		return 0;
 
+	/* stats already initialized */
 	if (per_cpu(cpufreq_stats_table, cpu))
-		return -EBUSY;
+		return -EEXIST;
+
 	stat = kzalloc(sizeof(*stat), GFP_KERNEL);
 	if ((stat) == NULL)
 		return -ENOMEM;
-- 
2.2.0

Re: [PATCH V2 02/14] cpufreq: stats: return -EEXIST when stats are already allocated

[Rafael J. Wysocki]

On Friday, January 02, 2015 11:16:39 AM Viresh Kumar wrote:
> __cpufreq_stats_create_table() is called from:
> - policy create notifier: stats will always be NULL here
> - cpufreq_stats_init() calls it for all CPUs as cpufreq-stats can be initialized
>   after cpufreq driver. Because CPUs share clock lines, 'stats' will be NULL
>   here for the first cpu only and will return back for others.
> 
> While we return for other CPUs, we don't return the right error value. We must
> return -EEXIST, as that is the case here.

What exactly is wrong with -EBUSY, then?

> Reviewed-by: Prarit Bhargava <prarit@redhat.com>
> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
> ---
>  drivers/cpufreq/cpufreq_stats.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/cpufreq/cpufreq_stats.c b/drivers/cpufreq/cpufreq_stats.c
> index 81be4d637ab4..403671b1a5ee 100644
> --- a/drivers/cpufreq/cpufreq_stats.c
> +++ b/drivers/cpufreq/cpufreq_stats.c
> @@ -192,8 +192,10 @@ static int __cpufreq_stats_create_table(struct cpufreq_policy *policy)
>  	if (unlikely(!table))
>  		return 0;
>  
> +	/* stats already initialized */
>  	if (per_cpu(cpufreq_stats_table, cpu))
> -		return -EBUSY;
> +		return -EEXIST;
> +
>  	stat = kzalloc(sizeof(*stat), GFP_KERNEL);
>  	if ((stat) == NULL)
>  		return -ENOMEM;
> 

-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

Re: [PATCH V2 02/14] cpufreq: stats: return -EEXIST when stats are already allocated

[Viresh Kumar]

On 4 January 2015 at 03:28, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> On Friday, January 02, 2015 11:16:39 AM Viresh Kumar wrote:
>> __cpufreq_stats_create_table() is called from:
>> - policy create notifier: stats will always be NULL here
>> - cpufreq_stats_init() calls it for all CPUs as cpufreq-stats can be initialized
>>   after cpufreq driver. Because CPUs share clock lines, 'stats' will be NULL
>>   here for the first cpu only and will return back for others.
>>
>> While we return for other CPUs, we don't return the right error value. We must
>> return -EEXIST, as that is the case here.
>
> What exactly is wrong with -EBUSY, then?

Its not that could would fail with EBUSY but this is what I thought.
Generally:
- Returning EBUSY means: we are busy right not try again. And that might be
immediate.
- Returning EEXIST means: We already have what you are trying to create and
there is no need to create it again, and so no more tries are required.

That's all..

Re: [PATCH V2 02/14] cpufreq: stats: return -EEXIST when stats are already allocated

[Rafael J. Wysocki]

On Sunday, January 04, 2015 08:48:21 AM Viresh Kumar wrote:
> On 4 January 2015 at 03:28, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> > On Friday, January 02, 2015 11:16:39 AM Viresh Kumar wrote:
> >> __cpufreq_stats_create_table() is called from:
> >> - policy create notifier: stats will always be NULL here
> >> - cpufreq_stats_init() calls it for all CPUs as cpufreq-stats can be initialized
> >>   after cpufreq driver. Because CPUs share clock lines, 'stats' will be NULL
> >>   here for the first cpu only and will return back for others.
> >>
> >> While we return for other CPUs, we don't return the right error value. We must
> >> return -EEXIST, as that is the case here.
> >
> > What exactly is wrong with -EBUSY, then?
> 
> Its not that could would fail with EBUSY but this is what I thought.
> Generally:
> - Returning EBUSY means: we are busy right not try again. And that might be
> immediate.
> - Returning EEXIST means: We already have what you are trying to create and
> there is no need to create it again, and so no more tries are required.

Now note how much useful the above is than the stuff in your original changelog.

I'd say all depends on whether or not the callers deal with the error code
appropriately.  If it is propagated all the way to user space, it really should
*not* be -EBUSY, but otherwise this is a matter of how the callers handle the
given error code.


-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

Re: [PATCH V2 02/14] cpufreq: stats: return -EEXIST when stats are already allocated

[Viresh Kumar]

On 6 January 2015 at 05:22, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> Now note how much useful the above is than the stuff in your original changelog.

Yes, its all in logs now :)

> I'd say all depends on whether or not the callers deal with the error code
> appropriately.  If it is propagated all the way to user space, it really should
> *not* be -EBUSY, but otherwise this is a matter of how the callers handle the
> given error code.

It might not get propagated to user space for now, but not sure if that wouldn't
change in future. Even if it stays only within the kernel, we better return the
error codes that fit the best for a particular situation and so I
would still like
to get this through.

[PATCH V2 03/14] cpufreq: stats: don't check for freq table while freeing stats

[Viresh Kumar]

While we allocate stats, we do need to check if freq-table is present or not as
we need to use it then. But while freeing stats, all we need to know is if stats
holds a valid pointer value. There is no use of testing if cpufreq table is
present or not.

Don't check it.

Reviewed-by: Prarit Bhargava <prarit@redhat.com>
Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 drivers/cpufreq/cpufreq_stats.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/cpufreq/cpufreq_stats.c b/drivers/cpufreq/cpufreq_stats.c
index 403671b1a5ee..62b6133f06aa 100644
--- a/drivers/cpufreq/cpufreq_stats.c
+++ b/drivers/cpufreq/cpufreq_stats.c
@@ -174,8 +174,7 @@ static void cpufreq_stats_free_table(unsigned int cpu)
 	if (!policy)
 		return;
 
-	if (cpufreq_frequency_get_table(policy->cpu))
-		__cpufreq_stats_free_table(policy);
+	__cpufreq_stats_free_table(policy);
 
 	cpufreq_cpu_put(policy);
 }
-- 
2.2.0

[PATCH V2 04/14] cpufreq: stats: pass 'stat' to cpufreq_stats_update()

[Viresh Kumar]

cpufreq_stats_update() needs only 'stat' pointer for its use and nothing else.
Currently it works fine as we have a per-cpu variable for storing stats. But
later commits would remove it and hence we better pass 'stat' to
cpufreq_stats_update().

Reviewed-by: Prarit Bhargava <prarit@redhat.com>
Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 drivers/cpufreq/cpufreq_stats.c | 13 +++++--------
 1 file changed, 5 insertions(+), 8 deletions(-)

diff --git a/drivers/cpufreq/cpufreq_stats.c b/drivers/cpufreq/cpufreq_stats.c
index 62b6133f06aa..ebd9e4c5c124 100644
--- a/drivers/cpufreq/cpufreq_stats.c
+++ b/drivers/cpufreq/cpufreq_stats.c
@@ -38,14 +38,11 @@ struct cpufreq_stats_attribute {
 	ssize_t(*show) (struct cpufreq_stats *, char *);
 };
 
-static int cpufreq_stats_update(unsigned int cpu)
+static int cpufreq_stats_update(struct cpufreq_stats *stat)
 {
-	struct cpufreq_stats *stat;
-	unsigned long long cur_time;
+	unsigned long long cur_time = get_jiffies_64();
 
-	cur_time = get_jiffies_64();
 	spin_lock(&cpufreq_stats_lock);
-	stat = per_cpu(cpufreq_stats_table, cpu);
 	if (stat->time_in_state)
 		stat->time_in_state[stat->last_index] +=
 			cur_time - stat->last_time;
@@ -70,7 +67,7 @@ static ssize_t show_time_in_state(struct cpufreq_policy *policy, char *buf)
 	struct cpufreq_stats *stat = per_cpu(cpufreq_stats_table, policy->cpu);
 	if (!stat)
 		return 0;
-	cpufreq_stats_update(stat->cpu);
+	cpufreq_stats_update(stat);
 	for (i = 0; i < stat->state_num; i++) {
 		len += sprintf(buf + len, "%u %llu\n", stat->freq_table[i],
 			(unsigned long long)
@@ -88,7 +85,7 @@ static ssize_t show_trans_table(struct cpufreq_policy *policy, char *buf)
 	struct cpufreq_stats *stat = per_cpu(cpufreq_stats_table, policy->cpu);
 	if (!stat)
 		return 0;
-	cpufreq_stats_update(stat->cpu);
+	cpufreq_stats_update(stat);
 	len += snprintf(buf + len, PAGE_SIZE - len, "   From  :    To\n");
 	len += snprintf(buf + len, PAGE_SIZE - len, "         : ");
 	for (i = 0; i < stat->state_num; i++) {
@@ -313,7 +310,7 @@ static int cpufreq_stat_notifier_trans(struct notifier_block *nb,
 	if (old_index == -1 || new_index == -1)
 		return 0;
 
-	cpufreq_stats_update(freq->cpu);
+	cpufreq_stats_update(stat);
 
 	if (old_index == new_index)
 		return 0;
-- 
2.2.0

Re: [PATCH V2 04/14] cpufreq: stats: pass 'stat' to cpufreq_stats_update()

[Rafael J. Wysocki]

On Friday, January 02, 2015 11:16:41 AM Viresh Kumar wrote:
> cpufreq_stats_update() needs only 'stat' pointer for its use and nothing else.
> Currently it works fine as we have a per-cpu variable for storing stats. But
> later commits would remove it and hence we better pass 'stat' to
> cpufreq_stats_update().

What about:

"It is better to pass a struct cpufreq_stats pointer to cpufreq_stats_update()
 instead of a CPU number, because ..."

?

> Reviewed-by: Prarit Bhargava <prarit@redhat.com>
> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
> ---
>  drivers/cpufreq/cpufreq_stats.c | 13 +++++--------
>  1 file changed, 5 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/cpufreq/cpufreq_stats.c b/drivers/cpufreq/cpufreq_stats.c
> index 62b6133f06aa..ebd9e4c5c124 100644
> --- a/drivers/cpufreq/cpufreq_stats.c
> +++ b/drivers/cpufreq/cpufreq_stats.c
> @@ -38,14 +38,11 @@ struct cpufreq_stats_attribute {
>  	ssize_t(*show) (struct cpufreq_stats *, char *);
>  };
>  
> -static int cpufreq_stats_update(unsigned int cpu)
> +static int cpufreq_stats_update(struct cpufreq_stats *stat)
>  {
> -	struct cpufreq_stats *stat;
> -	unsigned long long cur_time;
> +	unsigned long long cur_time = get_jiffies_64();
>  
> -	cur_time = get_jiffies_64();

Unrelated change.

>  	spin_lock(&cpufreq_stats_lock);
> -	stat = per_cpu(cpufreq_stats_table, cpu);
>  	if (stat->time_in_state)
>  		stat->time_in_state[stat->last_index] +=
>  			cur_time - stat->last_time;
> @@ -70,7 +67,7 @@ static ssize_t show_time_in_state(struct cpufreq_policy *policy, char *buf)
>  	struct cpufreq_stats *stat = per_cpu(cpufreq_stats_table, policy->cpu);
>  	if (!stat)
>  		return 0;
> -	cpufreq_stats_update(stat->cpu);
> +	cpufreq_stats_update(stat);
>  	for (i = 0; i < stat->state_num; i++) {
>  		len += sprintf(buf + len, "%u %llu\n", stat->freq_table[i],
>  			(unsigned long long)
> @@ -88,7 +85,7 @@ static ssize_t show_trans_table(struct cpufreq_policy *policy, char *buf)
>  	struct cpufreq_stats *stat = per_cpu(cpufreq_stats_table, policy->cpu);
>  	if (!stat)
>  		return 0;
> -	cpufreq_stats_update(stat->cpu);
> +	cpufreq_stats_update(stat);
>  	len += snprintf(buf + len, PAGE_SIZE - len, "   From  :    To\n");
>  	len += snprintf(buf + len, PAGE_SIZE - len, "         : ");
>  	for (i = 0; i < stat->state_num; i++) {
> @@ -313,7 +310,7 @@ static int cpufreq_stat_notifier_trans(struct notifier_block *nb,
>  	if (old_index == -1 || new_index == -1)
>  		return 0;
>  
> -	cpufreq_stats_update(freq->cpu);
> +	cpufreq_stats_update(stat);
>  
>  	if (old_index == new_index)
>  		return 0;
> 

-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

Re: [PATCH V2 04/14] cpufreq: stats: pass 'stat' to cpufreq_stats_update()

[Viresh Kumar]

On 4 January 2015 at 03:34, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> What about:
>
> "It is better to pass a struct cpufreq_stats pointer to cpufreq_stats_update()
>  instead of a CPU number, because ..."

Sure.

>> +static int cpufreq_stats_update(struct cpufreq_stats *stat)
>>  {
>> -     struct cpufreq_stats *stat;
>> -     unsigned long long cur_time;
>> +     unsigned long long cur_time = get_jiffies_64();
>>
>> -     cur_time = get_jiffies_64();
>
> Unrelated change.

Yeah, it was too small of a change so not that significant as well.
And so folded it into this commit only :(

Will send it separately in v3..

Re: [PATCH V2 04/14] cpufreq: stats: pass 'stat' to cpufreq_stats_update() *

[Viresh Kumar]

On 5 January 2015 at 12:44, Luigi Fioretti <luigi.fioretti@dinema.it> wrote:
> Hi,
> thanks for your help but I don't think to be solution.
> I had apply this patch :
>
> http://lists.infradead.org/pipermail/linux-arm-kernel/2010-December/036055.html
>
> You can read the first problems here:
> http://lists.linaro.org/pipermail/linaro-kernel/2014-March/012635.html
>
> I had found a solution to this problem, and I write to you into my first
> e-mail.
> Now I have this problems:
> When resume  and run this function

What does all this has to do with the mail below ?

> -----linaro-kernel-bounces@lists.linaro.org ha scritto: -----
> Per: "Rafael J. Wysocki" <rjw@rjwysocki.net>
> Da: Viresh Kumar
> Inviato da: linaro-kernel-bounces@lists.linaro.org
> Data: 05/01/2015 05.05AM
> Cc: Prarit Bhargava <prarit@redhat.com>, Lists linaro-kernel
> <linaro-kernel@lists.linaro.org>, Saravana Kannan <skannan@codeaurora.org>,
> "linux-pm@vger.kernel.org" <linux-pm@vger.kernel.org>
> Oggetto: Re: [PATCH V2 04/14] cpufreq: stats: pass 'stat' to
> cpufreq_stats_update() *
>
> On 4 January 2015 at 03:34, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
>> What about:
>>
>> "It is better to pass a struct cpufreq_stats pointer to
>> cpufreq_stats_update()
>>  instead of a CPU number, because ..."
>
> Sure.
>
>>> +static int cpufreq_stats_update(struct cpufreq_stats *stat)
>>>  {
>>> -     struct cpufreq_stats *stat;
>>> -     unsigned long long cur_time;
>>> +     unsigned long long cur_time = get_jiffies_64();
>>>
>>> -     cur_time = get_jiffies_64();
>>
>> Unrelated change.
>
> Yeah, it was too small of a change so not that significant as well.
> And so folded it into this commit only :(
>
> Will send it separately in v3..
>
> _______________________________________________
> linaro-kernel mailing list
> linaro-kernel@lists.linaro.org
> http://lists.linaro.org/mailman/listinfo/linaro-kernel

[PATCH V2 05/14] cpufreq: stats: get rid of per-cpu cpufreq_stats_table

[Viresh Kumar]

Because all CPUs in a policy share clock line, they share stats as well.  But
cpufreq-stats maintains a per-cpu list of tables for managing this. Get rid of
it by assigning a special field for stats in 'struct cpufreq_policy'.

Reviewed-by: Prarit Bhargava <prarit@redhat.com>
Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 drivers/cpufreq/cpufreq_stats.c | 51 ++++++++++++++++++-----------------------
 include/linux/cpufreq.h         |  3 +++
 2 files changed, 25 insertions(+), 29 deletions(-)

diff --git a/drivers/cpufreq/cpufreq_stats.c b/drivers/cpufreq/cpufreq_stats.c
index ebd9e4c5c124..106c89ccef30 100644
--- a/drivers/cpufreq/cpufreq_stats.c
+++ b/drivers/cpufreq/cpufreq_stats.c
@@ -31,8 +31,6 @@ struct cpufreq_stats {
 #endif
 };
 
-static DEFINE_PER_CPU(struct cpufreq_stats *, cpufreq_stats_table);
-
 struct cpufreq_stats_attribute {
 	struct attribute attr;
 	ssize_t(*show) (struct cpufreq_stats *, char *);
@@ -53,20 +51,17 @@ static int cpufreq_stats_update(struct cpufreq_stats *stat)
 
 static ssize_t show_total_trans(struct cpufreq_policy *policy, char *buf)
 {
-	struct cpufreq_stats *stat = per_cpu(cpufreq_stats_table, policy->cpu);
-	if (!stat)
-		return 0;
-	return sprintf(buf, "%d\n",
-			per_cpu(cpufreq_stats_table, stat->cpu)->total_trans);
+	struct cpufreq_stats *stat = policy->stats_data;
+
+	return sprintf(buf, "%d\n", stat->total_trans);
 }
 
 static ssize_t show_time_in_state(struct cpufreq_policy *policy, char *buf)
 {
+	struct cpufreq_stats *stat = policy->stats_data;
 	ssize_t len = 0;
 	int i;
-	struct cpufreq_stats *stat = per_cpu(cpufreq_stats_table, policy->cpu);
-	if (!stat)
-		return 0;
+
 	cpufreq_stats_update(stat);
 	for (i = 0; i < stat->state_num; i++) {
 		len += sprintf(buf + len, "%u %llu\n", stat->freq_table[i],
@@ -79,12 +74,10 @@ static ssize_t show_time_in_state(struct cpufreq_policy *policy, char *buf)
 #ifdef CONFIG_CPU_FREQ_STAT_DETAILS
 static ssize_t show_trans_table(struct cpufreq_policy *policy, char *buf)
 {
+	struct cpufreq_stats *stat = policy->stats_data;
 	ssize_t len = 0;
 	int i, j;
 
-	struct cpufreq_stats *stat = per_cpu(cpufreq_stats_table, policy->cpu);
-	if (!stat)
-		return 0;
 	cpufreq_stats_update(stat);
 	len += snprintf(buf + len, PAGE_SIZE - len, "   From  :    To\n");
 	len += snprintf(buf + len, PAGE_SIZE - len, "         : ");
@@ -150,8 +143,9 @@ static int freq_table_get_index(struct cpufreq_stats *stat, unsigned int freq)
 
 static void __cpufreq_stats_free_table(struct cpufreq_policy *policy)
 {
-	struct cpufreq_stats *stat = per_cpu(cpufreq_stats_table, policy->cpu);
+	struct cpufreq_stats *stat = policy->stats_data;
 
+	/* Already freed */
 	if (!stat)
 		return;
 
@@ -160,7 +154,7 @@ static void __cpufreq_stats_free_table(struct cpufreq_policy *policy)
 	sysfs_remove_group(&policy->kobj, &stats_attr_group);
 	kfree(stat->time_in_state);
 	kfree(stat);
-	per_cpu(cpufreq_stats_table, policy->cpu) = NULL;
+	policy->stats_data = NULL;
 }
 
 static void cpufreq_stats_free_table(unsigned int cpu)
@@ -189,7 +183,7 @@ static int __cpufreq_stats_create_table(struct cpufreq_policy *policy)
 		return 0;
 
 	/* stats already initialized */
-	if (per_cpu(cpufreq_stats_table, cpu))
+	if (policy->stats_data)
 		return -EEXIST;
 
 	stat = kzalloc(sizeof(*stat), GFP_KERNEL);
@@ -201,7 +195,7 @@ static int __cpufreq_stats_create_table(struct cpufreq_policy *policy)
 		goto error_out;
 
 	stat->cpu = cpu;
-	per_cpu(cpufreq_stats_table, cpu) = stat;
+	policy->stats_data = stat;
 
 	cpufreq_for_each_valid_entry(pos, table)
 		count++;
@@ -236,7 +230,7 @@ static int __cpufreq_stats_create_table(struct cpufreq_policy *policy)
 	sysfs_remove_group(&policy->kobj, &stats_attr_group);
 error_out:
 	kfree(stat);
-	per_cpu(cpufreq_stats_table, cpu) = NULL;
+	policy->stats_data = NULL;
 	return ret;
 }
 
@@ -259,14 +253,8 @@ static void cpufreq_stats_create_table(unsigned int cpu)
 
 static void cpufreq_stats_update_policy_cpu(struct cpufreq_policy *policy)
 {
-	struct cpufreq_stats *stat = per_cpu(cpufreq_stats_table,
-			policy->last_cpu);
-
-	pr_debug("Updating stats_table for new_cpu %u from last_cpu %u\n",
-			policy->cpu, policy->last_cpu);
-	per_cpu(cpufreq_stats_table, policy->cpu) = per_cpu(cpufreq_stats_table,
-			policy->last_cpu);
-	per_cpu(cpufreq_stats_table, policy->last_cpu) = NULL;
+	struct cpufreq_stats *stat = policy->stats_data;
+
 	stat->cpu = policy->cpu;
 }
 
@@ -293,15 +281,19 @@ static int cpufreq_stat_notifier_trans(struct notifier_block *nb,
 		unsigned long val, void *data)
 {
 	struct cpufreq_freqs *freq = data;
+	struct cpufreq_policy *policy = cpufreq_cpu_get(freq->cpu);
 	struct cpufreq_stats *stat;
 	int old_index, new_index;
 
 	if (val != CPUFREQ_POSTCHANGE)
 		return 0;
 
-	stat = per_cpu(cpufreq_stats_table, freq->cpu);
-	if (!stat)
-		return 0;
+	if (!policy) {
+		pr_err("%s: No policy found\n", __func__);
+		return -ENODEV;
+	}
+
+	stat = policy->stats_data;
 
 	old_index = stat->last_index;
 	new_index = freq_table_get_index(stat, freq->new);
@@ -322,6 +314,7 @@ static int cpufreq_stat_notifier_trans(struct notifier_block *nb,
 #endif
 	stat->total_trans++;
 	spin_unlock(&cpufreq_stats_lock);
+	cpufreq_cpu_put(policy);
 	return 0;
 }
 
diff --git a/include/linux/cpufreq.h b/include/linux/cpufreq.h
index 4d078cebafd2..3cb097b86d20 100644
--- a/include/linux/cpufreq.h
+++ b/include/linux/cpufreq.h
@@ -113,6 +113,9 @@ struct cpufreq_policy {
 	wait_queue_head_t	transition_wait;
 	struct task_struct	*transition_task; /* Task which is doing the transition */
 
+	/* cpufreq-stats data */
+	void			*stats_data;
+
 	/* For cpufreq driver's internal use */
 	void			*driver_data;
 };
-- 
2.2.0

Re: [PATCH V2 05/14] cpufreq: stats: get rid of per-cpu cpufreq_stats_table

[Rafael J. Wysocki]

On Friday, January 02, 2015 11:16:42 AM Viresh Kumar wrote:
> Because all CPUs in a policy share clock line, they share stats as well.

"All CPUs sharing a cpufreq policy share stats too."

Sharing a clock line is irrelevant here.

> But cpufreq-stats maintains a per-cpu list of tables for managing this. Get rid of
> it by assigning a special field for stats in 'struct cpufreq_policy'.

"For this reason, add a stats pointer to struct cpufreq_policy and drop
 per-CPU variable cpufreq_stats_table used for accessing cpufreq stats
 so as to reduce code complexity."

> 
> Reviewed-by: Prarit Bhargava <prarit@redhat.com>
> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
> ---
>  drivers/cpufreq/cpufreq_stats.c | 51 ++++++++++++++++++-----------------------
>  include/linux/cpufreq.h         |  3 +++
>  2 files changed, 25 insertions(+), 29 deletions(-)
> 
> diff --git a/drivers/cpufreq/cpufreq_stats.c b/drivers/cpufreq/cpufreq_stats.c
> index ebd9e4c5c124..106c89ccef30 100644
> --- a/drivers/cpufreq/cpufreq_stats.c
> +++ b/drivers/cpufreq/cpufreq_stats.c
> @@ -31,8 +31,6 @@ struct cpufreq_stats {
>  #endif
>  };
>  
> -static DEFINE_PER_CPU(struct cpufreq_stats *, cpufreq_stats_table);
> -
>  struct cpufreq_stats_attribute {
>  	struct attribute attr;
>  	ssize_t(*show) (struct cpufreq_stats *, char *);
> @@ -53,20 +51,17 @@ static int cpufreq_stats_update(struct cpufreq_stats *stat)
>  
>  static ssize_t show_total_trans(struct cpufreq_policy *policy, char *buf)
>  {
> -	struct cpufreq_stats *stat = per_cpu(cpufreq_stats_table, policy->cpu);
> -	if (!stat)
> -		return 0;
> -	return sprintf(buf, "%d\n",
> -			per_cpu(cpufreq_stats_table, stat->cpu)->total_trans);
> +	struct cpufreq_stats *stat = policy->stats_data;
> +
> +	return sprintf(buf, "%d\n", stat->total_trans);

First off, why do you call the new field stats_data instead of stats?
Moreover:

+	return sprintf(buf, "%d\n", policy->stats->total_trans);

and then you don't need the extra line above.

>  }
>  
>  static ssize_t show_time_in_state(struct cpufreq_policy *policy, char *buf)
>  {
> +	struct cpufreq_stats *stat = policy->stats_data;
>  	ssize_t len = 0;
>  	int i;
> -	struct cpufreq_stats *stat = per_cpu(cpufreq_stats_table, policy->cpu);
> -	if (!stat)
> -		return 0;
> +
>  	cpufreq_stats_update(stat);
>  	for (i = 0; i < stat->state_num; i++) {
>  		len += sprintf(buf + len, "%u %llu\n", stat->freq_table[i],
> @@ -79,12 +74,10 @@ static ssize_t show_time_in_state(struct cpufreq_policy *policy, char *buf)
>  #ifdef CONFIG_CPU_FREQ_STAT_DETAILS
>  static ssize_t show_trans_table(struct cpufreq_policy *policy, char *buf)
>  {
> +	struct cpufreq_stats *stat = policy->stats_data;
>  	ssize_t len = 0;
>  	int i, j;
>  
> -	struct cpufreq_stats *stat = per_cpu(cpufreq_stats_table, policy->cpu);
> -	if (!stat)
> -		return 0;
>  	cpufreq_stats_update(stat);
>  	len += snprintf(buf + len, PAGE_SIZE - len, "   From  :    To\n");
>  	len += snprintf(buf + len, PAGE_SIZE - len, "         : ");
> @@ -150,8 +143,9 @@ static int freq_table_get_index(struct cpufreq_stats *stat, unsigned int freq)
>  
>  static void __cpufreq_stats_free_table(struct cpufreq_policy *policy)
>  {
> -	struct cpufreq_stats *stat = per_cpu(cpufreq_stats_table, policy->cpu);
> +	struct cpufreq_stats *stat = policy->stats_data;
>  
> +	/* Already freed */
>  	if (!stat)
>  		return;
>  
> @@ -160,7 +154,7 @@ static void __cpufreq_stats_free_table(struct cpufreq_policy *policy)
>  	sysfs_remove_group(&policy->kobj, &stats_attr_group);
>  	kfree(stat->time_in_state);
>  	kfree(stat);
> -	per_cpu(cpufreq_stats_table, policy->cpu) = NULL;
> +	policy->stats_data = NULL;
>  }
>  
>  static void cpufreq_stats_free_table(unsigned int cpu)
> @@ -189,7 +183,7 @@ static int __cpufreq_stats_create_table(struct cpufreq_policy *policy)
>  		return 0;
>  
>  	/* stats already initialized */
> -	if (per_cpu(cpufreq_stats_table, cpu))
> +	if (policy->stats_data)
>  		return -EEXIST;
>  
>  	stat = kzalloc(sizeof(*stat), GFP_KERNEL);
> @@ -201,7 +195,7 @@ static int __cpufreq_stats_create_table(struct cpufreq_policy *policy)
>  		goto error_out;
>  
>  	stat->cpu = cpu;
> -	per_cpu(cpufreq_stats_table, cpu) = stat;
> +	policy->stats_data = stat;
>  
>  	cpufreq_for_each_valid_entry(pos, table)
>  		count++;
> @@ -236,7 +230,7 @@ static int __cpufreq_stats_create_table(struct cpufreq_policy *policy)
>  	sysfs_remove_group(&policy->kobj, &stats_attr_group);
>  error_out:
>  	kfree(stat);
> -	per_cpu(cpufreq_stats_table, cpu) = NULL;
> +	policy->stats_data = NULL;
>  	return ret;
>  }
>  
> @@ -259,14 +253,8 @@ static void cpufreq_stats_create_table(unsigned int cpu)
>  
>  static void cpufreq_stats_update_policy_cpu(struct cpufreq_policy *policy)
>  {
> -	struct cpufreq_stats *stat = per_cpu(cpufreq_stats_table,
> -			policy->last_cpu);
> -
> -	pr_debug("Updating stats_table for new_cpu %u from last_cpu %u\n",
> -			policy->cpu, policy->last_cpu);
> -	per_cpu(cpufreq_stats_table, policy->cpu) = per_cpu(cpufreq_stats_table,
> -			policy->last_cpu);
> -	per_cpu(cpufreq_stats_table, policy->last_cpu) = NULL;
> +	struct cpufreq_stats *stat = policy->stats_data;
> +
>  	stat->cpu = policy->cpu;

Why don't you change this line to

	policy->stats->cpu = policy->cpu;

and then you don't need the extra variable any more?

>  }
>  
> @@ -293,15 +281,19 @@ static int cpufreq_stat_notifier_trans(struct notifier_block *nb,
>  		unsigned long val, void *data)
>  {
>  	struct cpufreq_freqs *freq = data;
> +	struct cpufreq_policy *policy = cpufreq_cpu_get(freq->cpu);
>  	struct cpufreq_stats *stat;
>  	int old_index, new_index;
>  
>  	if (val != CPUFREQ_POSTCHANGE)
>  		return 0;
>  
> -	stat = per_cpu(cpufreq_stats_table, freq->cpu);
> -	if (!stat)
> -		return 0;
> +	if (!policy) {
> +		pr_err("%s: No policy found\n", __func__);

Do we need the extra message here?  If so, can it be pr_debug()?

> +		return -ENODEV;
> +	}

This changes the behavior in a subtle way.  Instead of returning 0 when stats
are not present, we'll now return an error code for a missing policy.  Are you
sure that this isn't going to break anything?

> +
> +	stat = policy->stats_data;

Is stat guaraneed to be a valid pointer at this point?

>  
>  	old_index = stat->last_index;
>  	new_index = freq_table_get_index(stat, freq->new);
> @@ -322,6 +314,7 @@ static int cpufreq_stat_notifier_trans(struct notifier_block *nb,
>  #endif
>  	stat->total_trans++;
>  	spin_unlock(&cpufreq_stats_lock);
> +	cpufreq_cpu_put(policy);
>  	return 0;
>  }
>  
> diff --git a/include/linux/cpufreq.h b/include/linux/cpufreq.h
> index 4d078cebafd2..3cb097b86d20 100644
> --- a/include/linux/cpufreq.h
> +++ b/include/linux/cpufreq.h
> @@ -113,6 +113,9 @@ struct cpufreq_policy {
>  	wait_queue_head_t	transition_wait;
>  	struct task_struct	*transition_task; /* Task which is doing the transition */
>  
> +	/* cpufreq-stats data */
> +	void			*stats_data;

Why isn't that

	struct cpufreq_stats	*stats;

?

> +
>  	/* For cpufreq driver's internal use */
>  	void			*driver_data;
>  };
> 

-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

Re: [PATCH V2 05/14] cpufreq: stats: get rid of per-cpu cpufreq_stats_table

[Viresh Kumar]

On 4 January 2015 at 03:50, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:

All comments accepted before this ..

>> @@ -293,15 +281,19 @@ static int cpufreq_stat_notifier_trans(struct notifier_block *nb,
>>               unsigned long val, void *data)
>>  {
>>       struct cpufreq_freqs *freq = data;
>> +     struct cpufreq_policy *policy = cpufreq_cpu_get(freq->cpu);
>>       struct cpufreq_stats *stat;
>>       int old_index, new_index;
>>
>>       if (val != CPUFREQ_POSTCHANGE)
>>               return 0;
>>
>> -     stat = per_cpu(cpufreq_stats_table, freq->cpu);
>> -     if (!stat)
>> -             return 0;
>> +     if (!policy) {
>> +             pr_err("%s: No policy found\n", __func__);
>
> Do we need the extra message here?  If so, can it be pr_debug()?

Because we are in the POSTCHANGE stage right now, the policy
isn't allowed to be NULL. And so this is an error and I used pr_err.

>> +             return -ENODEV;
>> +     }
>
> This changes the behavior in a subtle way.  Instead of returning 0 when stats
> are not present, we'll now return an error code for a missing policy.  Are you
> sure that this isn't going to break anything?

Something I didn't looked at earlier. This *can* break things a bit.
If I understand it clearly now, the notifiers aren't allowed to return these
errors. They should return:

NOTIFY_DONE or NOTIFY_OK or NOTIFY_BAD

And this is what the notifier chain does to the return value:

    if ((ret & NOTIFY_STOP_MASK) == NOTIFY_STOP_MASK)
            break;

So, if the return error has its 15th bit set, then we will break the loop.
(NOTIFY_STOP_MASK 0x8000).

It wouldn't happen in this case, but probably we should just return
NOTIFY_DONE (0)..

Right ?

>> +
>> +     stat = policy->stats_data;
>
> Is stat guaraneed to be a valid pointer at this point?

We better check it as well for some corner cases where we failed
to create a table.

>> +     void                    *stats_data;
>
> Why isn't that
>
>         struct cpufreq_stats    *stats;

It will be.

Re: [PATCH V2 05/14] cpufreq: stats: get rid of per-cpu cpufreq_stats_table

[Rafael J. Wysocki]

On Monday, January 05, 2015 09:53:45 AM Viresh Kumar wrote:
> On 4 January 2015 at 03:50, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> 
> All comments accepted before this ..
> 
> >> @@ -293,15 +281,19 @@ static int cpufreq_stat_notifier_trans(struct notifier_block *nb,
> >>               unsigned long val, void *data)
> >>  {
> >>       struct cpufreq_freqs *freq = data;
> >> +     struct cpufreq_policy *policy = cpufreq_cpu_get(freq->cpu);
> >>       struct cpufreq_stats *stat;
> >>       int old_index, new_index;
> >>
> >>       if (val != CPUFREQ_POSTCHANGE)
> >>               return 0;
> >>
> >> -     stat = per_cpu(cpufreq_stats_table, freq->cpu);
> >> -     if (!stat)
> >> -             return 0;
> >> +     if (!policy) {
> >> +             pr_err("%s: No policy found\n", __func__);
> >
> > Do we need the extra message here?  If so, can it be pr_debug()?
> 
> Because we are in the POSTCHANGE stage right now, the policy
> isn't allowed to be NULL. And so this is an error and I used pr_err.
> 
> >> +             return -ENODEV;
> >> +     }
> >
> > This changes the behavior in a subtle way.  Instead of returning 0 when stats
> > are not present, we'll now return an error code for a missing policy.  Are you
> > sure that this isn't going to break anything?
> 
> Something I didn't looked at earlier. This *can* break things a bit.
> If I understand it clearly now, the notifiers aren't allowed to return these
> errors. They should return:
> 
> NOTIFY_DONE or NOTIFY_OK or NOTIFY_BAD
> 
> And this is what the notifier chain does to the return value:
> 
>     if ((ret & NOTIFY_STOP_MASK) == NOTIFY_STOP_MASK)
>             break;
> 
> So, if the return error has its 15th bit set, then we will break the loop.
> (NOTIFY_STOP_MASK 0x8000).
> 
> It wouldn't happen in this case, but probably we should just return
> NOTIFY_DONE (0)..
> 
> Right ?

Looks correct to me.

> >> +
> >> +     stat = policy->stats_data;
> >
> > Is stat guaraneed to be a valid pointer at this point?
> 
> We better check it as well for some corner cases where we failed
> to create a table.

Exactly. :-)


-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

[PATCH V2 06/14] cpufreq: Remove (now) unused 'last_cpu' from struct cpufreq_policy

[Viresh Kumar]

'last_cpu' was used only from cpufreq-stats and isn't used anymore. Get rid of
it.

Reviewed-by: Prarit Bhargava <prarit@redhat.com>
Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 drivers/cpufreq/cpufreq.c | 3 ---
 include/linux/cpufreq.h   | 2 --
 2 files changed, 5 deletions(-)

diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
index a09a29c312a9..b88894578fd1 100644
--- a/drivers/cpufreq/cpufreq.c
+++ b/drivers/cpufreq/cpufreq.c
@@ -1091,10 +1091,7 @@ static int update_policy_cpu(struct cpufreq_policy *policy, unsigned int cpu,
 	}
 
 	down_write(&policy->rwsem);
-
-	policy->last_cpu = policy->cpu;
 	policy->cpu = cpu;
-
 	up_write(&policy->rwsem);
 
 	blocking_notifier_call_chain(&cpufreq_policy_notifier_list,
diff --git a/include/linux/cpufreq.h b/include/linux/cpufreq.h
index 3cb097b86d20..2d99f12e4199 100644
--- a/include/linux/cpufreq.h
+++ b/include/linux/cpufreq.h
@@ -66,8 +66,6 @@ struct cpufreq_policy {
 	unsigned int		shared_type; /* ACPI: ANY or ALL affected CPUs
 						should set cpufreq */
 	unsigned int		cpu;    /* cpu nr of CPU managing this policy */
-	unsigned int		last_cpu; /* cpu nr of previous CPU that managed
-					   * this policy */
 	struct clk		*clk;
 	struct cpufreq_cpuinfo	cpuinfo;/* see above */
 
-- 
2.2.0

[PATCH V2 07/14] cpufreq: stats: remove cpufreq_stats_update_policy_cpu()

[Viresh Kumar]

cpufreq_stats_update_policy_cpu() doesn't do any useful stuff anymore and must
be removed. Along with it, also remove 'cpu' field from 'struct cpufreq_stats'
as that isn't used as well.

Reviewed-by: Prarit Bhargava <prarit@redhat.com>
Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 drivers/cpufreq/cpufreq_stats.c | 14 --------------
 1 file changed, 14 deletions(-)

diff --git a/drivers/cpufreq/cpufreq_stats.c b/drivers/cpufreq/cpufreq_stats.c
index 106c89ccef30..f458fdbc5bfd 100644
--- a/drivers/cpufreq/cpufreq_stats.c
+++ b/drivers/cpufreq/cpufreq_stats.c
@@ -18,7 +18,6 @@
 static spinlock_t cpufreq_stats_lock;
 
 struct cpufreq_stats {
-	unsigned int cpu;
 	unsigned int total_trans;
 	unsigned long long last_time;
 	unsigned int max_state;
@@ -194,7 +193,6 @@ static int __cpufreq_stats_create_table(struct cpufreq_policy *policy)
 	if (ret)
 		goto error_out;
 
-	stat->cpu = cpu;
 	policy->stats_data = stat;
 
 	cpufreq_for_each_valid_entry(pos, table)
@@ -251,24 +249,12 @@ static void cpufreq_stats_create_table(unsigned int cpu)
 	cpufreq_cpu_put(policy);
 }
 
-static void cpufreq_stats_update_policy_cpu(struct cpufreq_policy *policy)
-{
-	struct cpufreq_stats *stat = policy->stats_data;
-
-	stat->cpu = policy->cpu;
-}
-
 static int cpufreq_stat_notifier_policy(struct notifier_block *nb,
 		unsigned long val, void *data)
 {
 	int ret = 0;
 	struct cpufreq_policy *policy = data;
 
-	if (val == CPUFREQ_UPDATE_POLICY_CPU) {
-		cpufreq_stats_update_policy_cpu(policy);
-		return 0;
-	}
-
 	if (val == CPUFREQ_CREATE_POLICY)
 		ret = __cpufreq_stats_create_table(policy);
 	else if (val == CPUFREQ_REMOVE_POLICY)
-- 
2.2.0

Re: [PATCH V2 07/14] cpufreq: stats: remove cpufreq_stats_update_policy_cpu()

[Rafael J. Wysocki]

On Friday, January 02, 2015 11:16:44 AM Viresh Kumar wrote:
> cpufreq_stats_update_policy_cpu() doesn't do any useful stuff anymore and must
> be removed.

Well, "can be" sounds more appropriate than "must be".  It wouldn't hurt to
leave it as is, after all.

> Along with it, also remove 'cpu' field from 'struct cpufreq_stats'
> as that isn't used as well.

It's the reverse.  Because the cpu field is not necessary any more, it can
be dropped *along* *with* cpufreq_stats_update_policy_cpu().

Modifying data structures is more important than dropping a trivial function
and it should be described this way in the changelog.


> Reviewed-by: Prarit Bhargava <prarit@redhat.com>
> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
> ---
>  drivers/cpufreq/cpufreq_stats.c | 14 --------------
>  1 file changed, 14 deletions(-)
> 
> diff --git a/drivers/cpufreq/cpufreq_stats.c b/drivers/cpufreq/cpufreq_stats.c
> index 106c89ccef30..f458fdbc5bfd 100644
> --- a/drivers/cpufreq/cpufreq_stats.c
> +++ b/drivers/cpufreq/cpufreq_stats.c
> @@ -18,7 +18,6 @@
>  static spinlock_t cpufreq_stats_lock;
>  
>  struct cpufreq_stats {
> -	unsigned int cpu;
>  	unsigned int total_trans;
>  	unsigned long long last_time;
>  	unsigned int max_state;
> @@ -194,7 +193,6 @@ static int __cpufreq_stats_create_table(struct cpufreq_policy *policy)
>  	if (ret)
>  		goto error_out;
>  
> -	stat->cpu = cpu;
>  	policy->stats_data = stat;
>  
>  	cpufreq_for_each_valid_entry(pos, table)
> @@ -251,24 +249,12 @@ static void cpufreq_stats_create_table(unsigned int cpu)
>  	cpufreq_cpu_put(policy);
>  }
>  
> -static void cpufreq_stats_update_policy_cpu(struct cpufreq_policy *policy)
> -{
> -	struct cpufreq_stats *stat = policy->stats_data;
> -
> -	stat->cpu = policy->cpu;

Well, if you wrote it the way I suggested in the previous patch, it would be
more obvious ...

> -}
> -
>  static int cpufreq_stat_notifier_policy(struct notifier_block *nb,
>  		unsigned long val, void *data)
>  {
>  	int ret = 0;
>  	struct cpufreq_policy *policy = data;
>  
> -	if (val == CPUFREQ_UPDATE_POLICY_CPU) {
> -		cpufreq_stats_update_policy_cpu(policy);
> -		return 0;
> -	}
> -
>  	if (val == CPUFREQ_CREATE_POLICY)
>  		ret = __cpufreq_stats_create_table(policy);
>  	else if (val == CPUFREQ_REMOVE_POLICY)
> 

-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

Re: [PATCH V2 07/14] cpufreq: stats: remove cpufreq_stats_update_policy_cpu()

[Viresh Kumar]

On 4 January 2015 at 03:55, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> Well, "can be" sounds more appropriate than "must be".  It wouldn't hurt to
> leave it as is, after all.

> It's the reverse.  Because the cpu field is not necessary any more, it can
> be dropped *along* *with* cpufreq_stats_update_policy_cpu().
>
> Modifying data structures is more important than dropping a trivial function
> and it should be described this way in the changelog.

> Well, if you wrote it the way I suggested in the previous patch, it would be
> more obvious ...

Ack.

[PATCH V2 08/14] cpufreq: remove CPUFREQ_UPDATE_POLICY_CPU notifications

[Viresh Kumar]

CPUFREQ_UPDATE_POLICY_CPU notifications were used only from cpufreq-stats which
doesn't use it anymore. Remove them.

This also fixes the value of other notification macros, hopefully all users are
using macro's instead of direct values and nothing will break :)

Reviewed-by: Prarit Bhargava <prarit@redhat.com>
Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 drivers/cpufreq/cpufreq.c | 3 ---
 include/linux/cpufreq.h   | 5 ++---
 2 files changed, 2 insertions(+), 6 deletions(-)

diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
index b88894578fd1..6d97dffeaaf7 100644
--- a/drivers/cpufreq/cpufreq.c
+++ b/drivers/cpufreq/cpufreq.c
@@ -1094,9 +1094,6 @@ static int update_policy_cpu(struct cpufreq_policy *policy, unsigned int cpu,
 	policy->cpu = cpu;
 	up_write(&policy->rwsem);
 
-	blocking_notifier_call_chain(&cpufreq_policy_notifier_list,
-			CPUFREQ_UPDATE_POLICY_CPU, policy);
-
 	return 0;
 }
 
diff --git a/include/linux/cpufreq.h b/include/linux/cpufreq.h
index 2d99f12e4199..43013eac26fd 100644
--- a/include/linux/cpufreq.h
+++ b/include/linux/cpufreq.h
@@ -368,9 +368,8 @@ static inline void cpufreq_resume(void) {}
 #define CPUFREQ_INCOMPATIBLE		(1)
 #define CPUFREQ_NOTIFY			(2)
 #define CPUFREQ_START			(3)
-#define CPUFREQ_UPDATE_POLICY_CPU	(4)
-#define CPUFREQ_CREATE_POLICY		(5)
-#define CPUFREQ_REMOVE_POLICY		(6)
+#define CPUFREQ_CREATE_POLICY		(4)
+#define CPUFREQ_REMOVE_POLICY		(5)
 
 #ifdef CONFIG_CPU_FREQ
 int cpufreq_register_notifier(struct notifier_block *nb, unsigned int list);
-- 
2.2.0

Re: [PATCH V2 08/14] cpufreq: remove CPUFREQ_UPDATE_POLICY_CPU notifications

[Rafael J. Wysocki]

On Friday, January 02, 2015 11:16:45 AM Viresh Kumar wrote:
> CPUFREQ_UPDATE_POLICY_CPU notifications were used only from cpufreq-stats which
> doesn't use it anymore. Remove them.
> 
> This also fixes the value of other notification macros, hopefully all users are
> using macro's instead of direct values and nothing will break :)

The paragraph above is completely unclear to me.  Do you mean that it changes
the definitions of CPUFREQ_CREATE_POLICY and CPUFREQ_REMOVE_POLICY too which
may affect code that uses numbers instead of these symbols?

> Reviewed-by: Prarit Bhargava <prarit@redhat.com>
> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
> ---
>  drivers/cpufreq/cpufreq.c | 3 ---
>  include/linux/cpufreq.h   | 5 ++---
>  2 files changed, 2 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
> index b88894578fd1..6d97dffeaaf7 100644
> --- a/drivers/cpufreq/cpufreq.c
> +++ b/drivers/cpufreq/cpufreq.c
> @@ -1094,9 +1094,6 @@ static int update_policy_cpu(struct cpufreq_policy *policy, unsigned int cpu,
>  	policy->cpu = cpu;
>  	up_write(&policy->rwsem);
>  
> -	blocking_notifier_call_chain(&cpufreq_policy_notifier_list,
> -			CPUFREQ_UPDATE_POLICY_CPU, policy);
> -
>  	return 0;
>  }
>  
> diff --git a/include/linux/cpufreq.h b/include/linux/cpufreq.h
> index 2d99f12e4199..43013eac26fd 100644
> --- a/include/linux/cpufreq.h
> +++ b/include/linux/cpufreq.h
> @@ -368,9 +368,8 @@ static inline void cpufreq_resume(void) {}
>  #define CPUFREQ_INCOMPATIBLE		(1)
>  #define CPUFREQ_NOTIFY			(2)
>  #define CPUFREQ_START			(3)
> -#define CPUFREQ_UPDATE_POLICY_CPU	(4)
> -#define CPUFREQ_CREATE_POLICY		(5)
> -#define CPUFREQ_REMOVE_POLICY		(6)
> +#define CPUFREQ_CREATE_POLICY		(4)
> +#define CPUFREQ_REMOVE_POLICY		(5)
>  
>  #ifdef CONFIG_CPU_FREQ
>  int cpufreq_register_notifier(struct notifier_block *nb, unsigned int list);
> 

-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

Re: [PATCH V2 08/14] cpufreq: remove CPUFREQ_UPDATE_POLICY_CPU notifications

[Viresh Kumar]

On 4 January 2015 at 03:58, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> On Friday, January 02, 2015 11:16:45 AM Viresh Kumar wrote:
>> CPUFREQ_UPDATE_POLICY_CPU notifications were used only from cpufreq-stats which
>> doesn't use it anymore. Remove them.
>>
>> This also fixes the value of other notification macros, hopefully all users are
>> using macro's instead of direct values and nothing will break :)
>
> The paragraph above is completely unclear to me.  Do you mean that it changes
> the definitions of CPUFREQ_CREATE_POLICY and CPUFREQ_REMOVE_POLICY too which
> may affect code that uses numbers instead of these symbols?

Yeah. I will try to rewrite it a bit..

[PATCH V2 09/14] cpufreq: stats: create sysfs group once we are ready

[Viresh Kumar]

Userspace is free to read value of any file from cpufreq/stats directory once
they are created. __cpufreq_stats_create_table() is creating the sysfs files
first and then allocating resources for them. Though it would be quite difficult
to trigger the racy situation here, but for the sake of keeping sensible code
lets create sysfs entries only after we are ready to go.

This also does some makeup to the routine to make it look better.

Reviewed-by: Prarit Bhargava <prarit@redhat.com>
Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 drivers/cpufreq/cpufreq_stats.c | 44 +++++++++++++++++++++++------------------
 1 file changed, 25 insertions(+), 19 deletions(-)

diff --git a/drivers/cpufreq/cpufreq_stats.c b/drivers/cpufreq/cpufreq_stats.c
index f458fdbc5bfd..dd55d57655ce 100644
--- a/drivers/cpufreq/cpufreq_stats.c
+++ b/drivers/cpufreq/cpufreq_stats.c
@@ -171,12 +171,13 @@ static void cpufreq_stats_free_table(unsigned int cpu)
 
 static int __cpufreq_stats_create_table(struct cpufreq_policy *policy)
 {
-	unsigned int i, count = 0, ret = 0;
+	unsigned int i = 0, count = 0, ret = -ENOMEM;
 	struct cpufreq_stats *stat;
 	unsigned int alloc_size;
 	unsigned int cpu = policy->cpu;
 	struct cpufreq_frequency_table *pos, *table;
 
+	/* We need cpufreq table for creating stats table */
 	table = cpufreq_frequency_get_table(cpu);
 	if (unlikely(!table))
 		return 0;
@@ -186,15 +187,10 @@ static int __cpufreq_stats_create_table(struct cpufreq_policy *policy)
 		return -EEXIST;
 
 	stat = kzalloc(sizeof(*stat), GFP_KERNEL);
-	if ((stat) == NULL)
+	if (!stat)
 		return -ENOMEM;
 
-	ret = sysfs_create_group(&policy->kobj, &stats_attr_group);
-	if (ret)
-		goto error_out;
-
-	policy->stats_data = stat;
-
+	/* Find total allocation size */
 	cpufreq_for_each_valid_entry(pos, table)
 		count++;
 
@@ -203,32 +199,42 @@ static int __cpufreq_stats_create_table(struct cpufreq_policy *policy)
 #ifdef CONFIG_CPU_FREQ_STAT_DETAILS
 	alloc_size += count * count * sizeof(int);
 #endif
-	stat->max_state = count;
+
+	/* Allocate memory for time_in_state/freq_table/trans_table in one go */
 	stat->time_in_state = kzalloc(alloc_size, GFP_KERNEL);
-	if (!stat->time_in_state) {
-		ret = -ENOMEM;
-		goto error_alloc;
-	}
+	if (!stat->time_in_state)
+		goto free_stat;
+
 	stat->freq_table = (unsigned int *)(stat->time_in_state + count);
 
 #ifdef CONFIG_CPU_FREQ_STAT_DETAILS
 	stat->trans_table = stat->freq_table + count;
 #endif
-	i = 0;
+
+	stat->max_state = count;
+
+	/* Find valid-unique entries */
 	cpufreq_for_each_valid_entry(pos, table)
 		if (freq_table_get_index(stat, pos->frequency) == -1)
 			stat->freq_table[i++] = pos->frequency;
 	stat->state_num = i;
+
 	spin_lock(&cpufreq_stats_lock);
 	stat->last_time = get_jiffies_64();
 	stat->last_index = freq_table_get_index(stat, policy->cur);
 	spin_unlock(&cpufreq_stats_lock);
-	return 0;
-error_alloc:
-	sysfs_remove_group(&policy->kobj, &stats_attr_group);
-error_out:
-	kfree(stat);
+
+	policy->stats_data = stat;
+	ret = sysfs_create_group(&policy->kobj, &stats_attr_group);
+	if (!ret)
+		return 0;
+
+	/* We failed, release resources */
 	policy->stats_data = NULL;
+	kfree(stat->time_in_state);
+free_stat:
+	kfree(stat);
+
 	return ret;
 }
 
-- 
2.2.0

[PATCH V2 10/14] cpufreq: stats: don't update stats from show_trans_table()

[Viresh Kumar]

cpufreq_stats_update() updates time_in_state and nothing else. It should ideally
be updated only in two cases:
- User requested for the current value of time_in_state.
- We have switched states and so need to update time for the last state.

Currently, we are also doing this while user asks for the transition table of
frequencies. It wouldn't do any harm, but no good as well. Its useless here.

Remove it.

Reviewed-by: Prarit Bhargava <prarit@redhat.com>
Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 drivers/cpufreq/cpufreq_stats.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/cpufreq/cpufreq_stats.c b/drivers/cpufreq/cpufreq_stats.c
index dd55d57655ce..c0c2ddece8fe 100644
--- a/drivers/cpufreq/cpufreq_stats.c
+++ b/drivers/cpufreq/cpufreq_stats.c
@@ -77,7 +77,6 @@ static ssize_t show_trans_table(struct cpufreq_policy *policy, char *buf)
 	ssize_t len = 0;
 	int i, j;
 
-	cpufreq_stats_update(stat);
 	len += snprintf(buf + len, PAGE_SIZE - len, "   From  :    To\n");
 	len += snprintf(buf + len, PAGE_SIZE - len, "         : ");
 	for (i = 0; i < stat->state_num; i++) {
-- 
2.2.0

[PATCH V2 11/14] cpufreq: stats: don't update stats on false notifiers

[Viresh Kumar]

No idea when 'old_index == new_index' in transition notifier (probably never?),
but if this happens, we aren't supposed to update time_in_state by calling
cpufreq_stats_update(). Its only relevant when we are changing frequencies.

So, move the call after matching indexes.

Reviewed-by: Prarit Bhargava <prarit@redhat.com>
Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 drivers/cpufreq/cpufreq_stats.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/cpufreq/cpufreq_stats.c b/drivers/cpufreq/cpufreq_stats.c
index c0c2ddece8fe..7701532b32c8 100644
--- a/drivers/cpufreq/cpufreq_stats.c
+++ b/drivers/cpufreq/cpufreq_stats.c
@@ -293,11 +293,11 @@ static int cpufreq_stat_notifier_trans(struct notifier_block *nb,
 	if (old_index == -1 || new_index == -1)
 		return 0;
 
-	cpufreq_stats_update(stat);
-
 	if (old_index == new_index)
 		return 0;
 
+	cpufreq_stats_update(stat);
+
 	spin_lock(&cpufreq_stats_lock);
 	stat->last_index = new_index;
 #ifdef CONFIG_CPU_FREQ_STAT_DETAILS
-- 
2.2.0

Re: [PATCH V2 11/14] cpufreq: stats: don't update stats on false notifiers

[Rafael J. Wysocki]

On Friday, January 02, 2015 11:16:48 AM Viresh Kumar wrote:
> No idea when 'old_index == new_index' in transition notifier (probably never?),

This piece of information is completely irrelevant here.

> but if this happens, we aren't supposed to update time_in_state by calling
> cpufreq_stats_update().

"..., because ...".

> Its only relevant when we are changing frequencies.
> 
> So, move the call after matching indexes.
> 
> Reviewed-by: Prarit Bhargava <prarit@redhat.com>
> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
> ---
>  drivers/cpufreq/cpufreq_stats.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/cpufreq/cpufreq_stats.c b/drivers/cpufreq/cpufreq_stats.c
> index c0c2ddece8fe..7701532b32c8 100644
> --- a/drivers/cpufreq/cpufreq_stats.c
> +++ b/drivers/cpufreq/cpufreq_stats.c
> @@ -293,11 +293,11 @@ static int cpufreq_stat_notifier_trans(struct notifier_block *nb,
>  	if (old_index == -1 || new_index == -1)
>  		return 0;
>  
> -	cpufreq_stats_update(stat);
> -
>  	if (old_index == new_index)
>  		return 0;
>  
> +	cpufreq_stats_update(stat);
> +
>  	spin_lock(&cpufreq_stats_lock);
>  	stat->last_index = new_index;
>  #ifdef CONFIG_CPU_FREQ_STAT_DETAILS
> 

-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

[PATCH V2 12/14] cpufreq: stats: replace spinlock with mutex

[Viresh Kumar]

Later commit will allocate memory from within the spinlock as that would be part
of critical region. kzalloc() might sleep and we can't sleep from inside
spinlocks critical region, so convert the spinlock into a mutex.

Reviewed-by: Prarit Bhargava <prarit@redhat.com>
Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 drivers/cpufreq/cpufreq_stats.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/drivers/cpufreq/cpufreq_stats.c b/drivers/cpufreq/cpufreq_stats.c
index 7701532b32c8..de55ca86b6f1 100644
--- a/drivers/cpufreq/cpufreq_stats.c
+++ b/drivers/cpufreq/cpufreq_stats.c
@@ -12,10 +12,11 @@
 #include <linux/cpu.h>
 #include <linux/cpufreq.h>
 #include <linux/module.h>
+#include <linux/mutex.h>
 #include <linux/slab.h>
 #include <linux/cputime.h>
 
-static spinlock_t cpufreq_stats_lock;
+static DEFINE_MUTEX(cpufreq_stats_lock);
 
 struct cpufreq_stats {
 	unsigned int total_trans;
@@ -39,12 +40,12 @@ static int cpufreq_stats_update(struct cpufreq_stats *stat)
 {
 	unsigned long long cur_time = get_jiffies_64();
 
-	spin_lock(&cpufreq_stats_lock);
+	mutex_lock(&cpufreq_stats_lock);
 	if (stat->time_in_state)
 		stat->time_in_state[stat->last_index] +=
 			cur_time - stat->last_time;
 	stat->last_time = cur_time;
-	spin_unlock(&cpufreq_stats_lock);
+	mutex_unlock(&cpufreq_stats_lock);
 	return 0;
 }
 
@@ -218,10 +219,10 @@ static int __cpufreq_stats_create_table(struct cpufreq_policy *policy)
 			stat->freq_table[i++] = pos->frequency;
 	stat->state_num = i;
 
-	spin_lock(&cpufreq_stats_lock);
+	mutex_lock(&cpufreq_stats_lock);
 	stat->last_time = get_jiffies_64();
 	stat->last_index = freq_table_get_index(stat, policy->cur);
-	spin_unlock(&cpufreq_stats_lock);
+	mutex_unlock(&cpufreq_stats_lock);
 
 	policy->stats_data = stat;
 	ret = sysfs_create_group(&policy->kobj, &stats_attr_group);
@@ -298,13 +299,13 @@ static int cpufreq_stat_notifier_trans(struct notifier_block *nb,
 
 	cpufreq_stats_update(stat);
 
-	spin_lock(&cpufreq_stats_lock);
+	mutex_lock(&cpufreq_stats_lock);
 	stat->last_index = new_index;
 #ifdef CONFIG_CPU_FREQ_STAT_DETAILS
 	stat->trans_table[old_index * stat->max_state + new_index]++;
 #endif
 	stat->total_trans++;
-	spin_unlock(&cpufreq_stats_lock);
+	mutex_unlock(&cpufreq_stats_lock);
 	cpufreq_cpu_put(policy);
 	return 0;
 }
@@ -322,7 +323,6 @@ static int __init cpufreq_stats_init(void)
 	int ret;
 	unsigned int cpu;
 
-	spin_lock_init(&cpufreq_stats_lock);
 	ret = cpufreq_register_notifier(&notifier_policy_block,
 				CPUFREQ_POLICY_NOTIFIER);
 	if (ret)
-- 
2.2.0

Re: [PATCH V2 12/14] cpufreq: stats: replace spinlock with mutex

[Rafael J. Wysocki]

On Friday, January 02, 2015 11:16:49 AM Viresh Kumar wrote:
> Later commit will allocate memory from within the spinlock as that would be part
> of critical region. kzalloc() might sleep and we can't sleep from inside
> spinlocks critical region, so convert the spinlock into a mutex.

"We will need to allocate memory under cpufreq_stats_lock, so it cannot be
 a spinlock any more.  Convert it to a mutex."

> Reviewed-by: Prarit Bhargava <prarit@redhat.com>
> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
> ---
>  drivers/cpufreq/cpufreq_stats.c | 16 ++++++++--------
>  1 file changed, 8 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/cpufreq/cpufreq_stats.c b/drivers/cpufreq/cpufreq_stats.c
> index 7701532b32c8..de55ca86b6f1 100644
> --- a/drivers/cpufreq/cpufreq_stats.c
> +++ b/drivers/cpufreq/cpufreq_stats.c
> @@ -12,10 +12,11 @@
>  #include <linux/cpu.h>
>  #include <linux/cpufreq.h>
>  #include <linux/module.h>
> +#include <linux/mutex.h>
>  #include <linux/slab.h>
>  #include <linux/cputime.h>
>  
> -static spinlock_t cpufreq_stats_lock;
> +static DEFINE_MUTEX(cpufreq_stats_lock);
>  
>  struct cpufreq_stats {
>  	unsigned int total_trans;
> @@ -39,12 +40,12 @@ static int cpufreq_stats_update(struct cpufreq_stats *stat)
>  {
>  	unsigned long long cur_time = get_jiffies_64();
>  
> -	spin_lock(&cpufreq_stats_lock);
> +	mutex_lock(&cpufreq_stats_lock);
>  	if (stat->time_in_state)
>  		stat->time_in_state[stat->last_index] +=
>  			cur_time - stat->last_time;
>  	stat->last_time = cur_time;
> -	spin_unlock(&cpufreq_stats_lock);
> +	mutex_unlock(&cpufreq_stats_lock);
>  	return 0;
>  }
>  
> @@ -218,10 +219,10 @@ static int __cpufreq_stats_create_table(struct cpufreq_policy *policy)
>  			stat->freq_table[i++] = pos->frequency;
>  	stat->state_num = i;
>  
> -	spin_lock(&cpufreq_stats_lock);
> +	mutex_lock(&cpufreq_stats_lock);
>  	stat->last_time = get_jiffies_64();
>  	stat->last_index = freq_table_get_index(stat, policy->cur);
> -	spin_unlock(&cpufreq_stats_lock);
> +	mutex_unlock(&cpufreq_stats_lock);
>  
>  	policy->stats_data = stat;
>  	ret = sysfs_create_group(&policy->kobj, &stats_attr_group);
> @@ -298,13 +299,13 @@ static int cpufreq_stat_notifier_trans(struct notifier_block *nb,
>  
>  	cpufreq_stats_update(stat);
>  
> -	spin_lock(&cpufreq_stats_lock);
> +	mutex_lock(&cpufreq_stats_lock);
>  	stat->last_index = new_index;
>  #ifdef CONFIG_CPU_FREQ_STAT_DETAILS
>  	stat->trans_table[old_index * stat->max_state + new_index]++;
>  #endif
>  	stat->total_trans++;
> -	spin_unlock(&cpufreq_stats_lock);
> +	mutex_unlock(&cpufreq_stats_lock);
>  	cpufreq_cpu_put(policy);
>  	return 0;
>  }
> @@ -322,7 +323,6 @@ static int __init cpufreq_stats_init(void)
>  	int ret;
>  	unsigned int cpu;
>  
> -	spin_lock_init(&cpufreq_stats_lock);
>  	ret = cpufreq_register_notifier(&notifier_policy_block,
>  				CPUFREQ_POLICY_NOTIFIER);
>  	if (ret)
> 

-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

[PATCH V2 13/14] cpufreq: stats: Fix locking

[Viresh Kumar]

Locking wasn't handled properly at places and this patch is an attempt to fix
it. Specially while creating/removing stat tables.

Reviewed-by: Prarit Bhargava <prarit@redhat.com>
Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 drivers/cpufreq/cpufreq_stats.c | 18 ++++++++++++------
 1 file changed, 12 insertions(+), 6 deletions(-)

diff --git a/drivers/cpufreq/cpufreq_stats.c b/drivers/cpufreq/cpufreq_stats.c
index de55ca86b6f1..95867985ea02 100644
--- a/drivers/cpufreq/cpufreq_stats.c
+++ b/drivers/cpufreq/cpufreq_stats.c
@@ -150,10 +150,12 @@ static void __cpufreq_stats_free_table(struct cpufreq_policy *policy)
 
 	pr_debug("%s: Free stat table\n", __func__);
 
+	mutex_lock(&cpufreq_stats_lock);
 	sysfs_remove_group(&policy->kobj, &stats_attr_group);
 	kfree(stat->time_in_state);
 	kfree(stat);
 	policy->stats_data = NULL;
+	mutex_unlock(&cpufreq_stats_lock);
 }
 
 static void cpufreq_stats_free_table(unsigned int cpu)
@@ -182,13 +184,17 @@ static int __cpufreq_stats_create_table(struct cpufreq_policy *policy)
 	if (unlikely(!table))
 		return 0;
 
+	mutex_lock(&cpufreq_stats_lock);
+
 	/* stats already initialized */
-	if (policy->stats_data)
-		return -EEXIST;
+	if (policy->stats_data) {
+		ret = -EEXIST;
+		goto unlock;
+	}
 
 	stat = kzalloc(sizeof(*stat), GFP_KERNEL);
 	if (!stat)
-		return -ENOMEM;
+		goto unlock;
 
 	/* Find total allocation size */
 	cpufreq_for_each_valid_entry(pos, table)
@@ -219,21 +225,21 @@ static int __cpufreq_stats_create_table(struct cpufreq_policy *policy)
 			stat->freq_table[i++] = pos->frequency;
 	stat->state_num = i;
 
-	mutex_lock(&cpufreq_stats_lock);
 	stat->last_time = get_jiffies_64();
 	stat->last_index = freq_table_get_index(stat, policy->cur);
-	mutex_unlock(&cpufreq_stats_lock);
 
 	policy->stats_data = stat;
 	ret = sysfs_create_group(&policy->kobj, &stats_attr_group);
 	if (!ret)
-		return 0;
+		goto unlock;
 
 	/* We failed, release resources */
 	policy->stats_data = NULL;
 	kfree(stat->time_in_state);
 free_stat:
 	kfree(stat);
+unlock:
+	mutex_unlock(&cpufreq_stats_lock);
 
 	return ret;
 }
-- 
2.2.0

Re: [PATCH V2 13/14] cpufreq: stats: Fix locking

[Rafael J. Wysocki]

On Friday, January 02, 2015 11:16:50 AM Viresh Kumar wrote:
> Locking wasn't handled properly at places and this patch is an attempt to fix
> it. Specially while creating/removing stat tables.

This is pants, sorry.

Whenever you change locking, you need to know exactly (a) what is wrong with
the way it is currently done and (b) how you are going to improve it.  All of
that needs to be described here.

For example, you seem to be thinking that locking is missing from
__cpufreq_stats_free_table(), so you are adding it in there.  Thus you need to
describe (a) why it is missing (eg. what races are possible because of that)
and (b) why adding it the way you do is going to improve the situation.

> Reviewed-by: Prarit Bhargava <prarit@redhat.com>
> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
> ---
>  drivers/cpufreq/cpufreq_stats.c | 18 ++++++++++++------
>  1 file changed, 12 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/cpufreq/cpufreq_stats.c b/drivers/cpufreq/cpufreq_stats.c
> index de55ca86b6f1..95867985ea02 100644
> --- a/drivers/cpufreq/cpufreq_stats.c
> +++ b/drivers/cpufreq/cpufreq_stats.c
> @@ -150,10 +150,12 @@ static void __cpufreq_stats_free_table(struct cpufreq_policy *policy)
>  
>  	pr_debug("%s: Free stat table\n", __func__);
>  
> +	mutex_lock(&cpufreq_stats_lock);
>  	sysfs_remove_group(&policy->kobj, &stats_attr_group);
>  	kfree(stat->time_in_state);
>  	kfree(stat);
>  	policy->stats_data = NULL;
> +	mutex_unlock(&cpufreq_stats_lock);
>  }
>  
>  static void cpufreq_stats_free_table(unsigned int cpu)
> @@ -182,13 +184,17 @@ static int __cpufreq_stats_create_table(struct cpufreq_policy *policy)
>  	if (unlikely(!table))
>  		return 0;
>  
> +	mutex_lock(&cpufreq_stats_lock);
> +
>  	/* stats already initialized */
> -	if (policy->stats_data)
> -		return -EEXIST;
> +	if (policy->stats_data) {
> +		ret = -EEXIST;
> +		goto unlock;
> +	}
>  
>  	stat = kzalloc(sizeof(*stat), GFP_KERNEL);
>  	if (!stat)
> -		return -ENOMEM;
> +		goto unlock;
>  
>  	/* Find total allocation size */
>  	cpufreq_for_each_valid_entry(pos, table)
> @@ -219,21 +225,21 @@ static int __cpufreq_stats_create_table(struct cpufreq_policy *policy)
>  			stat->freq_table[i++] = pos->frequency;
>  	stat->state_num = i;
>  
> -	mutex_lock(&cpufreq_stats_lock);
>  	stat->last_time = get_jiffies_64();
>  	stat->last_index = freq_table_get_index(stat, policy->cur);
> -	mutex_unlock(&cpufreq_stats_lock);
>  
>  	policy->stats_data = stat;
>  	ret = sysfs_create_group(&policy->kobj, &stats_attr_group);
>  	if (!ret)
> -		return 0;
> +		goto unlock;
>  
>  	/* We failed, release resources */
>  	policy->stats_data = NULL;
>  	kfree(stat->time_in_state);
>  free_stat:
>  	kfree(stat);
> +unlock:
> +	mutex_unlock(&cpufreq_stats_lock);
>  
>  	return ret;
>  }
> 

-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

Re: [PATCH V2 13/14] cpufreq: stats: Fix locking

[Viresh Kumar]

On 4 January 2015 at 04:13, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> This is pants, sorry.

I am sorry :(

> Whenever you change locking, you need to know exactly (a) what is wrong with
> the way it is currently done and (b) how you are going to improve it.  All of
> that needs to be described here.

Thanks for kicking me here..

> For example, you seem to be thinking that locking is missing from
> __cpufreq_stats_free_table(), so you are adding it in there.  Thus you need to
> describe (a) why it is missing (eg. what races are possible because of that)
> and (b) why adding it the way you do is going to improve the situation.

Yeah, that's what I thought. That race between create/free of stats will
be eliminated by this..

But now as you forced me to write it properly, I am failing to understand why
do we need to have any locking in place for stats ?

These are the reasons why I think we can remove all locking stuff
from stats:

1.) create/free calls to stats can't run in parallel. They are all sequential.
It happens with:
- cpu hotplug: which is sequential with proper locking in place.
- driver unregister: again sequential
- stats unregister: again sequential

So, actually we will never try to allocate stats for a single policy in parallel
threads. Same goes for freeing as well..

2.) Any race possible with sysfs reads ?

These routines are called from show() routine from cpufreq.c and it has
policy locks on the way. So, policy can't go away and so will stats.

Also, if stats unregister we will unregister the notifiers first. And that will
again make things fall in line.

What else are we protecting? Maybe the calls to cpufreq_stats_update()
can happen in parallel and so we may need locking only within that routine.

I might be missing the obvious logic, but then what exactly ?

--
viresh

Re: [PATCH V2 13/14] cpufreq: stats: Fix locking

[Rafael J. Wysocki]

On Monday, January 05, 2015 01:36:39 PM Viresh Kumar wrote:
> On 4 January 2015 at 04:13, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> > This is pants, sorry.
> 
> I am sorry :(
> 
> > Whenever you change locking, you need to know exactly (a) what is wrong with
> > the way it is currently done and (b) how you are going to improve it.  All of
> > that needs to be described here.
> 
> Thanks for kicking me here..
> 
> > For example, you seem to be thinking that locking is missing from
> > __cpufreq_stats_free_table(), so you are adding it in there.  Thus you need to
> > describe (a) why it is missing (eg. what races are possible because of that)
> > and (b) why adding it the way you do is going to improve the situation.
> 
> Yeah, that's what I thought. That race between create/free of stats will
> be eliminated by this..
> 
> But now as you forced me to write it properly, I am failing to understand why
> do we need to have any locking in place for stats ?
> 
> These are the reasons why I think we can remove all locking stuff
> from stats:
> 
> 1.) create/free calls to stats can't run in parallel. They are all sequential.
> It happens with:
> - cpu hotplug: which is sequential with proper locking in place.
> - driver unregister: again sequential
> - stats unregister: again sequential
> 
> So, actually we will never try to allocate stats for a single policy in parallel
> threads. Same goes for freeing as well..
> 
> 2.) Any race possible with sysfs reads ?
> 
> These routines are called from show() routine from cpufreq.c and it has
> policy locks on the way. So, policy can't go away and so will stats.
> 
> Also, if stats unregister we will unregister the notifiers first. And that will
> again make things fall in line.

That sounds about right.

> What else are we protecting? Maybe the calls to cpufreq_stats_update()
> can happen in parallel and so we may need locking only within that routine.
> 
> I might be missing the obvious logic, but then what exactly ?

Hard to say from the top of my head and I'm not sure if I have the time to
have a deeper look into that during the next few days.


-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

Re: [PATCH V2 13/14] cpufreq: stats: Fix locking

[Viresh Kumar]

On 6 January 2015 at 05:05, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> Hard to say from the top of my head and I'm not sure if I have the time to
> have a deeper look into that during the next few days.

Ok, let me resend the patches then with better improved logs. And lets
see if something is still missing.

[PATCH V2 14/14] cpufreq: stats: call cpufreq_stats_update() with locks held

[Viresh Kumar]

cpufreq_stats_update() has only two callers, one of them is already taking locks
and other one don't.

To make locking efficient for cpufreq_stat_notifier_trans(), lets call
cpufreq_stats_update() from within locks. Also update show_time_in_state() to
call cpufreq_stats_update() after taking locks.

Reviewed-by: Prarit Bhargava <prarit@redhat.com>
Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 drivers/cpufreq/cpufreq_stats.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/drivers/cpufreq/cpufreq_stats.c b/drivers/cpufreq/cpufreq_stats.c
index 95867985ea02..f1b234a09f66 100644
--- a/drivers/cpufreq/cpufreq_stats.c
+++ b/drivers/cpufreq/cpufreq_stats.c
@@ -40,12 +40,10 @@ static int cpufreq_stats_update(struct cpufreq_stats *stat)
 {
 	unsigned long long cur_time = get_jiffies_64();
 
-	mutex_lock(&cpufreq_stats_lock);
 	if (stat->time_in_state)
 		stat->time_in_state[stat->last_index] +=
 			cur_time - stat->last_time;
 	stat->last_time = cur_time;
-	mutex_unlock(&cpufreq_stats_lock);
 	return 0;
 }
 
@@ -62,7 +60,10 @@ static ssize_t show_time_in_state(struct cpufreq_policy *policy, char *buf)
 	ssize_t len = 0;
 	int i;
 
+	mutex_lock(&cpufreq_stats_lock);
 	cpufreq_stats_update(stat);
+	mutex_unlock(&cpufreq_stats_lock);
+
 	for (i = 0; i < stat->state_num; i++) {
 		len += sprintf(buf + len, "%u %llu\n", stat->freq_table[i],
 			(unsigned long long)
@@ -303,9 +304,9 @@ static int cpufreq_stat_notifier_trans(struct notifier_block *nb,
 	if (old_index == new_index)
 		return 0;
 
+	mutex_lock(&cpufreq_stats_lock);
 	cpufreq_stats_update(stat);
 
-	mutex_lock(&cpufreq_stats_lock);
 	stat->last_index = new_index;
 #ifdef CONFIG_CPU_FREQ_STAT_DETAILS
 	stat->trans_table[old_index * stat->max_state + new_index]++;
-- 
2.2.0

Re: [PATCH V2 14/14] cpufreq: stats: call cpufreq_stats_update() with locks held

[Rafael J. Wysocki]

On Friday, January 02, 2015 11:16:51 AM Viresh Kumar wrote:
> cpufreq_stats_update() has only two callers, one of them is already taking locks
> and other one don't.

"and the other one doesn't."

> To make locking efficient for cpufreq_stat_notifier_trans(), lets call
> cpufreq_stats_update() from within locks. Also update show_time_in_state() to
> call cpufreq_stats_update() after taking locks.

"It doesn't make much sense to drop the lock in cpufreq_stats_update() and
 re-acquire it immediately after that in cpufreq_stat_notifier_trans(), so
 make callers of cpufreq_stats_update() responsible for acquiring the lock
 around it as appropriate and modify cpufreq_stat_notifier_trans() to call
 it under the lock it already uses."

> Reviewed-by: Prarit Bhargava <prarit@redhat.com>
> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
> ---
>  drivers/cpufreq/cpufreq_stats.c | 7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/cpufreq/cpufreq_stats.c b/drivers/cpufreq/cpufreq_stats.c
> index 95867985ea02..f1b234a09f66 100644
> --- a/drivers/cpufreq/cpufreq_stats.c
> +++ b/drivers/cpufreq/cpufreq_stats.c
> @@ -40,12 +40,10 @@ static int cpufreq_stats_update(struct cpufreq_stats *stat)
>  {
>  	unsigned long long cur_time = get_jiffies_64();
>  
> -	mutex_lock(&cpufreq_stats_lock);
>  	if (stat->time_in_state)
>  		stat->time_in_state[stat->last_index] +=
>  			cur_time - stat->last_time;
>  	stat->last_time = cur_time;
> -	mutex_unlock(&cpufreq_stats_lock);
>  	return 0;
>  }
>  
> @@ -62,7 +60,10 @@ static ssize_t show_time_in_state(struct cpufreq_policy *policy, char *buf)
>  	ssize_t len = 0;
>  	int i;
>  
> +	mutex_lock(&cpufreq_stats_lock);
>  	cpufreq_stats_update(stat);
> +	mutex_unlock(&cpufreq_stats_lock);
> +
>  	for (i = 0; i < stat->state_num; i++) {
>  		len += sprintf(buf + len, "%u %llu\n", stat->freq_table[i],
>  			(unsigned long long)
> @@ -303,9 +304,9 @@ static int cpufreq_stat_notifier_trans(struct notifier_block *nb,
>  	if (old_index == new_index)
>  		return 0;
>  
> +	mutex_lock(&cpufreq_stats_lock);
>  	cpufreq_stats_update(stat);
>  
> -	mutex_lock(&cpufreq_stats_lock);
>  	stat->last_index = new_index;
>  #ifdef CONFIG_CPU_FREQ_STAT_DETAILS
>  	stat->trans_table[old_index * stat->max_state + new_index]++;
> 

-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

Re: [PATCH V2 00/14] cpufreq: stats: cleanups

[Prarit Bhargava]

On 01/02/2015 12:46 AM, Viresh Kumar wrote:
> This is a resend of cpufreq stats cleanups I had for sometime. Few issues were
> reported from Fenguang's bot and are all fixed now and are stable..
> 
> V1->V2:
> - Reviewed-by Prarit
> - Replaced lock with mutex as we may sleep from within critical region and so a
>   new patch "cpufreq: stats: replace spinlock with mutex".
> 

Reviewed-by: Prarit Bhargava <prarit@redhat.com>

P.

> Pushed here:
> git://git.linaro.org/people/viresh.kumar/linux.git cpufreq/stats/cleanups
> 
> Viresh Kumar (14):
>   cpufreq: stats: don't break strings into multiple lines
>   cpufreq: stats: return -EEXIST when stats are already allocated
>   cpufreq: stats: don't check for freq table while freeing stats
>   cpufreq: stats: pass 'stat' to cpufreq_stats_update()
>   cpufreq: stats: get rid of per-cpu cpufreq_stats_table
>   cpufreq: Remove (now) unused 'last_cpu' from struct cpufreq_policy
>   cpufreq: stats: remove cpufreq_stats_update_policy_cpu()
>   cpufreq: remove CPUFREQ_UPDATE_POLICY_CPU notifications
>   cpufreq: stats: create sysfs group once we are ready
>   cpufreq: stats: don't update stats from show_trans_table()
>   cpufreq: stats: don't update stats on false notifiers
>   cpufreq: stats: replace spinlock with mutex
>   cpufreq: stats: Fix locking
>   cpufreq: stats: call cpufreq_stats_update() with locks held
> 
>  drivers/cpufreq/cpufreq.c       |   6 --
>  drivers/cpufreq/cpufreq_stats.c | 148 ++++++++++++++++++----------------------
>  include/linux/cpufreq.h         |  10 +--
>  3 files changed, 73 insertions(+), 91 deletions(-)
>