svn commit: r326383 - head/sys/x86/cpufreq

[Conrad Meyer]

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.

Also, as a side effect of disabling verification, you have fixed PR
221621, 219213, and probably 218262.

Best,
Conrad

On Thu, Nov 30, 2017 at 11:21 AM, Jung-uk Kim <jkim at 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 <jkim at 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".
>
> Jung-uk Kim
>