Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1733074 > unrolled thread

[PATCH] cpufreq: cpufreq_stats: make last_index signed int

Back to article view | Back to linux.kernel

Contents

  [PATCH] cpufreq: cpufreq_stats: make last_index signed int Bo Yan <byan@nvidia.com> - 2017-09-15 22:20 +0200
    Re: [PATCH] cpufreq: cpufreq_stats: make last_index signed int Viresh Kumar <viresh.kumar@linaro.org> - 2017-09-18 04:00 +0200
      Re: [PATCH] cpufreq: cpufreq_stats: make last_index signed int Bo Yan <byan@nvidia.com> - 2017-09-18 19:50 +0200
        Re: [PATCH] cpufreq: cpufreq_stats: make last_index signed int Viresh Kumar <viresh.kumar@linaro.org> - 2017-09-19 21:00 +0200

#1733074 — [PATCH] cpufreq: cpufreq_stats: make last_index signed int

It is possible for last_index to get a -1 if current frequency
is not found in the freq table when stats is created. If the
function "cpufreq_stats_update" is called before last_index is
updated with a valid value, the "-1" will be used as index to
update stats->time_in_state, triggering an exception.

Change the type of last_index of cpufreq_stats from unsigned int
to signed int.

Move last_time to location right before time_in_state. This helps
save 4 bytes in a LP64 programming model.

Signed-off-by: Bo Yan <byan@nvidia.com>
---
 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 e75880eb037d..a6838f5a2004 100644
--- a/drivers/cpufreq/cpufreq_stats.c
+++ b/drivers/cpufreq/cpufreq_stats.c
@@ -18,10 +18,10 @@ static DEFINE_SPINLOCK(cpufreq_stats_lock);
 
 struct cpufreq_stats {
 	unsigned int total_trans;
-	unsigned long long last_time;
 	unsigned int max_state;
 	unsigned int state_num;
-	unsigned int last_index;
+	int last_index;
+	unsigned long long last_time;
 	u64 *time_in_state;
 	unsigned int *freq_table;
 	unsigned int *trans_table;
-- 
2.7.4

[toc] | [next] | [standalone]

#1733609

On 15-09-17, 13:13, Bo Yan wrote:
> It is possible for last_index to get a -1 if current frequency
> is not found in the freq table when stats is created. If the
> function "cpufreq_stats_update" is called before last_index is
> updated with a valid value, the "-1" will be used as index to
> update stats->time_in_state, triggering an exception.

No, that's not how it works AFAIK and if it did, then can you explain how your
solution fixes it?

AFAIK, what happens right now is that stats->last_index eventually stores
2147483647 (uint max) and the exception comes while accessing that value.

While with your change, it will become -1 and accessing array[-1] is fine by C
standards, though it is still the wrong thing to do as you are accessing
something outside of the array.

We should just check last_index == -1 before calling cpufreq_stats_update(),
which is already done by one of the callers.

-- 
viresh

[toc] | [prev] | [next] | [standalone]

#1734339

On 09/17/2017 06:50 PM, Viresh Kumar wrote:
> On 15-09-17, 13:13, Bo Yan wrote:
>> It is possible for last_index to get a -1 if current frequency
>> is not found in the freq table when stats is created. If the
>> function "cpufreq_stats_update" is called before last_index is
>> updated with a valid value, the "-1" will be used as index to
>> update stats->time_in_state, triggering an exception.
> 
> No, that's not how it works AFAIK and if it did, then can you explain how your
> solution fixes it?
> 
> AFAIK, what happens right now is that stats->last_index eventually stores
> 2147483647 (uint max) and the exception comes while accessing that value.
> 
> While with your change, it will become -1 and accessing array[-1] is fine by C
> standards, though it is still the wrong thing to do as you are accessing
> something outside of the array.
> 
> We should just check last_index == -1 before calling cpufreq_stats_update(),
> which is already done by one of the callers.
> 

Currently, the "last_index" is being checked before 
cpufreq_stats_update(stats) inside function 
"cpufreq_stats_record_transition", so it's taken care of.

However, the function "show_time_in_state" also calls 
cpufreq_stats_update, the similar check should be done there too, like this:

diff --git a/drivers/cpufreq/cpufreq_stats.c 
b/drivers/cpufreq/cpufreq_stats.c
index e75880eb037d..15305b5ec322 100644
--- a/drivers/cpufreq/cpufreq_stats.c
+++ b/drivers/cpufreq/cpufreq_stats.c
@@ -62,7 +62,8 @@ static ssize_t show_time_in_state(struct 
cpufreq_policy *policy, char *buf)
         if (policy->fast_switch_enabled)
                 return 0;

-       cpufreq_stats_update(stats);
+       if ((int)stats->last_index >= 0)
+               cpufreq_stats_update(stats);
         for (i = 0; i < stats->state_num; i++) {
                 len += sprintf(buf + len, "%u %llu\n", 
stats->freq_table[i],
                         (unsigned long long)


This is only needed when policy->cur is not in frequency table when 
stats table is created, in which case, stats->last_index will get -1, 
then user does a "cat time_in_state" before any frequency transition.

Does this make sense?

[toc] | [prev] | [next] | [standalone]

#1735175

On 18-09-17, 10:39, Bo Yan wrote:
> Currently, the "last_index" is being checked before
> cpufreq_stats_update(stats) inside function
> "cpufreq_stats_record_transition", so it's taken care of.
> 
> However, the function "show_time_in_state" also calls cpufreq_stats_update,
> the similar check should be done there too, like this:

Yeah, that's what I suggested.

> diff --git a/drivers/cpufreq/cpufreq_stats.c
> b/drivers/cpufreq/cpufreq_stats.c
> index e75880eb037d..15305b5ec322 100644
> --- a/drivers/cpufreq/cpufreq_stats.c
> +++ b/drivers/cpufreq/cpufreq_stats.c
> @@ -62,7 +62,8 @@ static ssize_t show_time_in_state(struct cpufreq_policy
> *policy, char *buf)
>         if (policy->fast_switch_enabled)
>                 return 0;
> 
> -       cpufreq_stats_update(stats);
> +       if ((int)stats->last_index >= 0)

You can rather do:

        if (stats->last_index != -1)

> +               cpufreq_stats_update(stats);
>         for (i = 0; i < stats->state_num; i++) {
>                 len += sprintf(buf + len, "%u %llu\n", stats->freq_table[i],
>                         (unsigned long long)
> 
> 
> This is only needed when policy->cur is not in frequency table when stats
> table is created, in which case, stats->last_index will get -1, then user
> does a "cat time_in_state" before any frequency transition.
> 
> Does this make sense?

-- 
viresh

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web