cvs commit: src/sys/i386/cpufreq est.c

[John Baldwin]

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