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

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

[Nishanth Menon]

commit 46a310b ([CPUFREQ] Don't set stat->last_index to -1 if the
pol->cur has incorrect value.) tries to handle case where policy->cur
does not match any entry in freq_table.

As indicated in the above commit, the exact match search of
freq_table_get index will return a -1 which is stored in
stat->last_index. However, as a result of the above commit,
cpufreq_stat_notifier_trans which updates the statistics, fails
to update any *further* valid transitions that take place as
stat->last_index is -1 as the condition occurs at boot and never
solved.

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
information are the available frequencies which is already available
in other cpufreq sysfs entries.

Cc: Tobias Diedrich <ranma+xen@tdiedrich.de>
Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
Cc: Dave Jones <davej@redhat.com>
Reported-by: Carlos Hernandez <ceh@ti.com>
Signed-off-by: Nishanth Menon <nm@ti.com>
---

Patch based on v3.12 tag

Reported by Carlos on TI vendor kernel (v3.12tag based):

OMAP5uEVM: http://pastebin.mozilla.org/3612196
AM335x-evm: http://pastebin.mozilla.org/3612220

As can be seen, the translation table are present, however none of the
transition information is ever updated.
With the current patch, this becomes: http://pastebin.mozilla.org/3612256

Original commit thread seems to be:
https://lkml.org/lkml/2011/6/16/760 but I dont see much information if
this was discussed further.

An alternate approach possible is to try and recover from this case by
using an 'atmost' match to find a closest match and then giveup when
we cant find one, but that does not really indicate a proper statistic
(if freq1 < cur_freq < freq2, was the intent to be at freq1 or freq2?
stats should not make that policy decision)
	http://pastebin.mozilla.org/3612179 (alternate approach 1)

Yet another option might be to update last_index using the first
transition into a valid index, but then, we wont have statistics
information 100% correct as we did not store the very first frequency
stat.
	 http://pastebin.mozilla.org/3612241 (alternate approach 2)

 drivers/cpufreq/cpufreq_stats.c |    8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/drivers/cpufreq/cpufreq_stats.c b/drivers/cpufreq/cpufreq_stats.c
index 4cf0d28..4c8a501 100644
--- a/drivers/cpufreq/cpufreq_stats.c
+++ b/drivers/cpufreq/cpufreq_stats.c
@@ -251,6 +251,14 @@ static int cpufreq_stats_create_table(struct cpufreq_policy *policy,
 	stat->last_time = get_jiffies_64();
 	stat->last_index = freq_table_get_index(stat, policy->cur);
 	spin_unlock(&cpufreq_stats_lock);
+
+	if (stat->last_index == -1) {
+		pr_err("%s: No match for current freq %u in table. Disabled!\n",
+		       __func__, policy->cur);
+		ret = -EINVAL;
+		goto error_out;
+	}
+
 	cpufreq_cpu_put(current_policy);
 	return 0;
 error_out:
-- 
1.7.9.5

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

[Rafael J. Wysocki]

On Friday, November 15, 2013 06:20:43 PM Nishanth Menon wrote:
> commit 46a310b ([CPUFREQ] Don't set stat->last_index to -1 if the
> pol->cur has incorrect value.) tries to handle case where policy->cur
> does not match any entry in freq_table.
> 
> As indicated in the above commit, the exact match search of
> freq_table_get index will return a -1 which is stored in
> stat->last_index. However, as a result of the above commit,
> cpufreq_stat_notifier_trans which updates the statistics, fails
> to update any *further* valid transitions that take place as
> stat->last_index is -1 as the condition occurs at boot and never
> solved.
> 
> 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
> information are the available frequencies which is already available
> in other cpufreq sysfs entries.
> 
> Cc: Tobias Diedrich <ranma+xen@tdiedrich.de>
> Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> Cc: Dave Jones <davej@redhat.com>
> Reported-by: Carlos Hernandez <ceh@ti.com>
> Signed-off-by: Nishanth Menon <nm@ti.com>

I like this one.  Any objections from anyone?

> ---
> 
> Patch based on v3.12 tag
> 
> Reported by Carlos on TI vendor kernel (v3.12tag based):
> 
> OMAP5uEVM: http://pastebin.mozilla.org/3612196
> AM335x-evm: http://pastebin.mozilla.org/3612220
> 
> As can be seen, the translation table are present, however none of the
> transition information is ever updated.
> With the current patch, this becomes: http://pastebin.mozilla.org/3612256
> 
> Original commit thread seems to be:
> https://lkml.org/lkml/2011/6/16/760 but I dont see much information if
> this was discussed further.
> 
> An alternate approach possible is to try and recover from this case by
> using an 'atmost' match to find a closest match and then giveup when
> we cant find one, but that does not really indicate a proper statistic
> (if freq1 < cur_freq < freq2, was the intent to be at freq1 or freq2?
> stats should not make that policy decision)
> 	http://pastebin.mozilla.org/3612179 (alternate approach 1)
> 
> Yet another option might be to update last_index using the first
> transition into a valid index, but then, we wont have statistics
> information 100% correct as we did not store the very first frequency
> stat.
> 	 http://pastebin.mozilla.org/3612241 (alternate approach 2)
> 
>  drivers/cpufreq/cpufreq_stats.c |    8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/drivers/cpufreq/cpufreq_stats.c b/drivers/cpufreq/cpufreq_stats.c
> index 4cf0d28..4c8a501 100644
> --- a/drivers/cpufreq/cpufreq_stats.c
> +++ b/drivers/cpufreq/cpufreq_stats.c
> @@ -251,6 +251,14 @@ static int cpufreq_stats_create_table(struct cpufreq_policy *policy,
>  	stat->last_time = get_jiffies_64();
>  	stat->last_index = freq_table_get_index(stat, policy->cur);
>  	spin_unlock(&cpufreq_stats_lock);
> +
> +	if (stat->last_index == -1) {
> +		pr_err("%s: No match for current freq %u in table. Disabled!\n",
> +		       __func__, policy->cur);
> +		ret = -EINVAL;
> +		goto error_out;
> +	}
> +
>  	cpufreq_cpu_put(current_policy);
>  	return 0;
>  error_out:
> 
-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

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

[viresh kumar]

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 :))

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

