Date: Mon, 11 Nov 2013 16:31:30 -0500 From: Jung-uk Kim <jkim@FreeBSD.org> To: Adrian Chadd <adrian@freebsd.org> Cc: freebsd-acpi <freebsd-acpi@freebsd.org> Subject: Re: Problems with amd FX 8 core and freq scaling Message-ID: <52814CB2.6050204@FreeBSD.org> In-Reply-To: <CAJ-VmomgCyjX_tR7bLzw6s8jSJJvfB=5fXFCj6UgdKFz6rW-FQ@mail.gmail.com> References: <5281358D.1010406@FreeBSD.org> <5281374F.7080802@FreeBSD.org> <CAJ-VmomgCyjX_tR7bLzw6s8jSJJvfB=5fXFCj6UgdKFz6rW-FQ@mail.gmail.com>
next in thread | previous in thread | raw e-mail | index | archive | help
This is a multi-part message in MIME format. --------------000605010905020407030200 Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit -----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1 On 2013-11-11 15:26:58 -0500, Adrian Chadd wrote: > On 11 November 2013 12:00, Jung-uk Kim <jkim@freebsd.org> wrote: >> -----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1 >> >> On 2013-11-11 13:16:47 -0500, Nicholas McKenzie wrote: >>> But wouldn't this just disable frequency scaling and the whole >>> point of powerd? >> >> No. acpi_throttle (and p4tcc) controls T-state. "Frequency >> scaling" should be done by changing P-state. > > Right. > > IIRC, T-state is just for emergency temperature throttling. It > shouldn't be used outside of that. > > >>>> There have been a number of reports of throttling causing >>>> crashes. This setting does not prevent powerd from adjusting >>>> your CPU's clock, it just disables some arcane feature which >>>> pre-dates the modern power management methods. > > .. did anyone ever figure out why crashes would be caused by > T-state adjustment? My memory is vague but I think it was not able to reject a broken FADT or _PTC table, or something like that. >> I rewrote acpi_throttle.c at some point to fix the problem but >> never committed it because nobody was really interested in >> testing the patch. Also, it is really an arcane and archaic >> feature: >> >> http://software.intel.com/en-us/blogs/2013/10/15/c-states-p-states-where-the-heck-are-those-t-states >> >> >> Now I think we should disable the feature by default because it is >> causing too much hassle for us (attached). Any objection? > > No, I think we should correctly identify which particular features > are required for which particular CPUs and enable/disable it based > on that. > > So, if it's relevant for P4 era hardware, let's default to having > it on for that class hardware. > > If it's not relevant for core and core 2 (and later) hardware, > then let's default to leaving it off for that. If you can maintain p4tcc for Intel processors, I am okay with that. However, I still want to disable acpi_throttle by default. In fact, I've never seen any non-Intel processor and/or motherboard with correct BIOS to support T-state change. > So, how about we come up with a table of CPUs and what particular > power save technologies we should be using, then start > implementing that? Please see p4tcc.c. It already has a quirk table based on CPUID. You just have to maintain it properly. > I'm reading the SDM bits on the adaptive frequency stuff (turbo > boost, etc) and I'll see about writing up some data gathering knobs > for the kernel. Cool. BTW, please don't forget AMD users, i.e., check the BKDGs. http://developer.amd.com/resources/documentation-articles/developer-guides-manuals/ > People still spin up FreeBSD on P4 (and earlier) class hardware. > I'd rather we get it right versus just flipping crap on and off > based on what 'current' hardware expects. I watched people do this > with the RTC code. It's ridiculous. Please see the attached patch, i.e., I reverted p4tcc-specific changes. Jung-uk Kim -----BEGIN PGP SIGNATURE----- Version: GnuPG v2.0.22 (FreeBSD) iQEcBAEBAgAGBQJSgUyyAAoJEHyflib82/FGfLoH/2jejB55Eqtj134Z71bi75MA YUCZ2z5r2PoDN5PUsJeHqyv5EyEWteYlAxLKr/mvV5rbaIk1Wlg0+6c4U7rH99Qj +QpkS1GFL4XQFlKM8pFJ55VxQAYmUVwGCRGJxtxl0z6J6GvCIByKopqV3ywy04eG LcxjML2Kka0FU5UmFKqjy/2j9jBBClEYfynSeVqpjc+REK970oZFMjblQqtLSNsf GKhVwuFwaQYyZZylBTyEZh5fD7346jtA5G/mtSqjKJN2dY6nI5hIqqSWpXulLOEC N16jqUWswO8hE8ZpgVeFSAmkznP4ITHsSjuxQgU4pyTdnF1DiOmizA7Snjekyvs= =S1SR -----END PGP SIGNATURE----- --------------000605010905020407030200 Content-Type: text/x-patch; name="cpu_throttle2.diff" Content-Transfer-Encoding: 7bit Content-Disposition: attachment; filename="cpu_throttle2.diff" Index: sys/dev/acpica/acpi_throttle.c =================================================================== --- sys/dev/acpica/acpi_throttle.c (revision 258002) +++ sys/dev/acpica/acpi_throttle.c (working copy) @@ -167,7 +167,7 @@ static int acpi_throttle_probe(device_t dev) { - if (resource_disabled("acpi_throttle", 0)) + if (!resource_enabled("acpi_throttle", 0)) return (ENXIO); /* Index: sys/kern/subr_hints.c =================================================================== --- sys/kern/subr_hints.c (revision 258002) +++ sys/kern/subr_hints.c (working copy) @@ -449,15 +449,29 @@ resource_find_dev(int *anchor, const char *name, i } /* - * Check to see if a device is disabled via a disabled hint. + * Check to see if a device is disabled or enabled via a hint. */ -int -resource_disabled(const char *name, int unit) +static __inline int +resource_find_hint(const char *name, int unit, const char *hint) { int error, value; - error = resource_int_value(name, unit, "disabled", &value); + error = resource_int_value(name, unit, hint, &value); if (error) return (0); return (value); } + +int +resource_disabled(const char *name, int unit) +{ + + return (resource_find_hint(name, unit, "disabled")); +} + +int +resource_enabled(const char *name, int unit) +{ + + return (resource_find_hint(name, unit, "enabled")); +} Index: sys/sys/bus.h =================================================================== --- sys/sys/bus.h (revision 258002) +++ sys/sys/bus.h (working copy) @@ -503,6 +503,7 @@ int resource_long_value(const char *name, int unit int resource_string_value(const char *name, int unit, const char *resname, const char **result); int resource_disabled(const char *name, int unit); +int resource_enabled(const char *name, int unit); int resource_find_match(int *anchor, const char **name, int *unit, const char *resname, const char *value); int resource_find_dev(int *anchor, const char *name, int *unit, --------------000605010905020407030200--
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?52814CB2.6050204>