Date: Wed, 06 Jun 2012 19:02:16 -0400 From: Jung-uk Kim <jkim@FreeBSD.org> To: Andriy Gapon <avg@freebsd.org> Cc: freebsd-stable@freebsd.org, Yamagi Burmeister <lists@yamagi.org>, seanbru@yahoo-inc.com Subject: Re: [stable 9] broken hwpstate calls Message-ID: <4FCFE178.9080505@FreeBSD.org> In-Reply-To: <4FCFD2A1.60706@FreeBSD.org> References: <1337319129.2915.4.camel@powernoodle-l7> <4FB6765A.2050307@FreeBSD.org> <1337710214.2916.8.camel@powernoodle-l7.corp.yahoo.com> <20120525163653.b61a08e2.lists@yamagi.org> <4FBFA9A9.7020806@FreeBSD.org> <4FBFBD39.7000105@FreeBSD.org> <4FBFDFFB.9020501@FreeBSD.org> <4FBFE624.1020208@FreeBSD.org> <20120526090233.f638c1d2.lists@yamagi.org> <4FC0A3A1.80200@FreeBSD.org> <4FC7D464.20602@FreeBSD.org> <4FCFD2A1.60706@FreeBSD.org>
next in thread | previous in thread | raw e-mail | index | archive | help
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1
On 2012-06-06 17:58:57 -0400, Andriy Gapon wrote:
> on 31/05/2012 23:28 Jung-uk Kim said the following:
>> It is simple but I don't like locking scheduler, binding CPU, and
>> writing the same MSR, multiple times for each core.
>
> Not sure if parse this. The MSR is _written_ /once/ for each
> core. (BTW, "locking scheduler" is not a completely accurate
> description of what thread_lock does)
I apologize. I didn't see the whole picture and read your patch wrong.
Any way, hwpstate still isn't quite right even without your patch.
sys/kern/kern_cpu.c
cpufreq_curr_sysctl() ->
CPUFREQ_SET() -> /* for all CPU devices */
cf_set_method() -> /* thread_lock(), sched_bind(), ... */
CPUFREQ_DRV_SET() ->
sys/x86/cpufreq/hwpstate.c
hwpstate_set() ->
hwpstate_goto_pstate() /* for each CPU unit */
/* thread_lock(), sched_bind(), ... */
Therefore, "sysctl dev.cpu.0.cpufreq=<freq>" loops n^2 times (i.e., n
times per CPU) where n is number of CPUs. At least, it should check
unit == 0, e.g.,
hwpstate_goto_pstate(...)
{
...
if (unit == 0) {
/* XXX Is this really necessary? */
CPU_FOREACH(i) {
...
wrmsr(MSR_AMD_10H_11H_CONTROL, id);
...
}
}
/* Check the current P-state. */
for (...) {
...
msr = rdmsr(MSR_AMD_10H_11H_STATUS);
if (msr == id)
break;
...
}
/* XXX Maybe your patch here? */
...
}
>> Besides, it introduces more delay and you may be reading the
>> correct status because of that. :-P
>
> Having a separate reading pass does introduce more delay indeed.
> Reading the correct status is a good thing, OTOH.
That's what I said.
> Why would anyone want to read incorrect status? (just want to note
> that "correct" and "expected" are different things)
Okay, okay.
>> If people really think checking MSRC001_0071[18:16] is unworthy
>> for
>
> Well, "other people" hasn't demonstrated/proved/convinced yet that
> it is worthy
>
>> Bulldozer, I prefer skipping status check
>
> That's what I suggested from the very start.
Buy me a Bulldozer and I'll fix it for you! :-P
>> but I disagree with this patch.
>
> Since I am not invested in this issue (I am not affected by the
> problem and I do not have any personal attachment to the code in
> question), I will just defer any decision to those who do care
> about the problem. I hope that a fix will be provided in the end.
Same here.
Jung-uk Kim
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2.0.19 (FreeBSD)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/
iEYEARECAAYFAk/P4XgACgkQmlay1b9qnVP8cgCgl9sAzyE956YjB2B3bK0wvOHu
n64Anih7sdWYQgflQVHuUGstdk05Fs9i
=2dS0
-----END PGP SIGNATURE-----
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?4FCFE178.9080505>