Date: Thu, 30 Nov 2017 14:21:34 -0500 From: Jung-uk Kim <jkim@FreeBSD.org> To: cem@freebsd.org Cc: src-committers <src-committers@freebsd.org>, svn-src-all@freebsd.org, svn-src-head@freebsd.org Subject: Re: svn commit: r326383 - head/sys/x86/cpufreq Message-ID: <e412e02d-8b52-221f-1f3c-8c300b1b3067@FreeBSD.org> In-Reply-To: <CAG6CVpVyooSG3rPG1GvqRZHT9Lyh48JBQb_tFf2-CzWmfW8JyQ@mail.gmail.com> References: <201711300140.vAU1e7dC001292@repo.freebsd.org> <CAG6CVpVyooSG3rPG1GvqRZHT9Lyh48JBQb_tFf2-CzWmfW8JyQ@mail.gmail.com>
next in thread | previous in thread | raw e-mail | index | archive | help
This is an OpenPGP/MIME signed message (RFC 4880 and 3156) --uNDKpI0H5TJeWUu4mGoBxApilSIaXqd4O Content-Type: multipart/mixed; boundary="QQQmE4AGqIXsSaAu0KRvDTkpHD31nvbjk"; protected-headers="v1" From: Jung-uk Kim <jkim@FreeBSD.org> To: cem@freebsd.org Cc: src-committers <src-committers@freebsd.org>, svn-src-all@freebsd.org, svn-src-head@freebsd.org Message-ID: <e412e02d-8b52-221f-1f3c-8c300b1b3067@FreeBSD.org> Subject: Re: svn commit: r326383 - head/sys/x86/cpufreq References: <201711300140.vAU1e7dC001292@repo.freebsd.org> <CAG6CVpVyooSG3rPG1GvqRZHT9Lyh48JBQb_tFf2-CzWmfW8JyQ@mail.gmail.com> In-Reply-To: <CAG6CVpVyooSG3rPG1GvqRZHT9Lyh48JBQb_tFf2-CzWmfW8JyQ@mail.gmail.com> --QQQmE4AGqIXsSaAu0KRvDTkpHD31nvbjk Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: quoted-printable On 11/30/2017 13:57, Conrad Meyer wrote: > Hi Jung-uk, >=20 > I have some questions about this change. >=20 > On Wed, Nov 29, 2017 at 5:40 PM, Jung-uk Kim <jkim@freebsd.org> wrote: >> Author: jkim >> Date: Thu Nov 30 01:40:07 2017 >> New Revision: 326383 >> URL: https://svnweb.freebsd.org/changeset/base/326383 >> >> Log: >> Add a tunable "debug.hwpstate_verify" to check P-state after changin= g it and >> turn it off by default. It is very inefficient to verify current P-= state of >> each core, especially for CPUs with many cores. When multiple comma= nds are >> requested to the same power domain before completion of pending tran= sitions, >> the last command is executed according to the manual. Because reque= sts are >> serialized by the caller, all cores will receive the same command fo= r each >> call. Do not call sched_bind() and sched_unbind(). It is redundant= because >> the caller does it anyway. >> ... >> @@ -176,47 +178,57 @@ hwpstate_goto_pstate(device_t dev, int pstate) >> if (limit > id) >> id =3D limit; >> >=20 > Should we bind the thread and record PCPU_GET() here? >=20 >> + HWPSTATE_DEBUG(dev, "setting P%d-state on cpu%d\n", id, >> + PCPU_GET(cpuid)); >> + /* Go To Px-state */ >> + wrmsr(MSR_AMD_10H_11H_CONTROL, id); >> + >> /* >> * We are going to the same Px-state on all cpus. >> * Probably should take _PSD into account. >> */ >> - error =3D 0; >> CPU_FOREACH(i) { >> + if (i =3D=3D PCPU_GET(cpuid)) >=20 > It seems like this check could evaluate to a different CPU every time? > When really we are trying to avoid setting on the CPU we set on > initially above? >=20 >> + continue; >> + >> /* Bind to each cpu. */ >> thread_lock(curthread); >> sched_bind(curthread, i); >> thread_unlock(curthread); >> - HWPSTATE_DEBUG(dev, "setting P%d-state on cpu%d\n", >> - id, PCPU_GET(cpuid)); >> + HWPSTATE_DEBUG(dev, "setting P%d-state on cpu%d\n", id= , i); >> /* Go To Px-state */ >> wrmsr(MSR_AMD_10H_11H_CONTROL, id); >> } >> - CPU_FOREACH(i) { >> - /* Bind to each cpu. */ >> - thread_lock(curthread); >> - sched_bind(curthread, i); >> - thread_unlock(curthread); >> - /* wait loop (100*100 usec is enough ?) */ >> - for (j =3D 0; j < 100; j++) { >> - /* get the result. not assure msr=3Did */ >> - msr =3D rdmsr(MSR_AMD_10H_11H_STATUS); >> - if (msr =3D=3D id) >> - break; >> - sbt =3D SBT_1MS / 10; >> - tsleep_sbt(dev, PZERO, "pstate_goto", sbt, >> - sbt >> tc_precexp, 0); >> + >> + /* >> + * Verify whether each core is in the requested P-state. >> + */ >> + if (hwpstate_verify) { >> + CPU_FOREACH(i) { >> + thread_lock(curthread); >> + sched_bind(curthread, i); >> + thread_unlock(curthread); >> + /* wait loop (100*100 usec is enough ?) */ >> + for (j =3D 0; j < 100; j++) { >> + /* get the result. not assure msr=3Did= */ >> + msr =3D rdmsr(MSR_AMD_10H_11H_STATUS);= >> + if (msr =3D=3D id) >> + break; >> + sbt =3D SBT_1MS / 10; >> + tsleep_sbt(dev, PZERO, "pstate_goto", = sbt, >> + sbt >> tc_precexp, 0); >> + } >> + HWPSTATE_DEBUG(dev, "result: P%d-state on cpu%= d\n", >> + (int)msr, i); >> + if (msr !=3D id) { >> + HWPSTATE_DEBUG(dev, >> + "error: loop is not enough.\n"); >=20 > In this error return, should the current thread be unbinded? The old > code did this by setting error and falling through to the ordinary > exit path. We could use 'goto out;' to avoid looping through the rest > of the CPUs. >=20 >> + return (ENXIO); >> + } >> } >> - HWPSTATE_DEBUG(dev, "result: P%d-state on cpu%d\n", >> - (int)msr, PCPU_GET(cpuid)); >> - if (msr !=3D id) { >> - HWPSTATE_DEBUG(dev, "error: loop is not enough= =2E\n"); >> - error =3D ENXIO; >> - } >> } >> - thread_lock(curthread); >> - sched_unbind(curthread); >> - thread_unlock(curthread); >> - return (error); >> + >> + return (0); >> } >> >> static int >> This driver is only called via cpufreq(4) (i.e., sys/kern/kern_cpu.c), and sched_bind()/sched_unbind() is done from cf_set_method() for cpu0. If you want to see the sequence, try "sysctl debug.cpufreq.verbose=3D1" and "sysctl debug.hwpstate_verbose=3D1". Jung-uk Kim --QQQmE4AGqIXsSaAu0KRvDTkpHD31nvbjk-- --uNDKpI0H5TJeWUu4mGoBxApilSIaXqd4O Content-Type: application/pgp-signature; name="signature.asc" Content-Description: OpenPGP digital signature Content-Disposition: attachment; filename="signature.asc" -----BEGIN PGP SIGNATURE----- iQEzBAEBCAAdFiEEl1bqgKaRyqfWXu/CfJ+WJvzb8UYFAlogWkMACgkQfJ+WJvzb 8UZsGwf9FZhjwSxr7g0Bljp1g2Lj/qEGfZZ+zjVDwT5daKsrGIgfQ9Z6vdwO8mM0 9NCTZkX8R4V9gvjR36wRCmYSC3iMm22aB3TBsQJlqEwMqPbJmaa/Blp3xuFOogEw kKoeZ3XD8KHlcmbVkdBxqkvW3Tyg9AgB5lWbEjbPXuc8dcFFs9r8h3CiY5Q0FUlw yDGB1Y6BrY0O8v+veAi2Btv39tWJ8FVqQ+R6gRAaSKsiNuJC1ogzWwfPd26uY5// E1OWSdAd+IG5WWbb2EXJk+O9gvXZ2v+6EiBuOIGlMP0bnS3lCnF+KyRp8JlKf4h7 zu3L1nEI8SO47PR7G2UeTDLpyjhkoQ== =+7U0 -----END PGP SIGNATURE----- --uNDKpI0H5TJeWUu4mGoBxApilSIaXqd4O--
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?e412e02d-8b52-221f-1f3c-8c300b1b3067>