Re: [Xen-devel] [RFC PATCH 04/31] cpufreq: make turbo settings to be configurable

[Oleksandr Tyshchenko <olekstysh@xxxxxxxxx>]

Hi Stefano

On Sat, Dec 2, 2017 at 3:06 AM, Stefano Stabellini
<sstabellini@xxxxxxxxxx> wrote:
> On Thu, 9 Nov 2017, Oleksandr Tyshchenko wrote:
>> From: Oleksandr Dmytryshyn <oleksandr.dmytryshyn@xxxxxxxxxxxxxxx>
>>
>> This settings is not needed for some architectures.
>> So make it to be configurable and use it for x86
>> architecture.
>>
>> This is a rebased version of the original patch:
>> https://lists.xen.org/archives/html/xen-devel/2014-11/msg00942.html
>>
>> Signed-off-by: Oleksandr Dmytryshyn <oleksandr.dmytryshyn@xxxxxxxxxxxxxxx>
>> Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@xxxxxxxx>
>> CC: Jan Beulich <jbeulich@xxxxxxxx>
>> CC: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
>> CC: Stefano Stabellini <sstabellini@xxxxxxxxxx>
>> CC: Julien Grall <julien.grall@xxxxxxxxxx>
>> ---
>>  xen/arch/x86/Kconfig          |  1 +
>>  xen/drivers/cpufreq/Kconfig   |  3 +++
>>  xen/drivers/cpufreq/utility.c | 11 ++++++++++-
>>  xen/drivers/pm/stat.c         |  6 ++++++
>>  xen/include/xen/cpufreq.h     |  6 ++++++
>>  5 files changed, 26 insertions(+), 1 deletion(-)
>>
>> diff --git a/xen/arch/x86/Kconfig b/xen/arch/x86/Kconfig
>> index 86c8eca..c1eac1d 100644
>> --- a/xen/arch/x86/Kconfig
>> +++ b/xen/arch/x86/Kconfig
>> @@ -24,6 +24,7 @@ config X86
>>       select NUMA
>>       select VGA
>>       select HAS_PM
>> +     select HAS_CPU_TURBO
>>
>>  config ARCH_DEFCONFIG
>>       string
>> diff --git a/xen/drivers/cpufreq/Kconfig b/xen/drivers/cpufreq/Kconfig
>> index cce80f4..427ea2a 100644
>> --- a/xen/drivers/cpufreq/Kconfig
>> +++ b/xen/drivers/cpufreq/Kconfig
>> @@ -1,3 +1,6 @@
>>
>>  config HAS_CPUFREQ
>>       bool
>> +
>> +config HAS_CPU_TURBO
>> +     bool
>> diff --git a/xen/drivers/cpufreq/utility.c b/xen/drivers/cpufreq/utility.c
>> index a687e5a..25bf983 100644
>> --- a/xen/drivers/cpufreq/utility.c
>> +++ b/xen/drivers/cpufreq/utility.c
>> @@ -209,7 +209,9 @@ int cpufreq_frequency_table_cpuinfo(struct 
>> cpufreq_policy *policy,
>>  {
>>      unsigned int min_freq = ~0;
>>      unsigned int max_freq = 0;
>> +#ifdef CONFIG_HAS_CPU_TURBO
>>      unsigned int second_max_freq = 0;
>> +#endif
>>      unsigned int i;
>>
>>      for (i=0; (table[i].frequency != CPUFREQ_TABLE_END); i++) {
>> @@ -221,6 +223,7 @@ int cpufreq_frequency_table_cpuinfo(struct 
>> cpufreq_policy *policy,
>>          if (freq > max_freq)
>>              max_freq = freq;
>>      }
>> +#ifdef CONFIG_HAS_CPU_TURBO
>>      for (i=0; (table[i].frequency != CPUFREQ_TABLE_END); i++) {
>>          unsigned int freq = table[i].frequency;
>>          if (freq == CPUFREQ_ENTRY_INVALID || freq == max_freq)
>> @@ -234,9 +237,13 @@ int cpufreq_frequency_table_cpuinfo(struct 
>> cpufreq_policy *policy,
>>          printk("max_freq: %u    second_max_freq: %u\n",
>>                 max_freq, second_max_freq);
>>
>> +    policy->cpuinfo.second_max_freq = second_max_freq;
>> +#else /* !CONFIG_HAS_CPU_TURBO */
>> +    if (cpufreq_verbose)
>> +        printk("max_freq: %u\n", max_freq);
>> +#endif /* CONFIG_HAS_CPU_TURBO */
>>      policy->min = policy->cpuinfo.min_freq = min_freq;
>>      policy->max = policy->cpuinfo.max_freq = max_freq;
>> -    policy->cpuinfo.second_max_freq = second_max_freq;
>>
>>      if (policy->min == ~0)
>>          return -EINVAL;
>> @@ -390,6 +397,7 @@ int cpufreq_driver_getavg(unsigned int cpu, unsigned int 
>> flag)
>>      return policy->cur;
>>  }
>>
>> +#ifdef CONFIG_HAS_CPU_TURBO
>>  int cpufreq_update_turbo(int cpuid, int new_state)
>>  {
>>      struct cpufreq_policy *policy;
>> @@ -430,6 +438,7 @@ int cpufreq_get_turbo_status(int cpuid)
>>      policy = per_cpu(cpufreq_cpu_policy, cpuid);
>>      return policy && policy->turbo == CPUFREQ_TURBO_ENABLED;
>>  }
>> +#endif /* CONFIG_HAS_CPU_TURBO */
>>
>>  /*********************************************************************
>>   *                 POLICY                                            *
>
> I am wondering if we need to go as far as #ifdef'ing
> cpufreq_update_turbo. For the sake of reducing the number if #ifdef's,
> would it be enough if we only make sure it is disabled?
>
> In other words, I would keep the changes to stat.c but I would leave
> utility.c and cpufreq.h pretty much untouched.

Yes. I was thinking about dropping this patch at all. If platform
doesn't support CPU Boost, the platform
driver should just inform framework about that (policy->turbo =
CPUFREQ_TURBO_UNSUPPORTED).
That's all.

cpufreq_update_turbo() will return -EOPNOTSUPP if someone tries to
enable/disable turbo mode.
cpufreq_get_turbo_status() will return that turbo mode "is not enabled".

Another question is second_max_freq. As I understand, it is highest
non-turbo frequency calculated by framework to limit target frequency
when
turbo mode "is disabled". And Xen assumes that second_max_freq is
always P1 if turbo mode is on.
But, there might be a case when a few highest frequencies are
turbo-frequencies. So, I propose to add an extra flag for handling
that.
So, each CPUFreq driver responsibility will be to mark
turbo-frequency(ies) for the framework to properly calculate
second_max_freq.

Something like that:

diff --git a/xen/drivers/cpufreq/utility.c b/xen/drivers/cpufreq/utility.c
index 25bf983..122a88b 100644
--- a/xen/drivers/cpufreq/utility.c
+++ b/xen/drivers/cpufreq/utility.c
@@ -226,7 +226,8 @@ int cpufreq_frequency_table_cpuinfo(struct
cpufreq_policy *policy,
 #ifdef CONFIG_HAS_CPU_TURBO
     for (i=0; (table[i].frequency != CPUFREQ_TABLE_END); i++) {
         unsigned int freq = table[i].frequency;
-        if (freq == CPUFREQ_ENTRY_INVALID || freq == max_freq)
+        if ((freq == CPUFREQ_ENTRY_INVALID) ||
+            (table[i].flags & CPUFREQ_BOOST_FREQ))
             continue;
         if (freq > second_max_freq)
             second_max_freq = freq;
diff --git a/xen/include/xen/cpufreq.h b/xen/include/xen/cpufreq.h
index 2e0c16a..77b29da 100644
--- a/xen/include/xen/cpufreq.h
+++ b/xen/include/xen/cpufreq.h
@@ -204,7 +204,11 @@ void cpufreq_verify_within_limits(struct
cpufreq_policy *policy,
 #define CPUFREQ_ENTRY_INVALID ~0
 #define CPUFREQ_TABLE_END     ~1

+/* Special Values of .flags field */
+#define CPUFREQ_BOOST_FREQ    (1 << 0)
+
 struct cpufreq_frequency_table {
+       unsigned int    flags;
     unsigned int    index;     /* any */
     unsigned int    frequency; /* kHz - doesn't need to be in ascending
                                 * order */

Both existing on x86 CPUFreq drivers just need to mark P0 frequency as
a turbo-frequency if turbo mode "is supported". Am I correct?

And the most important question is how to recognize in Xen on ARM
(using SCPI protocol) which frequencies are turbo-frequencies
actually? I couldn't find any information regarding that in protocol
description.
For DT-based CPUFreq it is not an issue, since there is a specific
property "turbo-mode" to mark corresponding OPPs. [1].
But neither SCPI DT bindings [2] nor the SCPI protocol itself [3]
mentions about it. Perhaps, additional command should be added to pass
such info.

[1] https://www.kernel.org/doc/Documentation/devicetree/bindings/opp/opp.txt
[2] 
http://elixir.free-electrons.com/linux/v4.15-rc1/source/Documentation/devicetree/bindings/arm/arm,scpi.txt
[3] 
http://infocenter.arm.com/help/topic/com.arm.doc.dui0922g/scp_message_interface_v1_2_DUI0922G_en.pdf

>
>
>> diff --git a/xen/drivers/pm/stat.c b/xen/drivers/pm/stat.c
>> index 2dbde1c..133e64d 100644
>> --- a/xen/drivers/pm/stat.c
>> +++ b/xen/drivers/pm/stat.c
>> @@ -290,7 +290,11 @@ static int get_cpufreq_para(struct xen_sysctl_pm_op *op)
>>              &op->u.get_para.u.ondemand.sampling_rate,
>>              &op->u.get_para.u.ondemand.up_threshold);
>>      }
>> +#ifdef CONFIG_HAS_CPU_TURBO
>>      op->u.get_para.turbo_enabled = cpufreq_get_turbo_status(op->cpuid);
>> +#else
>> +    op->u.get_para.turbo_enabled = 0;
>> +#endif
>>
>>      return ret;
>>  }
>> @@ -473,6 +477,7 @@ int do_pm_op(struct xen_sysctl_pm_op *op)
>>          break;
>>      }
>>
>> +#ifdef CONFIG_HAS_CPU_TURBO
>>      case XEN_SYSCTL_pm_op_enable_turbo:
>>      {
>>          ret = cpufreq_update_turbo(op->cpuid, CPUFREQ_TURBO_ENABLED);
>> @@ -484,6 +489,7 @@ int do_pm_op(struct xen_sysctl_pm_op *op)
>>          ret = cpufreq_update_turbo(op->cpuid, CPUFREQ_TURBO_DISABLED);
>>          break;
>>      }
>> +#endif /* CONFIG_HAS_CPU_TURBO */
>>
>>      default:
>>          printk("not defined sub-hypercall @ do_pm_op\n");
>> diff --git a/xen/include/xen/cpufreq.h b/xen/include/xen/cpufreq.h
>> index 30c70c9..2e0c16a 100644
>> --- a/xen/include/xen/cpufreq.h
>> +++ b/xen/include/xen/cpufreq.h
>> @@ -39,7 +39,9 @@ extern struct acpi_cpufreq_data *cpufreq_drv_data[NR_CPUS];
>>
>>  struct cpufreq_cpuinfo {
>>      unsigned int        max_freq;
>> +#ifdef CONFIG_HAS_CPU_TURBO
>>      unsigned int        second_max_freq;    /* P1 if Turbo Mode is on */
>> +#endif
>>      unsigned int        min_freq;
>>      unsigned int        transition_latency; /* in 10^(-9) s = nanoseconds */
>>  };
>> @@ -72,9 +74,11 @@ struct cpufreq_policy {
>>
>>      bool_t              resume; /* flag for cpufreq 1st run
>>                                   * S3 wakeup, hotplug cpu, etc */
>> +#ifdef CONFIG_HAS_CPU_TURBO
>>      s8                  turbo;  /* tristate flag: 0 for unsupported
>>                                   * -1 for disable, 1 for enabled
>>                                   * See CPUFREQ_TURBO_* below for defines */
>> +#endif
>>      bool_t              aperf_mperf; /* CPU has APERF/MPERF MSRs */
>>  };
>>  DECLARE_PER_CPU(struct cpufreq_policy *, cpufreq_cpu_policy);
>> @@ -138,8 +142,10 @@ extern int cpufreq_driver_getavg(unsigned int cpu, 
>> unsigned int flag);
>>  #define CPUFREQ_TURBO_UNSUPPORTED   0
>>  #define CPUFREQ_TURBO_ENABLED       1
>>
>> +#ifdef CONFIG_HAS_CPU_TURBO
>>  extern int cpufreq_update_turbo(int cpuid, int new_state);
>>  extern int cpufreq_get_turbo_status(int cpuid);
>> +#endif
>>
>>  static __inline__ int
>>  __cpufreq_governor(struct cpufreq_policy *policy, unsigned int event)



-- 
Regards,

Oleksandr Tyshchenko

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel