From owner-cvs-all@FreeBSD.ORG Fri May 2 15:35:24 2008 Return-Path: Delivered-To: cvs-all@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:4f8:fff6::34]) by hub.freebsd.org (Postfix) with ESMTP id B08411065670; Fri, 2 May 2008 15:35:24 +0000 (UTC) (envelope-from jhb@freebsd.org) Received: from elvis.mu.org (elvis.mu.org [192.203.228.196]) by mx1.freebsd.org (Postfix) with ESMTP id 91D918FC1C; Fri, 2 May 2008 15:35:24 +0000 (UTC) (envelope-from jhb@freebsd.org) Received: from server.baldwin.cx (unknown [208.65.91.234]) by elvis.mu.org (Postfix) with ESMTP id 09CD11A4D80; Fri, 2 May 2008 08:35:23 -0700 (PDT) Received: from localhost.corp.yahoo.com (john@localhost [127.0.0.1]) (authenticated bits=0) by server.baldwin.cx (8.14.2/8.14.2) with ESMTP id m42FZBZu035044; Fri, 2 May 2008 11:35:11 -0400 (EDT) (envelope-from jhb@freebsd.org) From: John Baldwin To: Rui Paulo Date: Fri, 2 May 2008 11:03:32 -0400 User-Agent: KMail/1.9.7 References: <200802281910.m1SJAgm1083976@repoman.freebsd.org> <200805011728.51152.jhb@freebsd.org> <481AEA17.7060702@FreeBSD.org> In-Reply-To: <481AEA17.7060702@FreeBSD.org> MIME-Version: 1.0 Content-Type: text/plain; charset="iso-8859-15" Content-Transfer-Encoding: 7bit Content-Disposition: inline Message-Id: <200805021103.32849.jhb@freebsd.org> X-Greylist: Sender succeeded SMTP AUTH authentication, not delayed by milter-greylist-2.0.2 (server.baldwin.cx [127.0.0.1]); Fri, 02 May 2008 11:35:12 -0400 (EDT) X-Virus-Scanned: ClamAV 0.91.2/7011/Fri May 2 09:11:57 2008 on server.baldwin.cx X-Virus-Status: Clean X-Spam-Status: No, score=-4.4 required=4.2 tests=ALL_TRUSTED,AWL,BAYES_00 autolearn=ham version=3.1.3 X-Spam-Checker-Version: SpamAssassin 3.1.3 (2006-06-01) on server.baldwin.cx 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 X-BeenThere: cvs-all@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: CVS commit messages for the entire tree List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 02 May 2008 15:35:24 -0000 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