On 11/30/2017 14:32, Conrad Meyer wrote: > Hi, > > I don't think this answers the second question about the conditional. > It seems like PCPU_GET() for the initial CPU should be pulled out of > the loop, which binds the thread to a different CPU every iteration.
Ah, good catch. I'll fix it soon. Sorry. > Also, as a side effect of disabling verification, you have fixed PR > 221621, 219213, and probably 218262. Probably. However, I am just trying to fix my FX-8350 and A10-6800 and I don't have Zen processors to verify the MSRs are actually working on those CPUs. Jung-uk Kim > On Thu, Nov 30, 2017 at 11:21 AM, Jung-uk Kim <j...@freebsd.org> wrote: >> On 11/30/2017 13:57, Conrad Meyer wrote: >>> Hi Jung-uk, >>> >>> I have some questions about this change. >>> >>> On Wed, Nov 29, 2017 at 5:40 PM, Jung-uk Kim <j...@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 changing 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 commands >>>> are >>>> requested to the same power domain before completion of pending >>>> transitions, >>>> the last command is executed according to the manual. Because requests >>>> are >>>> serialized by the caller, all cores will receive the same command for >>>> 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 = limit; >>>> >>> >>> Should we bind the thread and record PCPU_GET() here? >>> >>>> + 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 = 0; >>>> CPU_FOREACH(i) { >>>> + if (i == PCPU_GET(cpuid)) >>> >>> 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? >>> >>>> + 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 = 0; j < 100; j++) { >>>> - /* get the result. not assure msr=id */ >>>> - msr = rdmsr(MSR_AMD_10H_11H_STATUS); >>>> - if (msr == id) >>>> - break; >>>> - sbt = 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 = 0; j < 100; j++) { >>>> + /* get the result. not assure msr=id */ >>>> + msr = rdmsr(MSR_AMD_10H_11H_STATUS); >>>> + if (msr == id) >>>> + break; >>>> + sbt = 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 != id) { >>>> + HWPSTATE_DEBUG(dev, >>>> + "error: loop is not enough.\n"); >>> >>> 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. >>> >>>> + return (ENXIO); >>>> + } >>>> } >>>> - HWPSTATE_DEBUG(dev, "result: P%d-state on cpu%d\n", >>>> - (int)msr, PCPU_GET(cpuid)); >>>> - if (msr != id) { >>>> - HWPSTATE_DEBUG(dev, "error: loop is not >>>> enough.\n"); >>>> - error = 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=1" >> and "sysctl debug.hwpstate_verbose=1".
signature.asc
Description: OpenPGP digital signature
