Skip site navigation (1)Skip section navigation (2)
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>