| Message ID | 5287010F.9000508@linaro.org (mailing list archive) |
|---|---|
| State | RFC, archived |
| Headers | show |
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 :).
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.. -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 ;)
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.. -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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);