[stable 9] broken hwpstate calls

[Jung-uk Kim]

-----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-----