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

[Oleksandr Tyshchenko]

Hi, Jan.

Sorry for the late response.

On Mon, May 7, 2018 at 6:39 PM, Jan Beulich  wrote:
 On 09.11.17 at 18:09,  wrote:
>> --- a/xen/drivers/cpufreq/Kconfig
>> +++ b/xen/drivers/cpufreq/Kconfig
>> @@ -1,3 +1,6 @@
>>
>>  config HAS_CPUFREQ
>>   bool
>> +
>> +config HAS_CPU_TURBO
>> + bool
>
> This is about cpufreq, so HAS_CPUFREQ_TURBO please.
>
> Also please try to limit the number of #ifdef-s, perhaps by way of introducing
> a few helpers (ending up empty without that setting enabled).

I would like to inform you that we decided to drop this patch. We can
go on without it. Thanks to Stefano
for the valuable comments.

All what we need at the moment regarding "turbo frequencies" is to
"correct the way of defining second_max_freq".
But it is going to be an another patch.

BTW, what do you think about the following:

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 intflags;
 unsigned intindex; /* any */
 unsigned intfrequency; /* 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".

>
> Jan
>
>

-- 
Regards,

Oleksandr Tyshchenko

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

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

[Jan Beulich]

>>> On 09.11.17 at 18:09,  wrote:
> --- a/xen/drivers/cpufreq/Kconfig
> +++ b/xen/drivers/cpufreq/Kconfig
> @@ -1,3 +1,6 @@
>  
>  config HAS_CPUFREQ
>   bool
> +
> +config HAS_CPU_TURBO
> + bool

This is about cpufreq, so HAS_CPUFREQ_TURBO please.

Also please try to limit the number of #ifdef-s, perhaps by way of introducing
a few helpers (ending up empty without that setting enabled).

Jan



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

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

[Oleksandr Tyshchenko]

Hi Stefano

On Tue, Dec 5, 2017 at 12:18 AM, Stefano Stabellini
 wrote:
> On Sat, 2 Dec 2017, Oleksandr Tyshchenko wrote:
>> On Sat, Dec 2, 2017 at 3:06 AM, Stefano Stabellini
>>  wrote:
>> > On Thu, 9 Nov 2017, Oleksandr Tyshchenko wrote:
>> >> From: Oleksandr Dmytryshyn 
>> >>
>> >> 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 
>> >> Signed-off-by: Oleksandr Tyshchenko 
>> >> CC: Jan Beulich 
>> >> CC: Andrew Cooper 
>> >> CC: Stefano Stabellini 
>> >> CC: Julien Grall 
>> >> ---
>> >>  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: %usecond_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.
>
> Right
>
>
>> cpufreq_update_turbo() will return -EOPNOTSUPP 

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

[Stefano Stabellini]

On Sat, 2 Dec 2017, Oleksandr Tyshchenko wrote:
> On Sat, Dec 2, 2017 at 3:06 AM, Stefano Stabellini
>  wrote:
> > On Thu, 9 Nov 2017, Oleksandr Tyshchenko wrote:
> >> From: Oleksandr Dmytryshyn 
> >>
> >> 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 
> >> Signed-off-by: Oleksandr Tyshchenko 
> >> CC: Jan Beulich 
> >> CC: Andrew Cooper 
> >> CC: Stefano Stabellini 
> >> CC: Julien Grall 
> >> ---
> >>  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: %usecond_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.

Right


> 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".

Exactly what I was thinking


> Another question is second_max_freq. As I understand, it is 

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

[Andre Przywara]

Hi,



> 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.

So traditionally on ARM there is no notion of a "turbo" frequency. The
idea is to expose the highest possible frequency, and let thermal
throttling (possibly in hardware or in firmware) limit the frequency if
the thermal budget is busted.
Also in the ARM world it is expected that an OS has much better
knowledge on how to handle frequencies, for instance when to give more
power to the GPU and when to the CPU.

> 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.

The DT binding you mentioned in Linux is a generic one.
In general DT only describes non-discoverable properties. But for SCPI
the OPPs are handled in the SCP and advertised via SCPI calls (3.2.9 Get
DVFS Info, command 0x9).
So the OPP table is not in the DT, and thus you don't have any way of
detecting turbo frequencies.
But as mentioned before, this is so by design, as ARM does not endorse
the concept of turbo frequencies in general.

Now with the advent of more "server-y" chips and ACPI, this might change
in the future. For instance SCMI is designed to be closer to ACPI, so we
might inherit some turbo notion from there.

So we should not completely rule out the idea of turbo, but for a start
we can somewhat assume that an ARM based system does not have turbo per se.

Cheers,
Andre.

> [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)
>>>  >u.get_para.u.ondemand.sampling_rate,
>>>  >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 intmax_freq;
>>> +#ifdef CONFIG_HAS_CPU_TURBO
>>>  unsigned intsecond_max_freq;/* P1 if Turbo Mode is on */
>>> +#endif
>>>  unsigned intmin_freq;
>>>  unsigned inttransition_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)
> 
> 
> 

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