[Nishanth Menon]

On 11/15/2013 11:22 PM, viresh kumar wrote:
> 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 :)

http://pastebin.mozilla.org/3628975

I agree that it does show something, but would we not rather prefer to
stick with the entries available in freq_table than have to deal with
invalid frequencies that may be provided by the driver? for example:
how do we in stat know that there will only be one invalid frequency
request?


> 

[..]
> (@Rafael: Finally I have moved to thunderbird, found a way out, so no more
> crappy attachments from me :))
> 
there are some patch wrapping that thunderbird tends to do - I prefer
mutt that way, when I need to send inline patches :).

-- 
Regards,
Nishanth Menon

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

[Viresh Kumar]

On 18 November 2013 20:38, Nishanth Menon <nm@ti.com> wrote:
> On 11/15/2013 11:22 PM, viresh kumar wrote:

>> I have a untested patch for this. If this looks okay, Nishant can you please try
>> below patch? With some fixups from your side :)
>
> http://pastebin.mozilla.org/3628975
>
> I agree that it does show something

Were you required to update it or fix it? Or you used it as is?

> but would we not rather prefer to
> stick with the entries available in freq_table than have to deal with
> invalid frequencies that may be provided by the driver? for example:
> how do we in stat know that there will only be one invalid frequency
> request?

We don't have to. Actually there is nothing like an invalid freq, as system
is able to run with it. Its only if kernel is allowed to switch to that
frequency or not.

This information will be useful for platform developers to know that kernel
was running on some out of freq-table freq for some time, what that
frequency was their job to find out and kernel doesn't need to print all
out of range frequencies. Keeping a single entry for that is more than
sufficient..

Though I personally didn't like: 4294967295 to be printed there.. Will
see if I can improve that, but its obviously looks better than failing
in cpufreq-stats..

> there are some patch wrapping that thunderbird tends to do - I prefer
> mutt that way, when I need to send inline patches :).

There are steps in Documentation/email-clients.txt to get that fixed.. I
have used thunderbird for several years and never found anything
wrong with sending inline patches..

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

[Nishanth Menon]

On 11/18/2013 09:22 AM, Viresh Kumar wrote:
> On 18 November 2013 20:38, Nishanth Menon <nm@ti.com> wrote:
>> On 11/15/2013 11:22 PM, viresh kumar wrote:
> 
>>> I have a untested patch for this. If this looks okay, Nishant can you please try
>>> below patch? With some fixups from your side :)
>>
>> http://pastebin.mozilla.org/3628975
>>
>> I agree that it does show something
> 
> Were you required to update it or fix it? Or you used it as is?

as is - though manually applied ;)

> 
>> but would we not rather prefer to
>> stick with the entries available in freq_table than have to deal with
>> invalid frequencies that may be provided by the driver? for example:
>> how do we in stat know that there will only be one invalid frequency
>> request?
> 
> We don't have to. Actually there is nothing like an invalid freq, as system
> is able to run with it. Its only if kernel is allowed to switch to that
> frequency or not.

depends on how we look at it.. if we consider freq_table as a list of
valid frequencies, anything that is not in there is an invalid entry.
why should stat support it when freq_table is enforced by cpufreq layer?

> 
> This information will be useful for platform developers to know that kernel
> was running on some out of freq-table freq for some time, what that
> frequency was their job to find out and kernel doesn't need to print all
> out of range frequencies. Keeping a single entry for that is more than
> sufficient..
yeah, I guessed that.. no strong feelings other wise..

> 
> Though I personally didn't like: 4294967295 to be printed there.. Will
> see if I can improve that, but its obviously looks better than failing
> in cpufreq-stats..

> 
>> there are some patch wrapping that thunderbird tends to do - I prefer
>> mutt that way, when I need to send inline patches :).
> 
> There are steps in Documentation/email-clients.txt to get that fixed.. I
> have used thunderbird for several years and never found anything
> wrong with sending inline patches..

:) -> https://patchwork.kernel.org/patch/3192211/ shows how confused
my git am  feels ;)


-- 
Regards,
Nishanth Menon

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

[Viresh Kumar]

On 18 November 2013 21:04, Nishanth Menon <nm@ti.com> wrote:
> depends on how we look at it.. if we consider freq_table as a list of
> valid frequencies, anything that is not in there is an invalid entry.

Partly correct. Yes freq table has a list of valid frequencies, but normally
we just build a table of definite length which will cover frequencies of
all ranges.. Our plls allow us to configure other frequencies too and
they are valid too.. So, we really can't call that invalid.. Though they
must be called out of range? or something better?

> why should stat support it when freq_table is enforced by cpufreq layer?

We are not supporting them, and so had a single entry for them. It is
just to show to user that we are actually running on some freq outside
of table. You better check if that's fine for your system or not.

> :) -> https://patchwork.kernel.org/patch/3192211/ shows how confused
> my git am  feels ;)

I am surprised to see what patchwork has done to my diff :) .. whereas
lkml shows it correctly..

https://lkml.org/lkml/2013/11/16/3

Because I haven't created a commit, it was only diff and git am couldn't
get this fixed.. So either patch -p1 < mail would have done it or maybe
its better if I paste commits..