Porting the intel_pstate driver to Xen
v6 cover Porting the intel_pstate driver to Xen
v6 changes:
Changes include patch re-organization, data type change, coding style change etc. Please go to each patch for details.
v5 changes:
We have made various changes in this version, including introducing new data structures, coding styles changes etc. Please see each patch's commit message for change details.
v4 changes:
1) introduce a new struct, internal_governor, to "cpufreq_policy";
2) add a new header file, xen/include/asm-x86/cpufreq.h;
3) remove the APERF/MPERF feature detection code in cpufreq.c and powernow.c;
4) coding style changes.
Please check each patch's commit message for details.
v3 Changes:
1) coding style changes based on Jan's comments;
2) remove the function - unregister_cpu_notifier();
3) solve a bug in the CPU offline code (Patch 0007);
4) move the perf_limits struct into the per-CPU policy struct, so that each CPU can be managed individually;
5) "load_intel_pstate" is changed local to the intel_pstate.c file, and add its description to the xen-command-line.markdown.
v2 Changes:
1) The intel_pstate driver can be controlled via two ways:
A. min_perf_pct and max_perf_pct
The user directly adjusts min_perf_pct and max_perf_pct to get what they want. For example, if min_perf_pct=max_perf_pct=60%, then the user is asking for something similar to a userspace governor with setting the requested performance=60%.
B. set-scaling-governor
This one is functionally redundant, since A. can achieve all the governor functions. It is remained to give people time to get familiar with method A.
Users can choose from the four governors: Powersave, Ondemand, Powersave, Performance. The driver achieves the functionality of the selected governor via adjusting the min_perf_pct and max_perf_pct itself.
2) The xenpm "get-cpufreq-para" displays the following things:
cpu id : 10affected_cpus : 10
cpuinfo frequency : max [3700000] min [1200000] cur [1400000]scaling_driver : intel_pstatescaling_avail_gov : performance powersave userspace ondemandcurrent_governor : ondemandmax_perf_pct : 100min_perf_pct : 32turbo_pct : 54
turbo mode : enabled
3) Changed "intel_pstate=disable" to "intel_pstate=enable".
If "intel_pstate=enable" is added, but the CPU does not support the intel_pstate driver, the old P-state driver (acpi-cpufreq) will be loaded.
4) Moved the declarations under xen/include/acpi to an x86-specific header.
v1:
This patch series ports the intel_pstate driver from the Linux kernel to Xen. The intel_pstate driver is used to tune P states for SandyBridge+ processors. It needs to be enabled by adding "intel_pstate=enable" to the booting parameter list.
The intel_pstate.c file under xen/arch/x86/acpi/cpufreq/ contains all the logic for selecting the current P-state. It follows its implementation in the kernel. In order to better support future Intel CPUs (e.g. the HWP feature on Skylake+), intel_pstate changes to tune P-state based on percentage values.
The xenpm tool is also upgraded to support the intel_pstate driver. If intel_pstate is used, "get-cpufreq-para" displays percentage value based feedback. If the intel_pstate driver is not enabled, xenpm will work in the old style.
Wei Wang (6):
x86/intel_pstate: add some calculation related support x86/intel_pstate: introduce the internal_governor struct x86/intel_pstate: the main body of the intel_pstate driver x86/intel_pstate: add a booting param to select the driver to load x86/intel_pstate: support the use of intel_pstate in pmstat.c
tools: enable xenpm to control the intel_pstate driver
docs/misc/xen-command-line.markdown | 7 + tools/libxc/include/xenctrl.h | 20 +- tools/libxc/xc_pm.c | 16 +- tools/misc/xenpm.c | 116 +++- xen/arch/x86/acpi/cpufreq/Makefile | 1 + xen/arch/x86/acpi/cpufreq/cpufreq.c | 15 +- xen/arch/x86/acpi/cpufreq/intel_pstate.c | 882 +++++++++++++++++++++++++++++++ xen/arch/x86/oprofile/op_model_athlon.c | 9 - xen/drivers/acpi/pmstat.c | 183 ++++++- xen/drivers/cpufreq/cpufreq.c | 5 +- xen/include/acpi/cpufreq/cpufreq.h | 15 + xen/include/asm-x86/cpufreq.h | 33 ++ xen/include/asm-x86/div64.h | 91 ++++ xen/include/asm-x86/msr-index.h | 3 + xen/include/public/sysctl.h | 29 +- xen/include/xen/kernel.h | 23 + 16 files changed, 1356 insertions(+), 92 deletions(-) create mode 100644 xen/arch/x86/acpi/cpufreq/intel_pstate.c create mode 100644 xen/include/asm-x86/cpufreq.h
Signature · 1 line
1.9.1
PATCH v6 1/6 x86/intel_pstate: add some calculation related support
The added calculation related functions will be used in the intel_pstate.c. They are copied from the Linux kernel(commit 2418f4f2, f3002134, eb18cba7).
---
changes in v6:
1) #define clamp() - remove the typecast on the result of max();
2) add some comments to explain the functionality of some functions;
3) alignment changes.
xen/arch/x86/oprofile/op_model_athlon.c | 9 ---- xen/include/asm-x86/div64.h | 91 +++++++++++++++++++++++++++++++++ xen/include/xen/kernel.h | 23 +++++++++ 3 files changed, 114 insertions(+), 9 deletions(-)
xen/arch/x86/oprofile/op_model_athlon.c +0 -9
@@ -103,15 +103,6 @@ static u64 ibs_op_ctl;#define IBS_FETCH_CODE 13#define IBS_OP_CODE 14 -#define clamp (val, min, max) ({ \ - typeof(val) __val = (val); \ - typeof(min) __min = (min); \ - typeof(max) __max = (max); \ - (void) (&__val == &__min); \ - (void) (&__val == &__max); \ - __val = __val < __min ? __min: __val; \ - __val > __max ? __max: __val; }) -/* * 16-bit Linear Feedback Shift Register (LFSR) */
xen/include/asm-x86/div64.h +91 -0
95 lines not on this page · raw message
xen/include/xen/kernel.h +23 -0
@@ -43,6 +43,29 @@#define MAX (x,y) ((x) > (y) ? (x) : (y))/** +* clamp - return a value clamped to a given range with strict typechecking +* @val: current value +* @lo: lowest allowable value +* @hi: highest allowable value +* +* This macro does strict typechecking of lo/hi to make sure they are of the +* same type as val. See the unnecessary pointer comparisons. +*/ +#define clamp (val, lo, hi) min(max(val, lo), hi) + +/* +* clamp_t - return a value clamped to a given range using a given type +* @type: the type of variable to use +* @val: current value +* @lo: minimum allowable value +* @hi: maximum allowable value +* +* This macro does no typechecking and uses temporary variables of type +* 'type' to make all the comparisons. +*/ +#define clamp_t (type, val, lo, hi) min_t(type, max_t(type, val, lo), hi) + +/** * container_of - cast a member of a structure out to the containing structure * * @ptr: the pointer to the member.
Signature · 1 line
1.9.1
Re: [PATCH v6 1/6] x86/intel_pstate: add some calculation related support
xen/include/asm-x86/div64.h
Quoted 3 lines
Quoted 1 line
Quoted 1 line · On 28.10.15 at 04:21, <[email protected]> wrote:
xen/include/asm-x86/div64.h +0
-0
Considering you don't even do this in common code, why did you pick the BITS_PER_LONG == 32 variants from Linux? Would make for quite a bit smaller a patch and quite a bit better easier to read code.
Jan
PATCH v6 2/6 x86/intel_pstate: introduce the internal_governor struct
In reply to Wei WangIntroduce a simple internal_governor struct to manage internal governor related variables. Also, add a condition check in cpufreq_del_cpu to avoid going through the old ACPI governor framework when an internal governor is in use.
---
changes in v6:
1) create this patch by spliting it from the next big one.
xen/drivers/cpufreq/cpufreq.c | 5 +++-- xen/include/acpi/cpufreq/cpufreq.h | 7 +++++++ 2 files changed, 10 insertions(+), 2 deletions(-)
xen/drivers/cpufreq/cpufreq.c +3 -2
@@ -334,8 +334,9 @@ int cpufreq_del_cpu(unsigned int cpu)/* for HW_ALL, stop gov for each core of the _PSD domain */ /* for SW_ALL & SW_ANY, stop gov for the 1st core of the _PSD domain */ -if (hw_all || (cpumask_weight (cpufreq_dom->map) == - perf->domain_info.num_processors)) +if (!policy->internal_gov && + (hw_all || (cpumask_weight (cpufreq_dom->map) == + perf->domain_info.num_processors)))__cpufreq_governor (policy,CPUFREQ_GOV_STOP );cpufreq_statistic_exit (cpu);
xen/include/acpi/cpufreq/cpufreq.h +7 -0
@@ -53,6 +53,12 @@ struct perf_limits {uint32_t min_policy_pct; }; +struct internal_governor { +char *avail_gov; +uint32_t gov_num; +uint32_t cur_gov; +}; +struct cpufreq_policy {cpumask_var_t cpus;/* affected CPUs */ unsigned int shared_type;/* ANY or ALL affected CPUs
@@ -66,6 +72,7 @@ struct cpufreq_policy {* governors are used */ struct perf_limits limits;struct cpufreq_governor *governor; +struct internal_governor *internal_gov;bool_t resume;/* flag for cpufreq 1st run * S3 wakeup, hotplug cpu, etc */
Signature · 1 line
1.9.1
Re: [PATCH v6 2/6] x86/intel_pstate: introduce the internal_governor struct
xen/drivers/cpufreq/cpufreq.c int cpufreq_del_cpu(unsigned int cpu) · line 340
Quoted 13 lines
Quoted 1 line
Quoted 1 line · On 28.10.15 at 04:21, <[email protected]> wrote:
xen/drivers/cpufreq/cpufreq.c +3
-2@@ -334,8 +334,9 @@ int cpufreq_del_cpu(unsigned int cpu)/* for HW_ALL, stop gov for each core of the _PSD domain */ /* for SW_ALL & SW_ANY, stop gov for the 1st core of the _PSD domain */ -if (hw_all || (cpumask_weight (cpufreq_dom->map) == - perf->domain_info.num_processors)) +if (!policy->internal_gov && + (hw_all || (cpumask_weight (cpufreq_dom->map) == + perf->domain_info.num_processors)))__cpufreq_governor (policy,CPUFREQ_GOV_STOP );
Why do we need such a guard here, but not on any of the other __cpufreq_governor() invocations? (If they will end up unreachable by the following patches, then I'd still expect one or more ASSERT()s to be added to document/enforce this.)
xen/include/acpi/cpufreq/cpufreq.h struct perf_limits { · line 60
Quoted 11 lines
xen/include/acpi/cpufreq/cpufreq.h +5
-0@@ -53,6 +53,12 @@ struct perf_limits {uint32_t min_policy_pct; }; +struct internal_governor { +char *avail_gov; +uint32_t gov_num; +uint32_t cur_gov; +};
Apart from the fields being meaningless without user I don't think the gov_ prefix and the _gov suffixes are really helpful here. Also "num" isn't really unambiguous - if you mean "count", please use "count". If you mean something else, please pick an unambiguous field name.
Jan
PATCH v6 3/6 x86/intel_pstate: the main body of the intel_pstate driver
In reply to Wei WangWe simply grab the fundamental logic of the intel_pstate driver from Linux kernel, and customize it to Xen style. In the kernel, a user can adjust the limits via sysfs (limits.min_sysfs_pct/max_sysfs_pct). In Xen, the policy->limits.min_perf_pct/max_perf_pct acts as the transit station. A user interacts with it via xenpm.
The new xen/include/asm-x86/cpufreq.h header file is added.
---
changes in v6:
1) change some of the unnecessary signed types to be unsigned, as requested by Jan in v2;
2) remove "__ready_mostly" from the local variable, load, in intel_pstate_init();
3) coding style changes.
xen/arch/x86/acpi/cpufreq/Makefile | 1 + xen/arch/x86/acpi/cpufreq/intel_pstate.c | 882 +++++++++++++++++++++++++++++++ xen/include/acpi/cpufreq/cpufreq.h | 6 + xen/include/asm-x86/cpufreq.h | 31 ++ xen/include/asm-x86/msr-index.h | 3 + 5 files changed, 923 insertions(+) create mode 100644 xen/arch/x86/acpi/cpufreq/intel_pstate.c create mode 100644 xen/include/asm-x86/cpufreq.h
xen/arch/x86/acpi/cpufreq/Makefile +1 -0
@@ -1,2 +1,3 @@ obj-y += cpufreq.o +obj-y += intel_pstate.o obj-y += powernow.o
xen/arch/x86/acpi/cpufreq/intel_pstate.c new file +882 -0
882 lines not on this page · raw message
xen/include/acpi/cpufreq/cpufreq.h +6 -0
@@ -107,6 +107,12 @@ struct cpufreq_freqs {* CPUFREQ GOVERNORS * *********************************************************************/ +/* Please add internal governors here */ +#define INTERNAL_GOV_POWERSAVE (1) +#define INTERNAL_GOV_PERFORMANCE (2) +#define INTERNAL_GOV_USERSPACE (3) +#define INTERNAL_GOV_ONDEMAND (4) +#define CPUFREQ_GOV_START 1#define CPUFREQ_GOV_STOP 2#define CPUFREQ_GOV_LIMITS 3
xen/include/asm-x86/cpufreq.h new file +31 -0
@@ -0,0 +1,31 @@ +#ifndef _ASM_X86_CPUFREQ_H +#define _ASM_X86_CPUFREQ_H + +/* +* Copyright (C) 2015, Intel Corporation. +* Wei Wang <[email protected]> +* +* ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ +* This program is free software; you can redistribute it and/or modify +* it under the terms of the GNU General Public License as published by +* the Free Software Foundation; either version 2 of the License, or +* (at your option) any later version. +* +* This program is distributed in the hope that it will be useful, +* but WITHOUT ANY WARRANTY; without even the implied warranty of +* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +* GNU General Public License for more details. +* +* You should have received a copy of the GNU General Public License +* along with this program; If not, see <http://www.gnu.org/licenses/>. +* +* ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ +*/ + +/* +* Maximum transition latency is in nanoseconds - if it's unknown, +* CPUFREQ_ETERNAL shall be used. +*/ +#define CPUFREQ_ETERNAL (-1) + +#endif /* _ASM_X86_CPUFREQ_H */
xen/include/asm-x86/msr-index.h +3 -0
@@ -52,6 +52,8 @@#define MSR_IA32_MCG_STATUS 0x0000017a#define MSR_IA32_MCG_CTL 0x0000017b +#define MSR_NHM_TURBO_RATIO_LIMIT 0x000001ad +#define MSR_IA32_PEBS_ENABLE 0x000003f1#define MSR_IA32_DS_AREA 0x00000600#define MSR_IA32_PERF_CAPABILITIES 0x00000345
@@ -320,6 +322,7 @@#define MSR_IA32_MISC_ENABLE_MONITOR_ENABLE (1<<18)#define MSR_IA32_MISC_ENABLE_LIMIT_CPUID (1<<22)#define MSR_IA32_MISC_ENABLE_XTPR_DISABLE (1<<23) +#define MSR_IA32_MISC_ENABLE_TURBO_DISABLE (1ULL<<38)#define MSR_IA32_TSC_DEADLINE 0x000006E0#define MSR_IA32_ENERGY_PERF_BIAS 0x000001b0
Signature · 1 line
1.9.1
Re: [PATCH v6 3/6] x86/intel_pstate: the main body of the intel_pstate driver
Quoted 11 lines
Quoted 1 line
Quoted 1 line · On 28.10.15 at 04:21, <[email protected]> wrote:
xen/arch/x86/acpi/cpufreq/intel_pstate.c new file +7
-0@@ -0,0 +1,882 @@ +#include <xen/kernel.h> +#include <xen/types.h> +#include <xen/init.h> +#include <xen/bitmap.h> +#include <xen/cpumask.h> +#include <xen/timer.h> +#include <asm/msr.h>
xen/arch/x86/acpi/cpufreq/intel_pstate.c line 17
Quoted 10 lines
xen/arch/x86/acpi/cpufreq/intel_pstate.c new file +10
-0+#include <asm/msr-index.h> +#include <asm/processor.h> +#include <asm/div64.h> +#include <asm/cpufreq.h> +#include <acpi/cpufreq/cpufreq.h> + +#define BYT_RATIOS 0x66a +#define BYT_VIDS 0x66b +#define BYT_TURBO_RATIOS 0x66c +#define BYT_TURBO_VIDS 0x66d
Considering these are MSR names, the acronym MSR should appear in them.
xen/arch/x86/acpi/cpufreq/intel_pstate.c line 25
Quoted 7 lines
xen/arch/x86/acpi/cpufreq/intel_pstate.c new file +7
-0+#define FRAC_BITS 8 +#define int_tofp (X ) ((uint64_t)(X) << FRAC_BITS) +#define fp_toint (X ) ((X) >> FRAC_BITS) + +static inline uint32_t mul_fp (uint32_t x,uint32_t y) +{ +return ((uint64_t )x * (uint64_t )y) >>FRAC_BITS ;
Casting just one side of the * would suffice. Also please don't open-code fp_to_int() ...
xen/arch/x86/acpi/cpufreq/intel_pstate.c line 30
Quoted 3 lines
xen/arch/x86/acpi/cpufreq/intel_pstate.c new file +3
-0+static inline uint32_t div_fp (uint32_t x,uint32_t y) +{ +return div_s64 ((uint64_t )x <<FRAC_BITS , y);
... or int_to_fp() (for this one it would be a different thing if you removed the - apparently pointless - cast from the macro).
xen/arch/x86/acpi/cpufreq/intel_pstate.c line 42
Quoted 10 lines
xen/arch/x86/acpi/cpufreq/intel_pstate.c new file +10
-0+static inline uint32_t ceiling_fp (uint32_t x) +{ +uint32_t mask, ret; + + ret =fp_toint (x); + mask = (1 <<FRAC_BITS ) -1 ; +if ( x & mask ) + ret +=1 ; +return ret; +}
Blank line before final return statement please.
Code further down suggests there may be a sign in these fixed point numbers, yet here you converted _everything_ to uintNN_t. Did you go too far? What sense does e.g. div_s64() on a uint64_t make?
xen/arch/x86/acpi/cpufreq/intel_pstate.c line 78
Quoted 2 lines
xen/arch/x86/acpi/cpufreq/intel_pstate.c new file +2
-0+struct cpudata { +int cpu;
unsigned int (but I wonder if this is really needed, see the comment at the allocation site)
xen/arch/x86/acpi/cpufreq/intel_pstate.c line 153
Quoted 8 lines
xen/arch/x86/acpi/cpufreq/intel_pstate.c new file +8
-0+static signed int pid_calc (struct _pid *pid,uint32_t busy) +{ +signed int result; +int32_t pterm, dterm, fp_error; +int32_t integral_limit; + + fp_error =int_tofp (pid->setpoint) - busy; +if (ABS (fp_error) <=int_tofp (pid->deadband) )
int_tofp() currently returning an unsigned value - why ABS()?
xen/arch/x86/acpi/cpufreq/intel_pstate.c line 192
Quoted 3 lines
xen/arch/x86/acpi/cpufreq/intel_pstate.c new file +3
-0+static inline void intel_pstate_reset_all_pid (void ) +{ +uint32_t cpu;
unsigned int
xen/arch/x86/acpi/cpufreq/intel_pstate.c line 210
Quoted 10 lines
xen/arch/x86/acpi/cpufreq/intel_pstate.c new file +10
-0+static inline void update_turbo_state (struct cpufreq_policy *policy) +{ +uint64_t misc_en; +struct cpudata *cpu; + + cpu = all_cpu_data[policy->cpu]; +rdmsrl (MSR_IA32_MISC_ENABLE , misc_en); + policy->limits.turbo_disabled = + (misc_en &MSR_IA32_MISC_ENABLE_TURBO_DISABLE || + cpu->pstate.max_pstate == cpu->pstate.turbo_pstate);
At least the & needs parenthesization.
xen/arch/x86/acpi/cpufreq/intel_pstate.c line 248
Quoted 8 lines
xen/arch/x86/acpi/cpufreq/intel_pstate.c new file +8
-0+static void byt_set_pstate (struct perf_limits *limits, +struct cpudata *cpudata,uint32_t pstate) +{ +uint64_t val; +uint32_t vid_fp; +uint32_t vid; + + val = pstate <<8 ;
Didn't we agree not to have any unnamed literals?
xen/arch/x86/acpi/cpufreq/intel_pstate.c line 250
Quoted 2 lines
xen/arch/x86/acpi/cpufreq/intel_pstate.c new file +2
-0+if ( limits->no_turbo && !limits->turbo_disabled ) + val |= (uint64_t )1 <<BYT_TURBO_CONTROL_BIT ;
Perhaps the sense of that bit isn't what one would imply at first glance, but the combination of conditions in the if() looks certainly odd. Please confirm this is really intended to be this way (which should also include pointing out what meaning the bit being on has).
xen/arch/x86/acpi/cpufreq/intel_pstate.c line 260
Quoted 9 lines
xen/arch/x86/acpi/cpufreq/intel_pstate.c new file +9
-0+ vid_fp = cpudata->vid.min +mul_fp ( +int_tofp (pstate - cpudata->pstate.min_pstate), + cpudata->vid.ratio); + + vid_fp =clamp (vid_fp, cpudata->vid.min, cpudata->vid.max); + vid =ceiling_fp (vid_fp); + +if ( pstate > cpudata->pstate.max_pstate ) + vid = cpudata->vid.turbo;
The prior calculation of vid (and vid_fp) is completely pointless in this case. Also - what if limits->no_turbo or limits->turbo_disabled?
xen/arch/x86/acpi/cpufreq/intel_pstate.c line 269
Quoted 8 lines
xen/arch/x86/acpi/cpufreq/intel_pstate.c new file +8
-0+ val |= vid; + +wrmsrl (MSR_IA32_PERF_CTL , val); +} + +#define BYT_BCLK_FREQS 5 +#define TO_FREQ_TABLE_IDX_MASK 0x7 +static uint32_t byt_get_scaling (void )
Missing blank line.
xen/arch/x86/acpi/cpufreq/intel_pstate.c line 89
Quoted 2 lines
xen/arch/x86/acpi/cpufreq/intel_pstate.c new file +2
-0+{ + const uint32_t byt_freq_table[BYT_BCLK_FREQS] =
static
xen/arch/x86/acpi/cpufreq/intel_pstate.c line 277
Quoted 6 lines
xen/arch/x86/acpi/cpufreq/intel_pstate.c new file +6
-0+ {833, 1000, 1333, 1167, 800}; +uint64_t value; +int i; + +rdmsrl (MSR_FSB_FREQ , value); + i = value &TO_FREQ_TABLE_IDX_MASK ;
With this calculation i is hardly a signed quantity.
xen/arch/x86/acpi/cpufreq/intel_pstate.c line 309
Quoted 9 lines
xen/arch/x86/acpi/cpufreq/intel_pstate.c new file +9
-0+#define CORE_MIN_PSTATE (val) (((value) >> 40) & 0xff) +#define CORE_MAX_PSTATE (val) (((value) >> 8) & 0xff) +#define CORE_TURBO_PSTATE (value) ((value) & 0xff) +static uint32_t core_get_min_pstate (void ) +{ +uint64_t value; + +rdmsrl (MSR_INTEL_PLATFORM_INFO , value); +return CORE_MIN_PSTATE (val);
The fact that (presumably) don't get a build error here is because the mix of variable names here happens to invert the mix of names in the macros ahead of the function.
xen/arch/x86/acpi/cpufreq/intel_pstate.c line 350
Quoted 13 lines
xen/arch/x86/acpi/cpufreq/intel_pstate.c new file +13
-0+static void core_set_pstate (struct perf_limits *limits, +struct cpudata *cpudata,uint32_t pstate) +{ +uint64_t val; + + val = pstate <<8 ; +if ( limits->no_turbo && !limits->turbo_disabled ) + val |= (uint64_t )1 <<CORE_TURBO_CONTROL_BIT ; + +wrmsrl (MSR_IA32_PERF_CTL , val); +} + +static __initconst struct cpu_defaults core_params = {
static const struct cpu_defaults __initconst core_params = {
xen/arch/x86/acpi/cpufreq/intel_pstate.c line 461
Quoted 12 lines
xen/arch/x86/acpi/cpufreq/intel_pstate.c new file +12
-0+static inline void intel_pstate_calc_busy (struct cpudata *cpu) +{ +struct sample *sample = &cpu->sample; +uint64_t core_pct; + + core_pct =int_tofp (sample->aperf) *int_tofp (100 ); + core_pct =div64_u64 (core_pct,int_tofp (sample->mperf)); + + sample->freq =fp_toint (mul_fp (int_tofp (cpu->pstate.max_pstate * + cpu->pstate.scaling /100 ), core_pct)); + + sample->core_pct_busy = (int32_t )core_pct;
Pointless cast.
Quoted 6 lines
xen/arch/x86/acpi/cpufreq/intel_pstate.c new file +6
-0+static inline void intel_pstate_sample (struct cpudata *cpu) +{ +uint64_t aperf, mperf; +unsigned long flags; + +local_irq_save (flags);
xen/arch/x86/acpi/cpufreq/intel_pstate.c line 479
Quoted 10 lines
xen/arch/x86/acpi/cpufreq/intel_pstate.c new file +10
-0+rdmsrl (MSR_IA32_APERF , aperf); +rdmsrl (MSR_IA32_MPERF , mperf); +local_irq_restore (flags); + + cpu->last_sample_time = cpu->sample.time; + cpu->sample.time =get_s_time (); + cpu->sample.aperf = aperf; + cpu->sample.mperf = mperf; + cpu->sample.aperf -= cpu->prev_aperf; + cpu->sample.mperf -= cpu->prev_mperf;
Can these two please be done in a single statement each?
xen/arch/x86/acpi/cpufreq/intel_pstate.c line 484
Quoted 4 lines
xen/arch/x86/acpi/cpufreq/intel_pstate.c new file +4
-0+intel_pstate_calc_busy (cpu); + + cpu->prev_aperf = aperf; + cpu->prev_mperf = mperf;
And these two be moved up (intel_pstate_calc_busy() doesn't appear to be using the updated fields)?
xen/arch/x86/acpi/cpufreq/intel_pstate.c line 552
Quoted 3 lines
xen/arch/x86/acpi/cpufreq/intel_pstate.c new file +3
-0+static void intel_pstate_timer_func (void *data) +{ +struct cpudata *cpu = (struct cpudata *) data;
Pointless cast.
xen/arch/x86/acpi/cpufreq/intel_pstate.c line 680
Quoted 3 lines
xen/arch/x86/acpi/cpufreq/intel_pstate.c new file +3
-0+static int intel_pstate_cpu_exit (struct cpufreq_policy *policy) +{ +int cpu_num = policy->cpu;
unsigned int
xen/arch/x86/acpi/cpufreq/intel_pstate.c line 685
Quoted 5 lines
xen/arch/x86/acpi/cpufreq/intel_pstate.c new file +5
-0+struct cpudata *cpu = all_cpu_data[cpu_num]; + +kill_timer (&all_cpu_data[cpu_num]->timer); + +intel_pstate_set_pstate (policy, cpu, cpu->pstate.min_pstate);
Why min and not max? Or whatever the CPU was in when the governor initialized the CPU?
xen/arch/x86/acpi/cpufreq/intel_pstate.c line 702
Quoted 11 lines
xen/arch/x86/acpi/cpufreq/intel_pstate.c new file +11
-0+static int intel_pstate_turbo_update (int cpuid,struct cpufreq_policy *policy) +{ +struct cpudata *cpu = all_cpu_data[policy->cpu]; +struct perf_limits *limits = &policy->limits; + +update_turbo_state (policy); +if ( limits->turbo_disabled ) + { +printk ("Turbo disabled by BIOS or not supported on CPU\n" ); +return -EINVAL ; + }
I'm afraid this message may get printed more often than you'd want (it should be printed at most once during any session).
xen/arch/x86/acpi/cpufreq/intel_pstate.c line 703
Quoted 1 line
xen/arch/x86/acpi/cpufreq/intel_pstate.c new file +1
-0+ limits->no_turbo = policy->turbo ==CPUFREQ_TURBO_ENABLED ?0 :1 ;
limits->no_turbo = policy->turbo != CPUFREQ_TURBO_ENABLED;
Quoted 8 lines
xen/arch/x86/acpi/cpufreq/intel_pstate.c new file +8
-0+#define INTEL_PSTATE_GOV_NUM 4 +static struct internal_governor *intel_pstate_internal_gov_init (void ) +{ +unsigned int i =0 ; +struct internal_governor *gov; +char *avail_gov; + + gov = xzalloc(struct internal_governor);
xen/arch/x86/acpi/cpufreq/intel_pstate.c line 747
Quoted 10 lines
xen/arch/x86/acpi/cpufreq/intel_pstate.c new file +10
-0+if ( !gov ) +return NULL ; + avail_gov =xzalloc_array (char, +INTEL_PSTATE_GOV_NUM *CPUFREQ_NAME_LEN ); +if ( !avail_gov ) +return NULL ; + + gov->avail_gov = avail_gov; + + i +=scnprintf (&avail_gov[0 ],CPUFREQ_NAME_LEN ,"%s " ,"performance" );
i = ... (and the initializer dropped above).
xen/arch/x86/acpi/cpufreq/intel_pstate.c line 751
Quoted 4 lines
xen/arch/x86/acpi/cpufreq/intel_pstate.c new file +4
-0+ i +=scnprintf (&avail_gov[i],CPUFREQ_NAME_LEN ,"%s " ,"powersave" ); + i +=scnprintf (&avail_gov[i],CPUFREQ_NAME_LEN ,"%s " ,"userspace" ); + i +=scnprintf (&avail_gov[i],CPUFREQ_NAME_LEN ,"%s " ,"ondemand" ); + avail_gov[i-1 ] ='\0' ;
Coding style.
But overall I wonder whether this couldn't be done in a less convoluted way (e.g. a single strlcpy(), or a strlcpy() followed by three strlcat()s). In no case is the indirection via %s warranted.
xen/arch/x86/acpi/cpufreq/intel_pstate.c line 753
Quoted 2 lines
xen/arch/x86/acpi/cpufreq/intel_pstate.c new file +2
-0+ gov->gov_num =INTEL_PSTATE_GOV_NUM ; + gov->cur_gov =INTERNAL_GOV_ONDEMAND ;
Shouldn't this depend on the "cpufreq=" option setting (if any)?
xen/arch/x86/acpi/cpufreq/intel_pstate.c line 763
Quoted 7 lines
xen/arch/x86/acpi/cpufreq/intel_pstate.c new file +7
-0+static int intel_pstate_cpu_setup (struct cpufreq_policy *policy) +{ +struct cpudata *cpu; +struct perf_limits *limits = &policy->limits; +int rc; + + rc =intel_pstate_init_cpu (policy->cpu);
Please use reasonably consistent naming (compare the name of the function we're in and that of the called function).
xen/arch/x86/acpi/cpufreq/intel_pstate.c line 773
Quoted 10 lines
xen/arch/x86/acpi/cpufreq/intel_pstate.c new file +10
-0+if ( rc ) +return rc; + + policy->internal_gov =intel_pstate_internal_gov_init (); +if ( !policy->internal_gov ) +return -ENOMEM ; + + cpu = all_cpu_data[policy->cpu]; + policy->min = cpu->pstate.min_pstate * cpu->pstate.scaling; + policy->max = cpu->pstate.turbo_pstate * cpu->pstate.scaling;
DYM .max_pstate here?
xen/arch/x86/acpi/cpufreq/intel_pstate.c line 779
Quoted 5 lines
xen/arch/x86/acpi/cpufreq/intel_pstate.c new file +5
-0+/* cpuinfo and default policy values */ + policy->cpuinfo.min_freq = cpu->pstate.min_pstate * + cpu->pstate.scaling; + policy->cpuinfo.max_freq = cpu->pstate.turbo_pstate * + cpu->pstate.scaling;
Same here? Or if intentional, then I think a comment is warranted, and policy->turbo would then also seem to need initializing to 1 (that actually might eliminate the need for a separate comment).
xen/arch/x86/acpi/cpufreq/intel_pstate.c line 793
Quoted 6 lines
xen/arch/x86/acpi/cpufreq/intel_pstate.c new file +6
-0+ limits->min_policy_pct =clamp_t (uint32_t, + limits->min_policy_pct,0 ,100 ); + limits->max_policy_pct = (policy->max *100 ) / + policy->cpuinfo.max_freq; + limits->max_policy_pct =clamp_t (uint32_t, + limits->max_policy_pct,0 ,100 );
These appear to be the only two uses of clamp_t(), which can be easily made just clamp(). Hence I don't see the need for introducing clamp_t() in the first place.
xen/arch/x86/acpi/cpufreq/intel_pstate.c line 819
Quoted 1 line
xen/arch/x86/acpi/cpufreq/intel_pstate.c new file +1
-0+static void __init copy_pid_params (struct pstate_adjust_policy *policy)
const
xen/arch/x86/acpi/cpufreq/intel_pstate.c line 826
Quoted 7 lines
xen/arch/x86/acpi/cpufreq/intel_pstate.c new file +7
-0+{ + pid_params.sample_rate_ms = policy->sample_rate_ms; + pid_params.p_gain_pct = policy->p_gain_pct; + pid_params.i_gain_pct = policy->i_gain_pct; + pid_params.d_gain_pct = policy->d_gain_pct; + pid_params.deadband = policy->deadband; + pid_params.setpoint = policy->setpoint;
Why not just
pid_params = *policy;
?
xen/arch/x86/acpi/cpufreq/intel_pstate.c line 829
Quoted 1 line
xen/arch/x86/acpi/cpufreq/intel_pstate.c new file +1
-0+static void __init copy_cpu_funcs (struct pstate_funcs *funcs)
const
xen/arch/x86/acpi/cpufreq/intel_pstate.c line 836
Quoted 7 lines
xen/arch/x86/acpi/cpufreq/intel_pstate.c new file +7
-0+{ + pstate_funcs.get_max = funcs->get_max; + pstate_funcs.get_min = funcs->get_min; + pstate_funcs.get_turbo = funcs->get_turbo; + pstate_funcs.get_scaling = funcs->get_scaling; + pstate_funcs.set = funcs->set; + pstate_funcs.get_vid = funcs->get_vid;
And again
pstate_funcs = *funcs;
?
xen/arch/x86/acpi/cpufreq/intel_pstate.c line 841
Quoted 3 lines
xen/arch/x86/acpi/cpufreq/intel_pstate.c new file +3
-0+int __init intel_pstate_init (void ) +{ +int cpu, rc =0 ;
unsigned int cpu;
int rc;
xen/arch/x86/acpi/cpufreq/intel_pstate.c line 844
Quoted 3 lines
xen/arch/x86/acpi/cpufreq/intel_pstate.c new file +3
-0+const struct x86_cpu_id *id; +struct cpu_defaults *cpu_info; +static bool_t load;
__initdata (and I'm sure I said so before).
xen/arch/x86/acpi/cpufreq/intel_pstate.c line 854
Quoted 10 lines
xen/arch/x86/acpi/cpufreq/intel_pstate.c new file +10
-0+boolean_param ("intel_pstate" , load); + +if ( !load ) +return -ENODEV ; + + id =x86_match_cpu (intel_pstate_cpu_ids); +if ( !id ) +return -ENODEV ; + + cpu_info = (struct cpu_defaults *)id->driver_data;
This casts away const-ness, which is why casts should be avoided wherever possible (and the more when they're pointless like this one).
xen/arch/x86/acpi/cpufreq/intel_pstate.c line 860
Quoted 5 lines
xen/arch/x86/acpi/cpufreq/intel_pstate.c new file +5
-0+copy_pid_params (&cpu_info->pid_policy); +copy_cpu_funcs (&cpu_info->funcs); + +if (intel_pstate_msrs_not_valid () ) +return -ENODEV ;
This reads as if the function returned a boolean. Either make it so (and the perhaps invert the sense and drop the "not" from the name) or use the function's return value (in which the function should perhaps be named ..._validate or some such).
xen/arch/x86/acpi/cpufreq/intel_pstate.c line 862
Quoted 1 line
xen/arch/x86/acpi/cpufreq/intel_pstate.c new file +1
-0+ all_cpu_data = xzalloc_array(struct cpudata *, NR_CPUS);
Why NR_CPUS? Can't this be a per-CPU item anyway?
xen/arch/x86/acpi/cpufreq/intel_pstate.c line 870
Quoted 8 lines
xen/arch/x86/acpi/cpufreq/intel_pstate.c new file +8
-0+if ( !all_cpu_data ) +return -ENOMEM ; + + rc =cpufreq_register_driver (&intel_pstate_driver); +if ( rc ) + goto out; + +return rc;
Looks like this could be "return 0", albeit ...
xen/arch/x86/acpi/cpufreq/intel_pstate.c line 871
Quoted 1 line
xen/arch/x86/acpi/cpufreq/intel_pstate.c new file +1
-0+out:
... just a single path leading here (and the comment below taken into account) just doing the cleanup in the if() body above would seem more reasonable. Otherwise - labels indented by at least one space please.
xen/arch/x86/acpi/cpufreq/intel_pstate.c line 879
Quoted 8 lines
xen/arch/x86/acpi/cpufreq/intel_pstate.c new file +8
-0+for_each_online_cpu (cpu) + { +if ( all_cpu_data[cpu] ) + { +kill_timer (&all_cpu_data[cpu]->timer); +xfree (all_cpu_data[cpu]); + } + }
Why is all of this needed? If cpufreq_register_driver() (or one of its descendants) fails, it should clean up after itself.
Quoted 22 lines
xen/include/asm-x86/cpufreq.h new file +19
-0@@ -0,0 +1,31 @@ +#ifndef _ASM_X86_CPUFREQ_H +#define _ASM_X86_CPUFREQ_H + +/* +* Copyright (C) 2015, Intel Corporation. +* Wei Wang <[email protected]> +* +* ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ +* This program is free software; you can redistribute it and/or modify +* it under the terms of the GNU General Public License as published by +* the Free Software Foundation; either version 2 of the License, or +* (at your option) any later version. +* +* This program is distributed in the hope that it will be useful, +* but WITHOUT ANY WARRANTY; without even the implied warranty of +* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +* GNU General Public License for more details. +* +* You should have received a copy of the GNU General Public License
xen/include/asm-x86/cpufreq.h line 29
Quoted 10 lines
xen/include/asm-x86/cpufreq.h new file +10
-0+* along with this program; If not, see <http://www.gnu.org/licenses/>. +* +* ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ +*/ + +/* +* Maximum transition latency is in nanoseconds - if it's unknown, +* CPUFREQ_ETERNAL shall be used. +*/ +#define CPUFREQ_ETERNAL (-1)
So what is x86-specific about this #define? I.e. why can't this go into an existing header?
Jan
PATCH v6 4/6 x86/intel_pstate: add a booting param to select the driver to load
In reply to Wei WangBy default, the old P-state driver (acpi-freq) is used. Adding "intel_pstate" to the Xen booting param list to enable the use of intel_pstate. However, if intel_pstate is enabled on a machine which does not support the driver (e.g. Nehalem), the old P-state driver will be loaded due to the failure loading of intel_pstate.
Also, adding the intel_pstate booting parameter to xen-command-line.markdown.
---
changes in v6:
1) move the declaration of intel_pstate_init() to this patch.
docs/misc/xen-command-line.markdown | 7 +++++++ xen/arch/x86/acpi/cpufreq/cpufreq.c | 15 ++++++++++----- xen/include/asm-x86/cpufreq.h | 2 ++ 3 files changed, 19 insertions(+), 5 deletions(-)
docs/misc/xen-command-line.markdown +7 -0
@@ -858,6 +858,13 @@ debug hypervisor only). ### idle\_latency\_factor > `= <integer>` +### intel\_pstate +> `= <boolean>` + +> Default: `false` + +Enable the loading of the intel pstate driver. + ### ioapic\_ack > `= old | new`
xen/arch/x86/acpi/cpufreq/cpufreq.c +10 -5
@@ -40,6 +40,7 @@#include <asm/processor.h> #include <asm/percpu.h> #include <asm/cpufeature.h> +#include <asm/cpufreq.h> #include <acpi/acpi.h> #include <acpi/cpufreq/cpufreq.h>
@@ -647,11 +648,15 @@ static int __init cpufreq_driver_init(void) {int ret =0 ; -if ((cpufreq_controller == FREQCTL_xen) && - (boot_cpu_data.x86_vendor ==X86_VENDOR_INTEL )) - ret =cpufreq_register_driver (&acpi_cpufreq_driver); -else if ((cpufreq_controller == FREQCTL_xen) && - (boot_cpu_data.x86_vendor ==X86_VENDOR_AMD )) +if ( (cpufreq_controller == FREQCTL_xen) && + (boot_cpu_data.x86_vendor ==X86_VENDOR_INTEL ) ) + { + ret =intel_pstate_init (); +if ( ret ) + ret =cpufreq_register_driver (&acpi_cpufreq_driver); + } +else if ( (cpufreq_controller == FREQCTL_xen) && + (boot_cpu_data.x86_vendor ==X86_VENDOR_AMD ) ) ret =powernow_register_driver ();return ret;
xen/include/asm-x86/cpufreq.h +2 -0
@@ -22,6 +22,8 @@* ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ */ +extern int intel_pstate_init (void ); +/* * Maximum transition latency is in nanoseconds - if it's unknown, * CPUFREQ_ETERNAL shall be used.
Signature · 1 line
1.9.1
Re: [PATCH v6 4/6] x86/intel_pstate: add a booting param to select the driver to load
Quoted 2 lines
Quoted 1 line
Quoted 1 line · On 28.10.15 at 04:21, <[email protected]> wrote:
Signed-off-by Wei Wang <[email protected]>
with two remarks:
docs/misc/xen-command-line.markdown debug hypervisor only). · line 866
Quoted 12 lines
docs/misc/xen-command-line.markdown +6
-0@@ -858,6 +858,13 @@ debug hypervisor only). ### idle\_latency\_factor > `= <integer>` +### intel\_pstate +> `= <boolean>` + +> Default: `false` + +Enable the loading of the intel pstate driver.
"Loading" is probably not the right term, as it gets always loaded. Perhaps just drop "the loading of"?
xen/include/asm-x86/cpufreq.h line 25
Quoted 7 lines
xen/include/asm-x86/cpufreq.h +1
-0@@ -22,6 +22,8 @@* ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ */ +extern int intel_pstate_init (void );
Considering that I questioned the creation of this header in the previous patch, the addition of this declaration should not result in it now being created here. In fact if any new header was needed for this declaration, it would probably better live in xen/arch/x86/acpi/cpufreq/.
Moving this to a _suitable_ existing header does not void the ack given.
Jan
PATCH v6 5/6 x86/intel_pstate: support the use of intel_pstate in pmstat.c
In reply to Wei WangAdd support in the pmstat.c so that the xenpm tool can request to access the intel_pstate driver.
---
changes in v6:
1) add the NON_INTERNAL_GOV macro to replace literal 0;
2) code consolidation (e.g. merging some code into if/else, as required in v5);
3) somewhere, change to use clamp, instead of clamp_t;
4) xen_perf_alias, instead of perf_alias.
tools/libxc/include/xenctrl.h | 20 ++-- tools/libxc/xc_pm.c | 16 ++-- tools/misc/xenpm.c | 4 +- xen/drivers/acpi/pmstat.c | 183 +++++++++++++++++++++++++++++++------ xen/include/acpi/cpufreq/cpufreq.h | 2 + xen/include/public/sysctl.h | 29 ++++-- 6 files changed, 198 insertions(+), 56 deletions(-)
tools/libxc/include/xenctrl.h +11 -9
@@ -2270,6 +2270,17 @@ struct xc_get_cpufreq_para {uint32_t cpu_num;uint32_t freq_num;uint32_t gov_num; +int32_t turbo_enabled; + +uint32_t cpuinfo_cur_freq; +uint32_t cpuinfo_max_freq; +uint32_t cpuinfo_min_freq; +uint32_t scaling_cur_freq; + +uint32_t scaling_turbo_pct; +uint32_t scaling_max_perf; +uint32_t scaling_min_perf; +enum xen_perf_alias perf_alias;/* for all governors */ /* OUT variable */
@@ -2278,22 +2289,13 @@ struct xc_get_cpufreq_para {char *scaling_available_governors;char scaling_driver[CPUFREQ_NAME_LEN ]; -uint32_t cpuinfo_cur_freq; -uint32_t cpuinfo_max_freq; -uint32_t cpuinfo_min_freq; -uint32_t scaling_cur_freq; -char scaling_governor[CPUFREQ_NAME_LEN ]; -uint32_t scaling_max_freq; -uint32_t scaling_min_freq;/* for specific governor */ union {xc_userspace_t userspace;xc_ondemand_t ondemand; } u; - -int32_t turbo_enabled; }; int xc_get_cpufreq_para(xc_interface *xch, int cpuid,
tools/libxc/xc_pm.c +9 -7
@@ -260,13 +260,15 @@ int xc_get_cpufreq_para(xc_interface *xch, int cpuid, }else { - user_para->cpuinfo_cur_freq = sys_para->cpuinfo_cur_freq; - user_para->cpuinfo_max_freq = sys_para->cpuinfo_max_freq; - user_para->cpuinfo_min_freq = sys_para->cpuinfo_min_freq; - user_para->scaling_cur_freq = sys_para->scaling_cur_freq; - user_para->scaling_max_freq = sys_para->scaling_max_freq; - user_para->scaling_min_freq = sys_para->scaling_min_freq; - user_para->turbo_enabled = sys_para->turbo_enabled; + user_para->cpuinfo_cur_freq = sys_para->cpuinfo_cur_freq; + user_para->cpuinfo_max_freq = sys_para->cpuinfo_max_freq; + user_para->cpuinfo_min_freq = sys_para->cpuinfo_min_freq; + user_para->scaling_cur_freq = sys_para->scaling_cur_freq; + user_para->scaling_max_perf = sys_para->scaling_max_perf; + user_para->scaling_min_perf = sys_para->scaling_min_perf; + user_para->scaling_turbo_pct = sys_para->scaling_turbo_pct; + user_para->perf_alias = sys_para->perf_alias; + user_para->turbo_enabled = sys_para->turbo_enabled;memcpy (user_para->scaling_driver, sys_para->scaling_driver,CPUFREQ_NAME_LEN );
tools/misc/xenpm.c +2 -2
@@ -705,8 +705,8 @@ static void print_cpufreq_para(int cpuid, struct xc_get_cpufreq_para *p_cpufreq)printf ("\n" );printf ("scaling frequency : max [%u] min [%u] cur [%u]\n" , - p_cpufreq->scaling_max_freq, - p_cpufreq->scaling_min_freq, + p_cpufreq->scaling_max_perf, + p_cpufreq->scaling_min_perf, p_cpufreq->scaling_cur_freq); printf("turbo mode : %s\n",
xen/drivers/acpi/pmstat.c +155 -28
275 lines not on this page · raw message
xen/include/acpi/cpufreq/cpufreq.h +2 -0
@@ -107,7 +107,9 @@ struct cpufreq_freqs {* CPUFREQ GOVERNORS * *********************************************************************/ +/* Please add internal governors here */ +#define NON_INTERNAL_GOV (0)#define INTERNAL_GOV_POWERSAVE (1)#define INTERNAL_GOV_PERFORMANCE (2)#define INTERNAL_GOV_USERSPACE (3)
xen/include/public/sysctl.h +19 -10
@@ -297,11 +297,28 @@ typedef struct xen_ondemand xen_ondemand_t;* same as sysfs file name of native linux */ #define CPUFREQ_NAME_LEN 16 + +enum xen_perf_alias { +FREQUENCY =0 , +PERCENTAGE =1 +}; +struct xen_get_cpufreq_para {/* IN/OUT variable */ uint32_t cpu_num;uint32_t freq_num;uint32_t gov_num; +int32_t turbo_enabled; + +uint32_t cpuinfo_cur_freq; +uint32_t cpuinfo_max_freq; +uint32_t cpuinfo_min_freq; +uint32_t scaling_cur_freq; + +uint32_t scaling_turbo_pct; +uint32_t scaling_max_perf; +uint32_t scaling_min_perf; +enum xen_perf_alias perf_alias;/* for all governors */ /* OUT variable */
@@ -309,23 +326,13 @@ struct xen_get_cpufreq_para {XEN_GUEST_HANDLE_64 (uint32) scaling_available_frequencies;XEN_GUEST_HANDLE_64 (char ) scaling_available_governors;char scaling_driver[CPUFREQ_NAME_LEN ]; - -uint32_t cpuinfo_cur_freq; -uint32_t cpuinfo_max_freq; -uint32_t cpuinfo_min_freq; -uint32_t scaling_cur_freq; -char scaling_governor[CPUFREQ_NAME_LEN ]; -uint32_t scaling_max_freq; -uint32_t scaling_min_freq;/* for specific governor */ union {struct xen_userspace userspace;struct xen_ondemand ondemand; } u; - -int32_t turbo_enabled; };struct xen_set_cpufreq_gov {
@@ -338,6 +345,8 @@ struct xen_set_cpufreq_para {#define SCALING_SETSPEED 3#define SAMPLING_RATE 4#define UP_THRESHOLD 5 +#define SCALING_MAX_PCT 6 +#define SCALING_MIN_PCT 7uint32_t ctrl_type;uint32_t ctrl_value;
Signature · 1 line
1.9.1
Re: [PATCH v6 5/6] x86/intel_pstate: support the use of intel_pstate in pmstat.c
Quoted 11 lines · Wei Wang
Add support in the
pmstat.cso that the xenpm tool can request to access theintel_pstatedriver.Signed-off-by Wei Wang <[email protected]>---
changes in v6:
1) add theNON_INTERNAL_GOVmacro to replace literal 0;
2) code consolidation (e.g. merging some code into if/else, as required in v5);
3) somewhere, change to use clamp, instead ofclamp_t;
4)xen_perf_alias, instead ofperf_alias.
Quoted 3 lines
tools/libxc/include/xenctrl.h | 20 ++-- tools/libxc/xc_pm.c | 16 ++-- tools/misc/xenpm.c | 4 +-
I think all changes to toolstack code are merely trying to mirror hypervisor side changes and there are no functional changes.
With that understanding and subject to an ack from hypervisor side maintainer:
Re: [PATCH v6 5/6] x86/intel_pstate: support the use of intel_pstate in pmstat.c
In reply to Wei WangQuoted 3 lines
Quoted 1 line
Quoted 1 line · On 28.10.15 at 04:21, <[email protected]> wrote:
tools/libxc/include/xenctrl.h | 20 ++-- tools/libxc/xc_pm.c | 16 ++--
I won't (again) comment on the changes of these files (I'll leave it to the tools maintainers), but I can't help thinking that there's more shuffling there than necessary.
tools/misc/xenpm.c static void print_cpufreq_para(int cpuid, struct xc_get_cpufreq_para *p_cpufreq) · line 710
Quoted 11 lines
tools/misc/xenpm.c +2
-2@@ -705,8 +705,8 @@ static void print_cpufreq_para(int cpuid, struct xc_get_cpufreq_para *p_cpufreq)printf ("\n" );printf ("scaling frequency : max [%u] min [%u] cur [%u]\n" , - p_cpufreq->scaling_max_freq, - p_cpufreq->scaling_min_freq, + p_cpufreq->scaling_max_perf, + p_cpufreq->scaling_min_perf, p_cpufreq->scaling_cur_freq);
So where did your percentages go?
Quoted 24 lines
tools/misc/xenpm.c +14
-5@@ -240,40 +253,85 @@ static int get_cpufreq_para(struct xen_sysctl_pm_op *op)if ( ret )return ret; -if ( !(scaling_available_governors = -xzalloc_array (char, gov_num *CPUFREQ_NAME_LEN )) ) -return -ENOMEM ; -if ( (ret =read_scaling_available_governors (scaling_available_governors, - gov_num *CPUFREQ_NAME_LEN *sizeof (char ))) ) +if ( internal_gov ) { + ret =copy_to_guest (op->u.get_para.scaling_available_governors, + internal_gov->avail_gov, gov_num *CPUFREQ_NAME_LEN ); +if ( ret ) +return ret; + } +else + { +if ( !(scaling_available_governors = +xzalloc_array (char, gov_num *CPUFREQ_NAME_LEN )) ) +return -ENOMEM ; +if ( (ret =read_scaling_available_governors (scaling_available_governors, + gov_num *CPUFREQ_NAME_LEN *sizeof (char ))) ) + {
tools/misc/xenpm.c static int get_cpufreq_para(struct xen_sysctl_pm_op *op) · line 279
Quoted 10 lines
tools/misc/xenpm.c +7
-1+xfree (scaling_available_governors); +return ret; + } + ret =copy_to_guest (op->u.get_para.scaling_available_governors, + scaling_available_governors, gov_num *CPUFREQ_NAME_LEN );xfree (scaling_available_governors); -return ret; +if ( ret ) +return ret; }
Even if the benefit may look tiny, code that can be shared should be shared. In the case here, the conditional return should be pulled out of the if and else branches. (It in fact seems possible to also pull out the copy_to_guest(), which would result in quite a bit less churn on the code.)
tools/misc/xenpm.c static int get_cpufreq_para(struct xen_sysctl_pm_op *op) · line 289
Quoted 12 lines
tools/misc/xenpm.c +10
-2- op->u.get_para.scaling_max_freq = policy->max; - op->u.get_para.scaling_min_freq = policy->min; +if ( internal_gov ) + { + op->u.get_para.scaling_max_perf = limits->max_perf_pct; + op->u.get_para.scaling_min_perf = limits->min_perf_pct; + op->u.get_para.scaling_turbo_pct = limits->turbo_pct; +if ( !strncmp (cpufreq_driver->name, +"intel_pstate" ,CPUFREQ_NAME_LEN ) ) + op->u.get_para.perf_alias =PERCENTAGE ; +else + op->u.get_para.perf_alias =FREQUENCY ;
Shouldn't the internal governor tell you, rather than keying this on its name?
Quoted 7 lines
tools/misc/xenpm.c +1
-0@@ -299,16 +357,36 @@ static int get_cpufreq_para(struct xen_sysctl_pm_op *op)static int set_cpufreq_gov (struct xen_sysctl_pm_op *op) {struct cpufreq_policy new_policy, *old_policy; +struct internal_governor *internal_gov; old_policy =per_cpu (cpufreq_cpu_policy, op->cpuid);
tools/misc/xenpm.c static int get_cpufreq_para(struct xen_sysctl_pm_op *op) · line 369
Quoted 10 lines
tools/misc/xenpm.c +2
-3if ( !old_policy )return -EINVAL ; + internal_gov = old_policy->internal_gov;memcpy (&new_policy, old_policy,sizeof (struct cpufreq_policy )); - new_policy.governor =__find_governor (op->u.set_gov.scaling_governor); -if (new_policy.governor ==NULL ) -return -EINVAL ; +if ( internal_gov && internal_gov->cur_gov )
Either the right side of the && has to go away (preferred), or it has to become != NON_INTERNAL_GOV.
tools/misc/xenpm.c static int get_cpufreq_para(struct xen_sysctl_pm_op *op) · line 382
Quoted 13 lines
tools/misc/xenpm.c +13
-0+ { +if ( !strnicmp (op->u.set_gov.scaling_governor, +"performance" ,CPUFREQ_NAME_LEN ) ) + internal_gov->cur_gov =INTERNAL_GOV_PERFORMANCE ; +else if ( !strnicmp (op->u.set_gov.scaling_governor, +"powersave" ,CPUFREQ_NAME_LEN ) ) + internal_gov->cur_gov =INTERNAL_GOV_POWERSAVE ; +else if ( !strnicmp (op->u.set_gov.scaling_governor, +"userspace" ,CPUFREQ_NAME_LEN ) ) + internal_gov->cur_gov =INTERNAL_GOV_USERSPACE ; +else if ( !strnicmp (op->u.set_gov.scaling_governor, +"ondemand" ,CPUFREQ_NAME_LEN ) ) + internal_gov->cur_gov =INTERNAL_GOV_ONDEMAND ;
else ...
Quoted 15 lines
tools/misc/xenpm.c +3
-1@@ -317,10 +395,12 @@ static int set_cpufreq_para(struct xen_sysctl_pm_op *op) {int ret =0 ;struct cpufreq_policy *policy; +struct internal_governor *internal_gov; policy =per_cpu (cpufreq_cpu_policy, op->cpuid); + internal_gov = policy->internal_gov; -if ( !policy || !policy->governor ) +if ( !policy || (!policy->governor && !internal_gov) )return -EINVAL ; switch(op->u.set_para.ctrl_type)
tools/misc/xenpm.c static int set_cpufreq_para(struct xen_sysctl_pm_op *op) · line 413
Quoted 5 lines
tools/misc/xenpm.c +2
-0@@ -329,6 +409,9 @@ static int set_cpufreq_para(struct xen_sysctl_pm_op *op) {struct cpufreq_policy new_policy; +if ( !policy->governor || internal_gov ) +return -EINVAL ;
The "!policy->governor" part looks to be dead code, considering the check done in the previous hunk.
xen/include/public/sysctl.h typedef struct xen_ondemand xen_ondemand_t; · line 304
Quoted 11 lines
xen/include/public/sysctl.h +5
-0@@ -297,11 +297,28 @@ typedef struct xen_ondemand xen_ondemand_t;* same as sysfs file name of native linux */ #define CPUFREQ_NAME_LEN 16 + +enum xen_perf_alias { +FREQUENCY =0 , +PERCENTAGE =1 +};
The need proper prefixes to avoid name space collisions, e.g. XEN_CPUFREQ_MODE_ (and the enum name being "alias" is also odd, "mode" or some such would seem better there too).
Quoted 6 lines
xen/include/public/sysctl.h +1
-0struct xen_get_cpufreq_para {/* IN/OUT variable */ uint32_t cpu_num;uint32_t freq_num;uint32_t gov_num; +int32_t turbo_enabled;
xen/include/public/sysctl.h typedef struct xen_ondemand xen_ondemand_t; · line 321
Quoted 10 lines
xen/include/public/sysctl.h +10
-0+ +uint32_t cpuinfo_cur_freq; +uint32_t cpuinfo_max_freq; +uint32_t cpuinfo_min_freq; +uint32_t scaling_cur_freq; + +uint32_t scaling_turbo_pct; +uint32_t scaling_max_perf; +uint32_t scaling_min_perf; +enum xen_perf_alias perf_alias;
You shouldn't use fields of non-fixed type in the public interface.
Quoted 12 lines
xen/include/public/sysctl.h +0
-7@@ -309,23 +326,13 @@ struct xen_get_cpufreq_para {XEN_GUEST_HANDLE_64 (uint32) scaling_available_frequencies;XEN_GUEST_HANDLE_64 (char ) scaling_available_governors;char scaling_driver[CPUFREQ_NAME_LEN ]; - -uint32_t cpuinfo_cur_freq; -uint32_t cpuinfo_max_freq; -uint32_t cpuinfo_min_freq; -uint32_t scaling_cur_freq; -char scaling_governor[CPUFREQ_NAME_LEN ]; -uint32_t scaling_max_freq;
xen/include/public/sysctl.h struct xen_get_cpufreq_para { · line 336
Quoted 10 lines
xen/include/public/sysctl.h +0
-3-uint32_t scaling_min_freq;/* for specific governor */ union {struct xen_userspace userspace;struct xen_ondemand ondemand; } u; - -int32_t turbo_enabled; };
Just to repeat myself: Too much shuffling for no reason. I can see why you want turbo_enabled go up (to reduce holes), but I don't see the point of any of the other moving around of members.
Jan
PATCH v6 6/6 tools: enable xenpm to control the intel_pstate driver
In reply to Wei WangThe intel_pstate driver receives percentage values to set the performance limits. This patch adds interfaces to support the input of percentage values to control the intel_pstate driver. The "get-cpufreq-para" is modified to show percentage based feedback info.
Also, some changes in identation are made to make the printed info looks tidy.
---
changes in v6:
No big change in this version, since we did not get any comments from the maintainers in the previous version.
tools/misc/xenpm.c | 116 +++++++++++++++++++++++++++++++++++++++++++---------- 1 file changed, 94 insertions(+), 22 deletions(-)
tools/misc/xenpm.c +94 -22
@@ -32,6 +32,11 @@#define MAX_CORE_RESIDENCIES 8#define ARRAY_SIZE (a) (sizeof (a) / sizeof ((a)[0])) +#define min_t (type,x,y) \ + ({ type __x = (x); type __y = (y); __x < __y ? __x: __y; }) +#define max_t (type,x,y) \ + ({ type __x = (x); type __y = (y); __x > __y ? __x: __y; }) +#define clamp_t (type, val, lo, hi) min_t(type, max_t(type, val, lo), hi)static xc_interface *xc_handle;static unsigned int max_cpu_nr;
@@ -46,6 +51,9 @@ void show_help(void)" get-cpuidle-states [cpuid] list cpu idle info of CPU <cpuid> or all\n" " get-cpufreq-states [cpuid] list cpu freq info of CPU <cpuid> or all\n" " get-cpufreq-para [cpuid] list cpu freq parameter of CPU <cpuid> or all\n" +" set-scaling-max-pct [cpuid] <num> set max performance limit in percentage\n" +" or as scaling speed in percentage in userspace governor\n" +" set-scaling-min-pct [cpuid] <num> set min performance limit in percentage\n" " set-scaling-maxfreq [cpuid] <HZ> set max cpu frequency <HZ> on CPU <cpuid>\n" " or all CPUs\n" " set-scaling-minfreq [cpuid] <HZ> set min cpu frequency <HZ> on CPU <cpuid>\n"
@@ -59,10 +67,10 @@ void show_help(void)" set-up-threshold [cpuid] <num> set up threshold on CPU <cpuid> or all\n" " it is used in ondemand governor.\n" " get-cpu-topology get thread/core/socket topology info\n" -" set-sched-smt enable|disable enable/disable scheduler smt power saving\n" +" set-sched-smt enable|disable enable/disable scheduler smt power saving\n" " set-vcpu-migration-delay <num> set scheduler vcpu migration delay in us\n" " get-vcpu-migration-delay get scheduler vcpu migration delay\n" -" set-max-cstate <num> set the C-State limitation (<num> >= 0)\n" +" set-max-cstate <num> set the C-State limitation (<num> >= 0)\n" " start [seconds] start collect Cx/Px statistics,\n" " output after CTRL-C or SIGINT or several seconds.\n" " enable-turbo-mode [cpuid] enable Turbo Mode for processors that support it.\n"
@@ -677,37 +685,51 @@ static void print_cpufreq_para(int cpuid, struct xc_get_cpufreq_para *p_cpufreq)printf ("current_governor : %s\n" , p_cpufreq->scaling_governor);if ( !strncmp (p_cpufreq->scaling_governor, -"userspace" ,CPUFREQ_NAME_LEN ) ) +"userspace" ,CPUFREQ_NAME_LEN ) && +strncmp (p_cpufreq->scaling_driver, +"intel_pstate" ,CPUFREQ_NAME_LEN ) ) { -printf (" userspace specific :\n" ); -printf (" scaling_setspeed : %u\n" , +printf ("userspace specific :\n" ); +printf ("scaling_setspeed : %u\n" , p_cpufreq->u.userspace.scaling_setspeed); }else if ( !strncmp (p_cpufreq->scaling_governor, -"ondemand" ,CPUFREQ_NAME_LEN ) ) +"ondemand" ,CPUFREQ_NAME_LEN ) && +strncmp (p_cpufreq->scaling_driver, +"intel_pstate" ,CPUFREQ_NAME_LEN ) ) { -printf (" ondemand specific :\n" ); -printf (" sampling_rate : max [%u] min [%u] cur [%u]\n" , +printf ("ondemand specific :\n" ); +printf ("sampling_rate : max [%u] min [%u] cur [%u]\n" , p_cpufreq->u.ondemand.sampling_rate_max, p_cpufreq->u.ondemand.sampling_rate_min, p_cpufreq->u.ondemand.sampling_rate); -printf (" up_threshold : %u\n" , +printf ("up_threshold : %u\n" , p_cpufreq->u.ondemand.up_threshold); } -printf ("scaling_avail_freq :" ); -for ( i =0 ; i < p_cpufreq->freq_num; i++ ) -if ( p_cpufreq->scaling_available_frequencies[i] == - p_cpufreq->scaling_cur_freq ) -printf (" *%d" , p_cpufreq->scaling_available_frequencies[i]); -else -printf (" %d" , p_cpufreq->scaling_available_frequencies[i]); -printf ("\n" ); - -printf ("scaling frequency : max [%u] min [%u] cur [%u]\n" , - p_cpufreq->scaling_max_perf, - p_cpufreq->scaling_min_perf, - p_cpufreq->scaling_cur_freq); +switch ( p_cpufreq->perf_alias ) + { +case PERCENTAGE : +printf ("max_perf_pct : %d\n" , p_cpufreq->scaling_max_perf); +printf ("min_perf_pct : %d\n" , p_cpufreq->scaling_min_perf); +printf ("turbo_pct : %d\n" , p_cpufreq->scaling_turbo_pct); +break ; +case FREQUENCY : +default : +printf ("scaling_avail_freq :" ); +for ( i =0 ; i < p_cpufreq->freq_num; i++ ) +if ( p_cpufreq->scaling_available_frequencies[i] == + p_cpufreq->scaling_cur_freq ) +printf (" *%d" , p_cpufreq->scaling_available_frequencies[i]); +else +printf (" %d" , p_cpufreq->scaling_available_frequencies[i]); +printf ("\n" ); +printf ("scaling frequency : max [%u] min [%u] cur [%u]\n" , + p_cpufreq->scaling_max_perf, + p_cpufreq->scaling_min_perf, + p_cpufreq->scaling_cur_freq); +break ; + }printf ("turbo mode : %s\n" , p_cpufreq->turbo_enabled ?"enabled" :"disabled or n/a" );
@@ -854,6 +876,54 @@ void scaling_min_freq_func(int argc, char *argv[]) } } +void scaling_max_pct_func (int argc,char *argv[]) +{ +int cpuid =-1 , pct =-1 ; + +parse_cpuid_and_int (argc, argv, &cpuid, &pct,"percentage" ); + pct =clamp_t (int, pct,0 ,100 ); + +if ( cpuid <0 ) + { +int i; +for ( i =0 ; i < max_cpu_nr; i++ ) +if (xc_set_cpufreq_para (xc_handle, i,SCALING_MAX_PCT , pct) ) +fprintf (stderr, +"[CPU%d] failed to set scaling max freq (%d - %s)\n" , + i, errno,strerror (errno)); + } +else + { +if (xc_set_cpufreq_para (xc_handle, cpuid,SCALING_MAX_PCT , pct) ) +fprintf (stderr,"failed to set scaling max freq (%d - %s)\n" , + errno,strerror (errno)); + } +} + +void scaling_min_pct_func (int argc,char *argv[]) +{ +int cpuid =-1 , pct =-1 ; + +parse_cpuid_and_int (argc, argv, &cpuid, &pct,"percentage" ); + pct =clamp_t (int, pct,0 ,100 ); + +if ( cpuid <0 ) + { +int i; +for ( i =0 ; i < max_cpu_nr; i++ ) +if (xc_set_cpufreq_para (xc_handle, i,SCALING_MIN_PCT , pct) ) +fprintf (stderr, +"[CPU%d] failed to set scaling max pct (%d - %s)\n" , + i, errno,strerror (errno)); + } +else + { +if (xc_set_cpufreq_para (xc_handle, cpuid,SCALING_MIN_PCT , pct) ) +fprintf (stderr,"failed to set scaling min pct (%d - %s)\n" , + errno,strerror (errno)); + } +} +void scaling_speed_func (int argc,char *argv[]) {int cpuid =-1 , speed =-1 ;
@@ -1133,6 +1203,8 @@ struct { { "get-cpufreq-para", cpufreq_para_func }, { "set-scaling-maxfreq", scaling_max_freq_func }, { "set-scaling-minfreq", scaling_min_freq_func }, + { "set-scaling-max-pct", scaling_max_pct_func}, + { "set-scaling-min-pct", scaling_min_pct_func}, { "set-scaling-governor", scaling_governor_func }, { "set-scaling-speed", scaling_speed_func }, { "set-sampling-rate", scaling_sampling_rate_func },
Signature · 1 line
1.9.1
Re: [PATCH v6 6/6] tools: enable xenpm to control the intel_pstate driver
Quoted 9 lines · Wei Wang
The
intel_pstatedriver receives percentage values to set the performance limits. This patch adds interfaces to support the input of percentage values to control theintel_pstatedriver. The "get-cpufreq-para" is modified to show percentage based feedback info.
Also, some changes in identation are made to make the printed info looks tidy.Signed-off-by Wei Wang <[email protected]>