Date: Fri, 2 May 2008 11:03:32 -0400 From: John Baldwin <jhb@freebsd.org> To: Rui Paulo <rpaulo@freebsd.org> Cc: cvs-src@freebsd.org, src-committers@freebsd.org, phk@freebsd.org, cvs-all@freebsd.org Subject: Re: cvs commit: src/sys/i386/cpufreq est.c Message-ID: <200805021103.32849.jhb@freebsd.org> In-Reply-To: <481AEA17.7060702@FreeBSD.org> References: <200802281910.m1SJAgm1083976@repoman.freebsd.org> <200805011728.51152.jhb@freebsd.org> <481AEA17.7060702@FreeBSD.org>
next in thread | previous in thread | raw e-mail | index | archive | help
On Friday 02 May 2008 06:16:55 am Rui Paulo wrote:
> John Baldwin wrote:
> > On Thursday 01 May 2008 05:20:06 pm Rui Paulo wrote:
> >> John Baldwin wrote:
> >>> On Thursday 28 February 2008 02:10:42 pm Rui Paulo wrote:
> >>>> rpaulo 2008-02-28 19:10:42 UTC
> >>>>
> >>>> FreeBSD src repository
> >>>>
> >>>> Modified files:
> >>>> sys/i386/cpufreq est.c
> >>>> Log:
> >>>> Validate the id16 values gathered from ACPI (previously a TODO item).
> >>>> Style changes by me and njl.
> >>> What is the purpose of the 'saved_id16' variable? It is never used. I
> > think
> >>> what might be better is to just read it once at the start of the loop
and
> >>> then restore it at the end of the loop, though phk@ has overwritten this
> > with
> >>> the "chew up all battery on laptops at all cost" patch. :-P
> >>>
> >> What do you mean by 'It is never used.' ?
> >>
> >> % cat -n est.c | grep saved_id16
> >> 1082 uint16_t saved_id16;
> >> 1111 est_get_id16(&saved_id16);
> >
> > Right, it is initialized, but it's value isn't actually _used_ anywhere.
> > There isn't a est_set_id16(&saved_id16), etc.
> >
>
> Yes, you're right. Sorry, it was late yesterday :-)
> The variable is not necessary and has been removed.
I'm not sure that is really the right fix though. I think the systems where
phk had issues may be involved too. First off, I ran into some servers where
6.x was running them at a lower speed yesterday and merely bumping up the
latency from 10 to 1000 fixed those. However, the original speed setting
algorithm looked like this:
for (i = 0; i < count; i++) {
save_current_speed(&saved);
try_speed(speeds[i]);
set_speed(speeds[0]);
}
What this meant is that after the loop the CPU was always set to run at the
first speed in the list, whatever that happened to be. On all the systems
I've seen so far the speeds appear to be ordered from highest to lowest, but
I don't think that is mandated. I'm especially curious to see what the list
was like on phk's problematic server (if it was still slow after the 10 ->
1000 change). I'm betting the system did boot at full speed (the calibration
of the TSC frequency earlier in the boot would confirm this) and that this
loop ended up slowing it down. Also, by setting the speed to the first
speed, you basically made it so laptops booted on battery would run at full
speed hurting battery life if you didn't run powerd. I think the correct
algorithm should be this:
save_current_speed(&saved);
for (i = 0; i < count; i++) {
try_speed(speeds[i]);
}
set_speed(&saved);
This ensures that after the sanity check loop the CPU is restored to whatever
speed it was running at when the system started up. I think the original
code tried to do this, but it had a bug in that it set the speed
to 'speeds[0]' rather than 'saved'. So I guess a patch like this:
Index: est.c
===================================================================
RCS file: /usr/cvs/src/sys/i386/cpufreq/est.c,v
retrieving revision 1.16
diff -u -r1.16 est.c
--- est.c 2 May 2008 10:16:41 -0000 1.16
+++ est.c 2 May 2008 15:01:58 -0000
@@ -1078,7 +1078,8 @@
struct cf_setting *sets;
freq_info *table;
device_t perf_dev;
- int count, error, i, j, maxi, maxfreq;
+ int count, error, i, j;
+ uint16_t saved_id16;
perf_dev = device_find_child(device_get_parent(dev), "acpi_perf", -1);
if (perf_dev == NULL || !device_is_attached(perf_dev))
@@ -1101,7 +1102,7 @@
error = ENOMEM;
goto out;
}
- maxi = maxfreq = 0;
+ est_get_id16(&saved_id16);
for (i = 0, j = 0; i < count; i++) {
/*
* Confirm id16 value is correct.
@@ -1118,24 +1119,11 @@
table[j].id16 = sets[i].spec[0];
table[j].power = sets[i].power;
++j;
- if (sets[i].freq > maxfreq) {
- maxi = i;
- maxfreq = sets[i].freq;
- }
-
}
- /* restore saved setting */
- est_set_id16(dev, sets[i].spec[0], 0);
}
}
- /*
- * Set the frequency to max, so we get through boot fast, and don't
- * handicap systems not running powerd.
- */
- if (maxfreq != 0) {
- device_printf(dev, "Setting %d MHz\n", sets[maxi].freq);
- est_set_id16(dev, sets[maxi].spec[0], 0);
- }
+ /* restore saved setting */
+ est_set_id16(dev, saved_id16, 0);
/* Mark end of table with a terminator. */
bzero(&table[j], sizeof(freq_info));
--
John Baldwin
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?200805021103.32849.jhb>