| Message ID | 1352313166-28980-4-git-send-email-mark.langsdorf@calxeda.com (mailing list archive) |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
On Wed, Nov 07, 2012 at 12:32:43PM -0600, Mark Langsdorf wrote: > When collecting stats, if a frequency doesn't match the table, go through > the table again with both the search frequency and table values shifted > left by 10 bits. Why would that second pass succeed? And why is this in generic code (I'm assuming this is a Calxeda-specific case)?
On 11/11/2012 10:38 AM, Borislav Petkov wrote: > On Wed, Nov 07, 2012 at 12:32:43PM -0600, Mark Langsdorf wrote: >> When collecting stats, if a frequency doesn't match the table, go through >> the table again with both the search frequency and table values shifted >> left by 10 bits. > > Why would that second pass succeed? It's effectively a divide by 1024 and minimizes any jitter in the measured frequency value. > And why is this in generic code (I'm assuming this is a Calxeda-specific > case)? The function is buried pretty deep in the cpufreq_stat code. It didn't seem appropriate to make it a function pointer as part of struct cpufreq_driver. --Mark Langsdorf Calxeda, Inc. -- 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 Mon, Nov 12, 2012 at 10:35:49AM -0600, Mark Langsdorf wrote: > The function is buried pretty deep in the cpufreq_stat code. It didn't > seem appropriate to make it a function pointer as part of struct > cpufreq_driver. Right, what's cpufreq-speak for if (Calxeda) shift by 10 ? Better yet, add a flag or a bitfield called "minimize_jitter" or similar and set it only on your hardware... Thanks.
On 11/13/2012 10:24 AM, Borislav Petkov wrote: > On Mon, Nov 12, 2012 at 10:35:49AM -0600, Mark Langsdorf wrote: >> The function is buried pretty deep in the cpufreq_stat code. It didn't >> seem appropriate to make it a function pointer as part of struct >> cpufreq_driver. > > Right, what's cpufreq-speak for > > if (Calxeda) > shift by 10 > > ? > > Better yet, add a flag or a bitfield called "minimize_jitter" or similar > and set it only on your hardware... Doing it in two passes has a similar effect: systems that have exact frequencies will get caught in the first pass when the values match. But adding a flag makes sense. --Mark Langsdorf Calxeda, Inc. -- 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
From: linux-pm-owner@vger.kernel.org [linux-pm-owner@vger.kernel.org] On Behalf Of Mark Langsdorf [mark.langsdorf@calxeda.com] > On 11/13/2012 10:24 AM, Borislav Petkov wrote: >> On Mon, Nov 12, 2012 at 10:35:49AM -0600, Mark Langsdorf wrote: >>> The function is buried pretty deep in the cpufreq_stat code. It didn't >>> seem appropriate to make it a function pointer as part of struct >>> cpufreq_driver. >> >> Better yet, add a flag or a bitfield called "minimize_jitter" or similar >> and set it only on your hardware... > > Doing it in two passes has a similar effect: systems that have exact > frequencies will get caught in the first pass when the values match. But > adding a flag makes sense. I went back and looked at implementing this suggestion. Although cpufreq_driver has a flag field, no part of cpufreq_driver is directly passed to the cpufreq_stat code. Only cpufreq_policy is. It's cleaner to do passes of the while loop than to copy the cpufreq_driver.flag field into cpufreq_policy and then store it again in cpufreq_stats. --Mark Langsdorf Calxeda, Inc.-- 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 Tue, Nov 13, 2012 at 02:13:38PM -0500, Mark Langsdorf wrote: > Although cpufreq_driver has a flag field, no part of cpufreq_driver > is directly passed to the cpufreq_stat code. Only cpufreq_policy > is. It's cleaner to do passes of the while loop than to copy the > cpufreq_driver.flag field into cpufreq_policy and then store it again > in cpufreq_stats. That maybe so but this newly added loop which is only Calxeda-relevant is called in cpufreq_stat_notifier_trans, which is the frequency change notifier call, AFAICT. So each cpufreq driver will be paying that small and needless penalty now for nothing and on each frequency change. Which adds to the kernel-wide bloat and we absolutely don't want that. So you probably need to find a slick way of detecting calxeda hw somewhere along the init path of cpufreq_stats_init and set a hw-specific flag instead of adding that cost to each driver. Thanks.
On Saturday, November 17, 2012 03:50:48 PM Borislav Petkov wrote: > On Tue, Nov 13, 2012 at 02:13:38PM -0500, Mark Langsdorf wrote: > > Although cpufreq_driver has a flag field, no part of cpufreq_driver > > is directly passed to the cpufreq_stat code. Only cpufreq_policy > > is. It's cleaner to do passes of the while loop than to copy the > > cpufreq_driver.flag field into cpufreq_policy and then store it again > > in cpufreq_stats. > > That maybe so but this newly added loop which is only Calxeda-relevant > is called in cpufreq_stat_notifier_trans, which is the frequency change > notifier call, AFAICT. > > So each cpufreq driver will be paying that small and needless penalty > now for nothing and on each frequency change. Which adds to the > kernel-wide bloat and we absolutely don't want that. > > So you probably need to find a slick way of detecting calxeda hw > somewhere along the init path of cpufreq_stats_init and set a > hw-specific flag instead of adding that cost to each driver. Mark, I suppose you'd like me to take this series for v3.8, but the above comment from Boris has to be addressed for that. Thanks, Rafael
On 11/24/2012 04:05 AM, Rafael J. Wysocki wrote: > On Saturday, November 17, 2012 03:50:48 PM Borislav Petkov wrote: >> On Tue, Nov 13, 2012 at 02:13:38PM -0500, Mark Langsdorf wrote: >>> Although cpufreq_driver has a flag field, no part of cpufreq_driver >>> is directly passed to the cpufreq_stat code. Only cpufreq_policy >>> is. It's cleaner to do passes of the while loop than to copy the >>> cpufreq_driver.flag field into cpufreq_policy and then store it again >>> in cpufreq_stats. >> >> That maybe so but this newly added loop which is only Calxeda-relevant >> is called in cpufreq_stat_notifier_trans, which is the frequency change >> notifier call, AFAICT. Drivers only go through the loop if they can't find an exact frequency. So every driver that isn't Calxeda shouldn't see the issue. >> So you probably need to find a slick way of detecting calxeda hw >> somewhere along the init path of cpufreq_stats_init and set a >> hw-specific flag instead of adding that cost to each driver. > > Mark, I suppose you'd like me to take this series for v3.8, but the above > comment from Boris has to be addressed for that. I think I'd rather drop this particular patch and not have cpufreq_stat support for Highbank. Redesigning it to meet Boris' requirements is going to take more time than I currently have available. Would it be acceptable to drop this patch and fix the issues with patches 4 and 6 to get the series in? --Mark Langsdorf Calxeda, Inc. -- 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 Monday, November 26, 2012 07:57:30 AM Mark Langsdorf wrote: > On 11/24/2012 04:05 AM, Rafael J. Wysocki wrote: > > On Saturday, November 17, 2012 03:50:48 PM Borislav Petkov wrote: > >> On Tue, Nov 13, 2012 at 02:13:38PM -0500, Mark Langsdorf wrote: > >>> Although cpufreq_driver has a flag field, no part of cpufreq_driver > >>> is directly passed to the cpufreq_stat code. Only cpufreq_policy > >>> is. It's cleaner to do passes of the while loop than to copy the > >>> cpufreq_driver.flag field into cpufreq_policy and then store it again > >>> in cpufreq_stats. > >> > >> That maybe so but this newly added loop which is only Calxeda-relevant > >> is called in cpufreq_stat_notifier_trans, which is the frequency change > >> notifier call, AFAICT. > > Drivers only go through the loop if they can't find an exact frequency. > So every driver that isn't Calxeda shouldn't see the issue. > > >> So you probably need to find a slick way of detecting calxeda hw > >> somewhere along the init path of cpufreq_stats_init and set a > >> hw-specific flag instead of adding that cost to each driver. > > > > Mark, I suppose you'd like me to take this series for v3.8, but the above > > comment from Boris has to be addressed for that. > > I think I'd rather drop this particular patch and not have cpufreq_stat > support for Highbank. Redesigning it to meet Boris' requirements is > going to take more time than I currently have available. > > Would it be acceptable to drop this patch and fix the issues with > patches 4 and 6 to get the series in? Yes, it would, but please resubmit ASAP. Thanks, Rafael
diff --git a/drivers/cpufreq/cpufreq_stats.c b/drivers/cpufreq/cpufreq_stats.c index 3998316..30aee36 100644 --- a/drivers/cpufreq/cpufreq_stats.c +++ b/drivers/cpufreq/cpufreq_stats.c @@ -161,6 +161,9 @@ static int freq_table_get_index(struct cpufreq_stats *stat, unsigned int freq) for (index = 0; index < stat->max_state; index++) if (stat->freq_table[index] == freq) return index; + for (index = 0; index < stat->max_state; index++) + if ((stat->freq_table[index] >> 10) == (freq >> 10)) + return index; return -1; } @@ -251,6 +254,8 @@ static int cpufreq_stats_create_table(struct cpufreq_policy *policy, spin_lock(&cpufreq_stats_lock); stat->last_time = get_jiffies_64(); stat->last_index = freq_table_get_index(stat, policy->cur); + if (stat->last_index > stat->max_state) + stat->last_index = stat->max_state - 1; spin_unlock(&cpufreq_stats_lock); cpufreq_cpu_put(data); return 0;
When collecting stats, if a frequency doesn't match the table, go through the table again with both the search frequency and table values shifted left by 10 bits. Signed-off-by: Mark Langsdorf <mark.langsdorf@calxeda.com> Cc: MyungJoo Ham <myungjoo.ham@gmail.com> --- Changes from v3, v2 None Changes from v1: Implemented a simple round-up algorithm instead of the over/under method that could cause errors on Intel processors with boost mode. drivers/cpufreq/cpufreq_stats.c | 5 +++++ 1 file changed, 5 insertions(+)