linux-kernel - Re: [PATCH] cpufreq: stats: Do not populate stats when policy->cur has no exact match

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Sat, 16 Nov 2013 10:52:23 +0530
From:	viresh kumar <viresh.kumar@...aro.org>
To:	"Rafael J. Wysocki" <rjw@...ysocki.net>, Nishanth Menon <nm@...com>
CC:	cpufreq@...r.kernel.org, linux-pm@...r.kernel.org,
	linux-kernel@...r.kernel.org, ceh@...com,
	Tobias Diedrich <ranma+xen@...edrich.de>,
	Konrad Rzeszutek Wilk <konrad.wilk@...cle.com>,
	Dave Jones <davej@...hat.com>
Subject: Re: [PATCH] cpufreq: stats: Do not populate stats when policy->cur
 has no exact match

On Saturday 16 November 2013 06:40 AM, Rafael J. Wysocki wrote:
> On Friday, November 15, 2013 06:20:43 PM Nishanth Menon wrote:

>> So, instead of having a statistics information that never ever
>> reflects valid data in the mentioned case and scratching our heads, we
>> instead, refuse to populate any of the statistics entries and note in
>> kernel log the error condition for developers to fix. The only useable

s/useable/usable

>> information are the available frequencies which is already available
>> in other cpufreq sysfs entries.

> I like this one.  Any objections from anyone?

Well nothing against the patch but I have other thoughts. There are platforms
which might have no choice of fixing this issue as their bootloaders might be
setting boot freq to any value outside of our freq table.

And those might not have a chance to fix that in driver as well in case they are
using something like cpufreq-cpu0..

So, eventually this patch wouldn't do anything except giving a boot time error
and not initializing any stats at all..

Wouldn't it be better to create another frequency in all these tables, which
will be an *Invalid* frequency.. With a value of -1 (i.e. largest value of an
unsigned int) ??

And so nobody will ever miss stats again, even if they are running on invalid
frequencies. We will capture that information too:
- we have moved from/to invalid frequency to/from a valid/invalid frequency this
much times.
- We have stayed at valid/invalid frequencies for this much time.

I have a untested patch for this. If this looks okay, Nishant can you please try
below patch? With some fixups from your side :)

diff --git a/drivers/cpufreq/cpufreq_stats.c b/drivers/cpufreq/cpufreq_stats.c
index 4cf0d28..0c551a6 100644
--- a/drivers/cpufreq/cpufreq_stats.c
+++ b/drivers/cpufreq/cpufreq_stats.c
@@ -145,10 +145,12 @@ static struct attribute_group stats_attr_group = {
 static int freq_table_get_index(struct cpufreq_stats *stat, unsigned int freq)
 {
        int index;
-       for (index = 0; index < stat->max_state; index++)
+       for (index = 0; index < stat->max_state - 1; index++)
                if (stat->freq_table[index] == freq)
                        return index;
-       return -1;
+
+       /* Last state is INVALID, to mark out of table frequency */
+       return stat->max_state - 1;
 }

 /* should be called late in the CPU removal sequence so that the stats
@@ -222,6 +224,9 @@ static int cpufreq_stats_create_table(struct cpufreq_policy
*policy,
                count++;
        }

+       /* An extra entry for Invalid frequencies */
+       count++;
+
        alloc_size = count * sizeof(int) + count * sizeof(u64);

 #ifdef CONFIG_CPU_FREQ_STAT_DETAILS
@@ -243,9 +248,13 @@ static int cpufreq_stats_create_table(struct cpufreq_policy
*policy,
                unsigned int freq = table[i].frequency;
                if (freq == CPUFREQ_ENTRY_INVALID)
                        continue;
-               if (freq_table_get_index(stat, freq) == -1)
+               if (freq_table_get_index(stat, freq) == stat->max_state - 1)
                        stat->freq_table[j++] = freq;
        }
+
+       /* Mark Invalid freq as max value to indicate Invalid freq */
+       stat->freq_table[j++] = -1;
+
        stat->state_num = j;
        spin_lock(&cpufreq_stats_lock);
        stat->last_time = get_jiffies_64();
@@ -315,10 +324,6 @@ static int cpufreq_stat_notifier_trans(struct
notifier_block *nb,
        old_index = stat->last_index;
        new_index = freq_table_get_index(stat, freq->new);

-       /* We can't do stat->time_in_state[-1]= .. */
-       if (old_index == -1 || new_index == -1)
-               return 0;
-
        cpufreq_stats_update(freq->cpu);

        if (old_index == new_index)


(@Rafael: Finally I have moved to thunderbird, found a way out, so no more
crappy attachments from me :))
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/