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

[Conrad Meyer]

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
>

Best,
Conrad