Porting the intel_pstate driver to Xen
v5 cover Porting the intel_pstate driver to Xen
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 (9):
x86/intel_pstate: add some calculation related support x86/intel_pstate: APERF/MPERF feature detect x86/intel_pstate: add a new driver interface, setpolicy() x86/intel_pstate: relocate the driver register function x86/intel_pstate: changes in cpufreq_del_cpu for CPU offline x86/intel_pstate: the main boby 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 | 21 +- 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 | 884 +++++++++++++++++++++++++++++++ xen/arch/x86/acpi/cpufreq/powernow.c | 6 +- xen/arch/x86/cpu/common.c | 4 + xen/arch/x86/oprofile/op_model_athlon.c | 9 - xen/drivers/acpi/pmstat.c | 179 ++++++- xen/drivers/cpufreq/cpufreq.c | 21 +- xen/drivers/cpufreq/utility.c | 3 + xen/include/acpi/cpufreq/cpufreq.h | 54 +- xen/include/asm-x86/cpufeature.h | 5 + xen/include/asm-x86/cpufreq.h | 34 ++ xen/include/asm-x86/div64.h | 79 +++ xen/include/asm-x86/msr-index.h | 3 + xen/include/public/sysctl.h | 29 +- xen/include/xen/kernel.h | 23 + 20 files changed, 1384 insertions(+), 125 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 v5 1/9 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).
--- xen/arch/x86/oprofile/op_model_athlon.c | 9 ---- xen/include/asm-x86/div64.h | 79 +++++++++++++++++++++++++++++++++ xen/include/xen/kernel.h | 23 ++++++++++ 3 files changed, 102 insertions(+), 9 deletions(-)
changes in v5:
1) add clamp(), a type checking variant of clamp_t();
2) remove the private copy of clamp() in op_model_athlon.c.
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 +79 -0
83 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((typeof(val))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 v5 1/9] x86/intel_pstate: add some calculation related support
Quoted 39 lines · Wei Wang
The added calculation related functions will be used in the
intel_pstate.c. They are copied from the Linuxkernel(commit 2418f4f2, f3002134, eb18cba7).Signed-off-by Wei Wang <[email protected]>--- xen/arch/x86/oprofile/op_model_athlon.c | 9 ---- xen/include/asm-x86/div64.h | 79 +++++++++++++++++++++++++++++++++ xen/include/xen/kernel.h | 23 ++++++++++ 3 files changed, 102 insertions(+), 9 deletions(-)changes in v5:
1) addclamp(), a type checking variant ofclamp_t();
2) remove the private copy ofclamp()inop_model_athlon.c.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 +0
-0
xen/include/asm-x86/div64.h line 13
Quoted 3 lines
xen/include/asm-x86/div64.h +0
-0@@ -11,4 +11,83 @@ __rem; \ })
Comment to explain the functionality?
xen/include/asm-x86/div64.h line 15
Quoted 2 lines
xen/include/asm-x86/div64.h +2
-0+static inline uint64_t div_u64_rem (uint64_t dividend,uint32_t divisor, +uint32_t *remainder)
Alignment
xen/include/asm-x86/div64.h
Quoted 13 lines
xen/include/asm-x86/div64.h +8
-0+{ + *remainder =do_div (dividend, divisor); +return dividend; +} + +static inline uint64_t div_u64 (uint64_t dividend,uint32_t divisor) + +/** * container_of - cast a member of a structure out to the containing structure * * @ptr: the pointer to the member.
Too many spaces before "divisor"
xen/include/asm-x86/div64.h line 31
Quoted 10 lines
xen/include/asm-x86/div64.h +10
-0+{ +uint32_t remainder; + +return div_u64_rem (dividend, divisor, &remainder); +} + +/* +* div64_u64 - unsigned 64bit divide with 64bit divisor +* @dividend: 64bit dividend +* @divisor: 64bit divisor
Alignment also looks wonky here.
Quoted 18 lines
+ *
+ * This implementation is a modified version of the algorithm proposed
+ * by the book 'Hacker's Delight'. The original source and full proof
+ * can be found here and is available for use without restriction.
+ *
+ * 'http://www.hackersdelight.org/HDcode/newCode/divDouble.c.txt'
+ */xen/include/asm-x86/div64.h +11
-0+static inline uint64_t div64_u64 (uint64_t dividend,uint64_t divisor) +{ +uint32_t high = divisor >>32 ; +uint64_t quot; + +if ( high ==0 ) + quot =div_u64 (dividend, divisor); +else + { +int n =1 +fls (high); +
xen/include/asm-x86/div64.h line 59
Quoted 10 lines
xen/include/asm-x86/div64.h +10
-0+ quot =div_u64 (dividend >> n, divisor >> n); + +if ( quot !=0 ) + quot--; +if ( (dividend - quot * divisor) >= divisor ) + quot++; + } +return quot; +} +
Comment?
xen/include/asm-x86/div64.h line 61
Quoted 2 lines
xen/include/asm-x86/div64.h +2
-0+static inline int64_t div_s64_rem (int64_t dividend,int32_t divisor, +int32_t *remainder)
Alignment.
xen/include/asm-x86/div64.h line 68
Quoted 7 lines
xen/include/asm-x86/div64.h +7
-0+{ +int64_t quotient; + +if ( dividend <0 ) + { + quotient =div_u64_rem (-dividend,ABS (divisor), + (uint32_t *)remainder);
Alignment.
xen/include/asm-x86/div64.h line 76
Quoted 8 lines
xen/include/asm-x86/div64.h +8
-0+ *remainder = -*remainder; +if ( divisor >0 ) + quotient = -quotient; + } +else + { + quotient =div_u64_rem (dividend,ABS (divisor), + (uint32_t *)remainder);
Alignment.
Quoted 24 lines
xen/include/asm-x86/div64.h +16
-0+if ( divisor <0 ) + quotient = -quotient; + } +return quotient; +} + +/* +* div_s64 - signed 64bit divide with 32bit divisor +*/ +static inline int64_t div_s64 (int64_t dividend,int32_t divisor) +{ +int32_t remainder; + +return div_s64_rem (dividend, divisor, &remainder); +} +#endif xen/include/xen/kernel.h +0
-0@@ -43,6 +43,29 @@#define MAX (x,y) ((x) > (y) ? (x) : (y))
xen/include/xen/kernel.h line 54
Quoted 10 lines
xen/include/xen/kernel.h +9
-0/** +* 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((typeof(val))max(val, lo), hi)
This is a change of behaviour from the clamp() you removed, as this now evaluates its arguments multiple times.
Please use a ({ }) style macro to avoid evaluating the arguments multiple times.
xen/include/xen/kernel.h line 66
Quoted 12 lines
xen/include/xen/kernel.h +12
-0+ +/* +* 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)
And here as well please.
~Andrew
Re: [PATCH v5 1/9] x86/intel_pstate: add some calculation related support
Quoted 21 lines
Quoted 1 line
Quoted 1 line · On 17.09.15 at 17:19, <[email protected]> wrote:
Quoted 5 lines · Wei Wang
xen/include/xen/kernel.h +0
-0@@ -43,6 +43,29 @@#define MAX (x,y) ((x) > (y) ? (x) : (y))xen/include/xen/kernel.h line 54
Quoted 10 lines
xen/include/xen/kernel.h +9
-0/** +* 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((typeof(val))max(val, lo), hi)This is a change of behaviour from the
clamp()you removed, as this now evaluates its arguments multiple times.
Quoted 2 lines
Please use a ({ }) style macro to avoid evaluating the arguments multiple times.
So where do you see the multiple argument evaluation? The only odd thing I can spot is the type cast on the result of max(), which ought to be superfluous (due to the strict type handling of both max() and min()).
Jan
PATCH v5 2/9 x86/intel_pstate: APERF/MPERF feature detect
In reply to Wei WangAdd support to detect the APERF/MPERF feature. Also, remove the identical code in cpufreq.c and powernow.c. This patch is independent of the earlier patches.
--- xen/arch/x86/acpi/cpufreq/cpufreq.c | 6 ++---- xen/arch/x86/acpi/cpufreq/powernow.c | 6 ++---- xen/arch/x86/cpu/common.c | 4 ++++ xen/include/asm-x86/cpufeature.h | 5 +++++ 4 files changed, 13 insertions(+), 8 deletions(-)
changes in v5:
1) define macros for 0x1 and CPUID leaf5;
2) add a statement stating that this patch is independent of the previous ones.
xen/arch/x86/acpi/cpufreq/cpufreq.c +2 -4
@@ -50,7 +50,6 @@ enum { };#define INTEL_MSR_RANGE (0xffffull) -#define CPUID_6_ECX_APERFMPERF_CAPABILITY (0x1)struct acpi_cpufreq_data *cpufreq_drv_data[NR_CPUS ];
@@ -351,10 +350,9 @@ static unsigned int get_cur_freq_on_cpu(unsigned int cpu)static void feature_detect (void *info) {struct cpufreq_policy *policy = info; -unsigned int eax, ecx; +unsigned int eax; - ecx =cpuid_ecx (6 ); -if (ecx &CPUID_6_ECX_APERFMPERF_CAPABILITY ) { +if ( cpu_has_aperfmperf ) { policy->aperf_mperf =1 ; acpi_cpufreq_driver.getavg = get_measured_perf; }
xen/arch/x86/acpi/cpufreq/powernow.c +2 -4
@@ -37,7 +37,6 @@#include <acpi/acpi.h> #include <acpi/cpufreq/cpufreq.h> -#define CPUID_6_ECX_APERFMPERF_CAPABILITY (0x1)#define CPUID_FREQ_VOLT_CAPABILITIES 0x80000007#define CPB_CAPABLE 0x00000200#define USE_HW_PSTATE 0x00000080
@@ -211,10 +210,9 @@ static int powernow_cpufreq_verify(struct cpufreq_policy *policy)static void feature_detect (void *info) {struct cpufreq_policy *policy = info; -unsigned int ecx, edx; +unsigned int edx; - ecx =cpuid_ecx (6 ); -if (ecx &CPUID_6_ECX_APERFMPERF_CAPABILITY ) { +if ( cpu_has_aperfmperf ) { policy->aperf_mperf =1 ; powernow_cpufreq_driver.getavg = get_measured_perf; }
xen/arch/x86/cpu/common.c +4 -0
@@ -239,6 +239,10 @@ static void __cpuinit generic_identify(struct cpuinfo_x86 *c)if (cpu_has (c,X86_FEATURE_CLFLSH ) ) c->x86_clflush_size = ((ebx >>8 ) &0xff ) *8 ; +if ( (c->cpuid_level >CPUID_PM_LEAF ) && + (cpuid_ecx (CPUID_PM_LEAF ) &CPUID6_ECX_APERFMPERF_CAPABILITY ) ) +set_bit (X86_FEATURE_APERFMPERF , c->x86_capability); +/* AMD-defined flags: level 0x80000001 */ c->extended_cpuid_level =cpuid_eax (0x80000000 );if ( (c->extended_cpuid_level &0xffff0000 ) ==0x80000000 ) {
xen/include/asm-x86/cpufeature.h +5 -0
@@ -69,6 +69,7 @@#define X86_FEATURE_XTOPOLOGY (3*32+13)/* cpu topology enum extensions */ #define X86_FEATURE_CPUID_FAULTING (3*32+14)/* cpuid faulting */ #define X86_FEATURE_CLFLUSH_MONITOR (3*32+15)/* clflush reqd with monitor */ +#define X86_FEATURE_APERFMPERF (3*32+28)/* APERFMPERF */ /* Intel-defined CPU features, CPUID level 0x00000001 (ecx), word 4 */ #define X86_FEATURE_XMM3 (4*32+ 0)/* Streaming SIMD Extensions-3 */
@@ -165,6 +166,9 @@#define CPUID5_ECX_EXTENSIONS_SUPPORTED 0x1#define CPUID5_ECX_INTERRUPT_BREAK 0x2 +#define CPUID_PM_LEAF 6 +#define CPUID6_ECX_APERFMPERF_CAPABILITY 0x1 +#define cpu_has_vme 0#define cpu_has_de 1#define cpu_has_pse 1
@@ -190,6 +194,7 @@#define cpu_has_page1gb boot_cpu_has(X86_FEATURE_PAGE1GB)#define cpu_has_efer 1#define cpu_has_fsgsbase boot_cpu_has(X86_FEATURE_FSGSBASE) +#define cpu_has_aperfmperf boot_cpu_has(X86_FEATURE_APERFMPERF)#define cpu_has_smep boot_cpu_has(X86_FEATURE_SMEP)#define cpu_has_smap boot_cpu_has(X86_FEATURE_SMAP)
Signature · 1 line
1.9.1
Re: [PATCH v5 2/9] x86/intel_pstate: APERF/MPERF feature detect
Quoted 5 lines · Wei Wang
Add support to detect the APERF/MPERF feature. Also, remove the identical code in
cpufreq.candpowernow.c. This patch is independent of the earlier patches.Signed-off-by Wei Wang <[email protected]>
Re: [PATCH v5 2/9] x86/intel_pstate: APERF/MPERF feature detect
In reply to Wei WangQuoted 5 lines
Quoted 1 line
Quoted 1 line · On 14.09.15 at 04:32, <[email protected]> wrote:
changes in v5:
1) define macros for0x1and CPUID leaf5;
2) add a statement stating that this patch is independent of the previous ones.
This statement doesn't belong in the commit message, i.e. should go after the first ---.
Quoted 11 lines
xen/arch/x86/acpi/cpufreq/cpufreq.c +0
-1@@ -50,7 +50,6 @@ enum { };#define INTEL_MSR_RANGE (0xffffull) -#define CPUID_6_ECX_APERFMPERF_CAPABILITY (0x1)struct acpi_cpufreq_data *cpufreq_drv_data[NR_CPUS ];
xen/arch/x86/acpi/cpufreq/cpufreq.c static unsigned int get_cur_freq_on_cpu(unsigned int cpu) · line 355
Quoted 9 lines
xen/arch/x86/acpi/cpufreq/cpufreq.c +2
-3@@ -351,10 +350,9 @@ static unsigned int get_cur_freq_on_cpu(unsigned int cpu)static void feature_detect (void *info) {struct cpufreq_policy *policy = info; -unsigned int eax, ecx; +unsigned int eax; - ecx =cpuid_ecx (6 ); -if (ecx &CPUID_6_ECX_APERFMPERF_CAPABILITY ) { +if ( cpu_has_aperfmperf ) {
You should have fully fixed coding style here.
xen/arch/x86/cpu/common.c static void __cpuinit generic_identify(struct cpuinfo_x86 *c) · line 243
Quoted 8 lines
xen/arch/x86/cpu/common.c +2
-0@@ -239,6 +239,10 @@ static void __cpuinit generic_identify(struct cpuinfo_x86 *c)if (cpu_has (c,X86_FEATURE_CLFLSH ) ) c->x86_clflush_size = ((ebx >>8 ) &0xff ) *8 ; +if ( (c->cpuid_level >CPUID_PM_LEAF ) && + (cpuid_ecx (CPUID_PM_LEAF ) &CPUID6_ECX_APERFMPERF_CAPABILITY ) )
Indentation.
xen/include/asm-x86/cpufeature.h line 72
Quoted 7 lines
xen/include/asm-x86/cpufeature.h +1
-0@@ -69,6 +69,7 @@#define X86_FEATURE_XTOPOLOGY (3*32+13)/* cpu topology enum extensions */ #define X86_FEATURE_CPUID_FAULTING (3*32+14)/* cpuid faulting */ #define X86_FEATURE_CLFLUSH_MONITOR (3*32+15)/* clflush reqd with monitor */ +#define X86_FEATURE_APERFMPERF (3*32+28)/* APERFMPERF */
Why 28 instead of the consecutive 16?
xen/include/asm-x86/cpufeature.h line 197
Quoted 5 lines
xen/include/asm-x86/cpufeature.h +1
-0@@ -190,6 +194,7 @@#define cpu_has_page1gb boot_cpu_has(X86_FEATURE_PAGE1GB)#define cpu_has_efer 1#define cpu_has_fsgsbase boot_cpu_has(X86_FEATURE_FSGSBASE) +#define cpu_has_aperfmperf boot_cpu_has(X86_FEATURE_APERFMPERF)
Should use tab for aligning the right side to match the lines right above.
I've taken care of all of these for you, but I would really appreciate you taking care of such going forward.
Jan
PATCH v5 3/9 x86/intel_pstate: add a new driver interface, setpolicy()
In reply to Wei WangIn order to better support future Intel processors, intel_pstate changes to use percentage values to tune P-states. The setpolicy driver interface is used to configure the intel_pstate internal policy. The __cpufreq_set_policy needs to be intercepted to use the setpolicy driver if it exists.
--- xen/drivers/cpufreq/utility.c | 3 +++ xen/include/acpi/cpufreq/cpufreq.h | 14 ++++++++++++++ 2 files changed, 17 insertions(+)
changes in v5:
1) delay the addition of the structures that are used in later patches.
xen/drivers/cpufreq/utility.c +3 -0
@@ -456,6 +456,9 @@ int __cpufreq_set_policy(struct cpufreq_policy *data, data->min = policy->min; data->max = policy->max; + data->limits = policy->limits; +if (cpufreq_driver->setpolicy) +return cpufreq_driver->setpolicy (data);if (policy->governor != data->governor) {/* save old, working values */
xen/include/acpi/cpufreq/cpufreq.h +14 -0
@@ -41,6 +41,18 @@ struct cpufreq_cpuinfo {unsigned int transition_latency;/* in 10^(-9) s = nanoseconds */ }; +struct perf_limits { +bool_t no_turbo; +bool_t turbo_disabled; +uint32_t turbo_pct; +uint32_t max_perf_pct;/* max performance in percentage */ +uint32_t min_perf_pct;/* min performance in percentage */ +uint32_t max_perf; +uint32_t min_perf; +uint32_t max_policy_pct; +uint32_t min_policy_pct; +}; +struct cpufreq_policy {cpumask_var_t cpus;/* affected CPUs */ unsigned int shared_type;/* ANY or ALL affected CPUs
@@ -52,6 +64,7 @@ struct cpufreq_policy {unsigned int max;/* in kHz */ unsigned int cur;/* in kHz, only needed if cpufreq * governors are used */ +struct perf_limits limits;struct cpufreq_governor *governor;bool_t resume;/* flag for cpufreq 1st run
@@ -145,6 +158,7 @@ struct cpufreq_driver {char name[CPUFREQ_NAME_LEN ];int (*init)(struct cpufreq_policy *policy);int (*verify)(struct cpufreq_policy *policy); +int (*setpolicy)(struct cpufreq_policy *policy);int (*update)(int cpuid,struct cpufreq_policy *policy); int (*target)(struct cpufreq_policy *policy, unsigned int target_freq,
Signature · 1 line
1.9.1
Re: [PATCH v5 3/9] x86/intel_pstate: add a new driver interface, setpolicy()
Quoted 7 lines · Wei Wang
In order to better support future Intel processors,
intel_pstatechanges to use percentage values to tune P-states. The setpolicy driver interface is used to configure theintel_pstateinternal policy. The__cpufreq_set_policyneeds to be intercepted to use the setpolicy driver if it exists.Signed-off-by Wei Wang <[email protected]>
Re: [PATCH v5 3/9] x86/intel_pstate: add a new driver interface, setpolicy()
In reply to Wei WangQuoted 8 lines
Quoted 1 line
Quoted 1 line · On 14.09.15 at 04:32, <[email protected]> wrote:
In order to better support future Intel processors,
intel_pstatechanges to use percentage values to tune P-states. The setpolicy driver interface is used to configure theintel_pstateinternal policy. The__cpufreq_set_policyneeds to be intercepted to use the setpolicy driver if it exists.Signed-off-by Wei Wang <[email protected]>
Andrew having given his R-b I'm going to apply this as is, but I don't see how ...
xen/drivers/cpufreq/utility.c int __cpufreq_set_policy(struct cpufreq_policy *data, · line 459
Quoted 7 lines
xen/drivers/cpufreq/utility.c +1
-0@@ -456,6 +456,9 @@ int __cpufreq_set_policy(struct cpufreq_policy *data, data->min = policy->min; data->max = policy->max; + data->limits = policy->limits;
... this and ...
Quoted 7 lines
xen/include/acpi/cpufreq/cpufreq.h +1
-0@@ -41,6 +41,18 @@ struct cpufreq_cpuinfo {unsigned int transition_latency;/* in 10^(-9) s = nanoseconds */ }; +struct perf_limits {
xen/include/acpi/cpufreq/cpufreq.h struct cpufreq_cpuinfo { · line 54
Quoted 10 lines
xen/include/acpi/cpufreq/cpufreq.h +10
-0+bool_t no_turbo; +bool_t turbo_disabled; +uint32_t turbo_pct; +uint32_t max_perf_pct;/* max performance in percentage */ +uint32_t min_perf_pct;/* min performance in percentage */ +uint32_t max_perf; +uint32_t min_perf; +uint32_t max_policy_pct; +uint32_t min_policy_pct; +};
.. this are related to the patch subject. In particular it is impossible to tell at this point whether and how all of the fields above are going to be used. Please try to split patch series into _logical_ pieces.
Jan
PATCH v5 4/9 x86/intel_pstate: relocate the driver register function
In reply to Wei WangMove the driver register function to
the cpufreq.c.
--- xen/drivers/cpufreq/cpufreq.c | 15 +++++++++++++++ xen/include/acpi/cpufreq/cpufreq.h | 27 +-------------------------- 2 files changed, 16 insertions(+), 26 deletions(-)
changes in v5:
1) keep cpufreq_presmp_init() intact.
xen/drivers/cpufreq/cpufreq.c +15 -0
@@ -638,3 +638,18 @@ static int __init cpufreq_presmp_init(void) }presmp_initcall (cpufreq_presmp_init); +int cpufreq_register_driver (struct cpufreq_driver *driver_data) +{ +if ( !driver_data || !driver_data->init || + !driver_data->verify || !driver_data->exit || + (!driver_data->target == !driver_data->setpolicy) ) +return -EINVAL ; + +if ( cpufreq_driver ) +return -EBUSY ; + + cpufreq_driver = driver_data; + +register_cpu_notifier (&cpu_nfb); +return 0 ; +}
xen/include/acpi/cpufreq/cpufreq.h +1 -26
@@ -170,32 +170,7 @@ struct cpufreq_driver {extern struct cpufreq_driver *cpufreq_driver; -static __inline__ -int cpufreq_register_driver (struct cpufreq_driver *driver_data) -{ -if (!driver_data || - !driver_data->init || - !driver_data->exit || - !driver_data->verify || - !driver_data->target) -return -EINVAL ; - -if (cpufreq_driver) -return -EBUSY ; - - cpufreq_driver = driver_data; -return 0 ; -} - -static __inline__ -int cpufreq_unregister_driver (struct cpufreq_driver *driver) -{ -if (!cpufreq_driver || (driver != cpufreq_driver)) -return -EINVAL ; - - cpufreq_driver =NULL ; -return 0 ; -} +extern int cpufreq_register_driver (struct cpufreq_driver *driver_data); static __inline__ void cpufreq_verify_within_limits(struct cpufreq_policy *policy,
Signature · 1 line
1.9.1
Re: [PATCH v5 4/9] x86/intel_pstate: relocate the driver register function
Quoted 17 lines · Wei Wang
Move the driver register function to
thecpufreq.c.Signed-off-by Wei Wang <[email protected]>--- xen/drivers/cpufreq/cpufreq.c | 15 +++++++++++++++ xen/include/acpi/cpufreq/cpufreq.h | 27 +-------------------------- 2 files changed, 16 insertions(+), 26 deletions(-)changes in v5:
1) keepcpufreq_presmp_init()intact.xen/drivers/cpufreq/cpufreq.c +0
-0
xen/drivers/cpufreq/cpufreq.c static int __init cpufreq_presmp_init(void) · line 645
Quoted 8 lines
xen/drivers/cpufreq/cpufreq.c +5
-0@@ -638,3 +638,18 @@ static int __init cpufreq_presmp_init(void) }presmp_initcall (cpufreq_presmp_init); +int cpufreq_register_driver (struct cpufreq_driver *driver_data) +{ +if ( !driver_data || !driver_data->init || + !driver_data->verify || !driver_data->exit || + (!driver_data->target == !driver_data->setpolicy) )
This line will incur the wrath of newer GCC's which have warnings against such logic.
Either bracket the (!driver_data->$X) or alter the logic itself.
Also, the commit message needs to be corrected to state that you are also introducing a new check to the registration function.
~Andrew
Re: [PATCH v5 4/9] x86/intel_pstate: relocate the driver register function
Quoted 22 lines
Quoted 1 line
Quoted 1 line · On 17.09.15 at 17:38, <[email protected]> wrote:
Quoted 20 lines · Wei Wang
Move the driver register function to
thecpufreq.c.Signed-off-by Wei Wang <[email protected]>--- xen/drivers/cpufreq/cpufreq.c | 15 +++++++++++++++ xen/include/acpi/cpufreq/cpufreq.h | 27 +-------------------------- 2 files changed, 16 insertions(+), 26 deletions(-)changes in v5:
1) keepcpufreq_presmp_init()intact.xen/drivers/cpufreq/cpufreq.c +0
-0@@ -638,3 +638,18 @@ static int __init cpufreq_presmp_init(void) }presmp_initcall (cpufreq_presmp_init);
Quoted 8 lines
xen/drivers/cpufreq/cpufreq.c static int __init cpufreq_presmp_init(void) · line 645
Quoted 5 lines
xen/drivers/cpufreq/cpufreq.c +5
-0+int cpufreq_register_driver (struct cpufreq_driver *driver_data) +{ +if ( !driver_data || !driver_data->init || + !driver_data->verify || !driver_data->exit || + (!driver_data->target == !driver_data->setpolicy) )This line will incur the wrath of newer GCC's which have warnings against such logic.
Hmm, I think we have other instances of such, without gcc complaining. Iirc there was a bug in an early 5.0-rc which got fixed in the final 5.1.0.
Quoted 1 line
Either bracket the (!driver_data->$X) or alter the logic itself.
I'd prefer to avoid either change.
Jan
Re: [PATCH v5 4/9] x86/intel_pstate: relocate the driver register function
In reply to Wei WangQuoted 3 lines
Quoted 1 line
Quoted 1 line · On 14.09.15 at 04:32, <[email protected]> wrote:
Move the driver register function to
thecpufreq.c.
... and remove the (unused) de-registration one.
Jan
PATCH v5 5/9 x86/intel_pstate: changes in cpufreq_del_cpu for CPU offline
In reply to Wei WangWe change to NULL the cpufreq_cpu_policy pointer after the call of cpufreq_driver->exit, because the pointer is still needed in intel_pstate_set_pstate().
--- xen/drivers/cpufreq/cpufreq.c | 6 +++--- xen/include/acpi/cpufreq/cpufreq.h | 7 +++++++ 2 files changed, 10 insertions(+), 3 deletions(-)
changes in v5:
1) put this patch prior to the "main body of intel pstate driver", which is one of the acceptable options suggested by the Jan.
xen/drivers/cpufreq/cpufreq.c +3 -3
@@ -334,12 +334,11 @@ 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); -per_cpu (cpufreq_cpu_policy, cpu) =NULL ;cpumask_clear_cpu (cpu, policy->cpus);cpumask_clear_cpu (cpu, cpufreq_dom->map);
@@ -348,6 +347,7 @@ int cpufreq_del_cpu(unsigned int cpu)free_cpumask_var (policy->cpus);xfree (policy); } +per_cpu (cpufreq_cpu_policy, cpu) =NULL ;/* for the last cpu of the domain, clean room */ /* It's safe here to free freq_table, drv_data and policy */
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 v5 5/9] x86/intel_pstate: changes in cpufreq_del_cpu for CPU offline
Quoted 19 lines · Wei Wang
We change to NULL the
cpufreq_cpu_policypointer after the call ofcpufreq_driver->exit, because the pointer is still needed inintel_pstate_set_pstate().Signed-off-by Wei Wang <[email protected]>--- xen/drivers/cpufreq/cpufreq.c | 6 +++--- xen/include/acpi/cpufreq/cpufreq.h | 7 +++++++ 2 files changed, 10 insertions(+), 3 deletions(-)changes in v5:
1) put this patch prior to the "main body of intel pstate driver", which is one of the acceptable options suggested by the Jan.xen/drivers/cpufreq/cpufreq.c +0
-0
xen/drivers/cpufreq/cpufreq.c int cpufreq_del_cpu(unsigned int cpu) · line 338
Quoted 7 lines
xen/drivers/cpufreq/cpufreq.c +2
-2@@ -334,12 +334,11 @@ 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)))
This will be easier to read if you change the location of the linebreak as such:
perf->domain_info.num_processors)))
if (!policy->internal_gov &&
(hw_all || (cpumask_weight(cpufreq_dom->map) ==
perf->domain_info.num_processors)))Quoted 20 lines
xen/drivers/cpufreq/cpufreq.c +1
-1__cpufreq_governor (policy,CPUFREQ_GOV_STOP );cpufreq_statistic_exit (cpu); -per_cpu (cpufreq_cpu_policy, cpu) =NULL ;cpumask_clear_cpu (cpu, policy->cpus);cpumask_clear_cpu (cpu, cpufreq_dom->map);@@ -348,6 +347,7 @@ int cpufreq_del_cpu(unsigned int cpu)free_cpumask_var (policy->cpus);xfree (policy); } +per_cpu (cpufreq_cpu_policy, cpu) =NULL ;/* for the last cpu of the domain, clean room */ /* It's safe here to free freq_table, drv_data and policy */ xen/include/acpi/cpufreq/cpufreq.h +0
-0
xen/include/acpi/cpufreq/cpufreq.h struct perf_limits { · line 61
Quoted 9 lines
xen/include/acpi/cpufreq/cpufreq.h +6
-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; +}; +
Adding this structure needs a mention in the commit message.
~Andrew
Re: [PATCH v5 5/9] x86/intel_pstate: changes in cpufreq_del_cpu for CPU offline
In reply to Wei WangQuoted 6 lines
Quoted 1 line
Quoted 1 line · On 14.09.15 at 04:32, <[email protected]> wrote:
We change to NULL the
cpufreq_cpu_policypointer after the call ofcpufreq_driver->exit, because the pointer is still needed inintel_pstate_set_pstate().Signed-off-by Wei Wang <[email protected]>
Quoted 8 lines
--- xen/drivers/cpufreq/cpufreq.c | 6 +++--- xen/include/acpi/cpufreq/cpufreq.h | 7 +++++++ 2 files changed, 10 insertions(+), 3 deletions(-)changes in v5:
1) put this patch prior to the "main body of intel pstate driver", which is one of the acceptable options suggested by the Jan.
Did I? With the description above, I still don't see why this would be needed (here or later):
Quoted 4 lines
xen/drivers/cpufreq/cpufreq.c +0
-0@@ -334,12 +334,11 @@ int cpufreq_del_cpu(unsigned int cpu)
xen/drivers/cpufreq/cpufreq.c int cpufreq_del_cpu(unsigned int cpu) · line 342
Quoted 10 lines
xen/drivers/cpufreq/cpufreq.c +2
-3/* 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); -per_cpu (cpufreq_cpu_policy, cpu) =NULL ;
Prior to this line we still have policy == per_cpu(cpufreq_cpu_policy, cpu), i.e. the data needed is being made available to the ->exit() handler called a few lines down. I.e. if you want to convince me or anyone else that the change is needed, you will need to say in the description why that is not enough.
Quoted 16 lines
xen/include/acpi/cpufreq/cpufreq.h +6
-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
xen/include/acpi/cpufreq/cpufreq.h struct cpufreq_policy { · line 75
Quoted 4 lines
xen/include/acpi/cpufreq/cpufreq.h +1
-0@@ -66,6 +72,7 @@ struct cpufreq_policy {* governors are used */ struct perf_limits limits;struct cpufreq_governor *governor; +struct internal_governor *internal_gov;
Both of the changes to this file look unrelated to the purpose of the patch, as does the one consumer of the new pointer.
Jan
Re: [PATCH v5 5/9] x86/intel_pstate: changes in cpufreq_del_cpu for CPU offline
Quoted 24 lines · Jan Beulich
Quoted 24 lines
Quoted 1 line
Quoted 1 line · On 14.09.15 at 04:32, <[email protected]> wrote:
We change to NULL the
cpufreq_cpu_policypointer after the call ofcpufreq_driver->exit, because the pointer is still needed inintel_pstate_set_pstate().Signed-off-by Wei Wang <[email protected]>--- xen/drivers/cpufreq/cpufreq.c | 6 +++--- xen/include/acpi/cpufreq/cpufreq.h | 7 +++++++ 2 files changed, 10 insertions(+), 3 deletions(-)xen/drivers/cpufreq/cpufreq.c +1
-2@@ -334,12 +334,11 @@ 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);
Quoted 8 lines
Quoted 2 lines
cpufreq_statistic_exit(cpu);-
per_cpu(cpufreq_cpu_policy, cpu)= NULL;Prior to this line we still have policy ==
per_cpu(cpufreq_cpu_policy, cpu), i.e. the data needed is being made available to the ->exit() handler called a few lines down. I.e. if you want to convince me or anyone else that the change is needed, you will need to say in the description why that is not enough.
OK. Without this patch, we need to change intel_pstate_set_pstate() to receive the "policy" parameter from ->exit(), instead of referencing the global per_cpu() variable. The cumbersome part is that we need to change all other callers of intel_pstate_set_pstate() to get the "policy" parameter as well and pass it to the intel_pstate_set_pstate() function. I will include this change in the next version to see if this way is better.
Best,
Wei
PATCH v5 6/9 x86/intel_pstate: the main boby 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.
--- xen/arch/x86/acpi/cpufreq/Makefile | 1 + xen/arch/x86/acpi/cpufreq/intel_pstate.c | 880 +++++++++++++++++++++++++++++++ xen/include/acpi/cpufreq/cpufreq.h | 6 + xen/include/asm-x86/cpufreq.h | 34 ++ xen/include/asm-x86/msr-index.h | 3 + 5 files changed, 924 insertions(+) create mode 100644 xen/arch/x86/acpi/cpufreq/intel_pstate.c create mode 100644 xen/include/asm-x86/cpufreq.h
changes in v5:
1) customize it to Xen style.
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 +880 -0
880 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 +34 -0
@@ -0,0 +1,34 @@ +#ifndef _ASM_X86_CPUFREQ_H +#define _ASM_X86_CPUFREQ_H + +/* +* Copyright (C) 2015 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, write to the Free Software +* Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA +* +* ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ +*/ + +extern int intel_pstate_init (void ); + +/* +* 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 v5 6/9] x86/intel_pstate: the main boby of the intel_pstate driver
Quoted 2 lines · Wei Wang
We simply grab the fundamental logic of the
intel_pstatedriver from Linux kernel, and customize it to Xen style.
How much customisation? Is it just style, or other additions as well? (Deletions are less interesting)
For files we import directly from Linux, we keep Linux style to simplify taking new patches in the future. Whether it is sensible to do so here depends on how modified it is from Linux.
Quoted 17 lines
xen/include/asm-x86/cpufreq.h new file +11
-0@@ -0,0 +1,34 @@ +#ifndef _ASM_X86_CPUFREQ_H +#define _ASM_X86_CPUFREQ_H + +/* +* Copyright (C) 2015 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
xen/include/asm-x86/cpufreq.h line 21
Quoted 10 lines
xen/include/asm-x86/cpufreq.h new file +10
-0+* (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, write to the Free Software +* Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA
Per c/s 443701e "Replace FSF street address with canonical URL", please correct this GPL header.
~Andrew
Re: [PATCH v5 6/9] x86/intel_pstate: the main boby of the intel_pstate driver
In reply to Wei WangQuoted 11 lines
Quoted 1 line
Quoted 1 line · On 14.09.15 at 04:32, <[email protected]> wrote:
We simply grab the fundamental logic of the
intel_pstatedriver 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.hheader file is added.Signed-off-by Wei Wang <[email protected]>
I went back to the replies I sent on earlier versions (namely v2), and finding several issues un-addressed or only partly addressed I'm not going to re-review this patch in detail, NAK-ing it in its current shape.
Jan
PATCH v5 7/9 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.
--- docs/misc/xen-command-line.markdown | 7 +++++++ xen/arch/x86/acpi/cpufreq/cpufreq.c | 9 ++++++--- xen/arch/x86/acpi/cpufreq/intel_pstate.c | 4 ++++ 3 files changed, 17 insertions(+), 3 deletions(-)
changes in v5:
1) move the booting parameter into the intel_pstate_init() function - have
it be a local variable;
2) rename "intel_pstate_load" to "load".
docs/misc/xen-command-line.markdown +7 -0
@@ -849,6 +849,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 +6 -3
@@ -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,9 +648,11 @@ 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_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 ();
xen/arch/x86/acpi/cpufreq/intel_pstate.c +4 -0
@@ -843,7 +843,11 @@ int __init intel_pstate_init(void)int cpu, rc =0 ;const struct x86_cpu_id *id;struct cpu_defaults *cpu_info; +static bool_t __read_mostly load; +boolean_param ("intel_pstate" , load); +if ( !load ) +return -ENODEV ; id =x86_match_cpu (intel_pstate_cpu_ids);if ( !id )
Signature · 1 line
1.9.1
Re: [PATCH v5 7/9] x86/intel_pstate: add a booting param to select the driver to load
Quoted 53 lines · Wei Wang
By default, the old P-state driver (acpi-freq) is used. Adding "
intel_pstate" to the Xen booting param list to enable the use ofintel_pstate. However, ifintel_pstateis 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 ofintel_pstate.Also, adding the
intel_pstatebooting parameter to xen-command-line.markdown.Signed-off-by Wei Wang <[email protected]>--- docs/misc/xen-command-line.markdown | 7 +++++++ xen/arch/x86/acpi/cpufreq/cpufreq.c | 9 ++++++--- xen/arch/x86/acpi/cpufreq/intel_pstate.c | 4 ++++ 3 files changed, 17 insertions(+), 3 deletions(-)changes in v5:
1) move the booting parameter into theintel_pstate_init()function - haveit be a local variable;2) rename "
intel_pstate_load" to "load".docs/misc/xen-command-line.markdown +7
-0@@ -849,6 +849,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 +1
-0@@ -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>
xen/arch/x86/acpi/cpufreq/cpufreq.c static int __init cpufreq_driver_init(void) · line 654
Quoted 10 lines
xen/arch/x86/acpi/cpufreq/cpufreq.c +4
-3@@ -647,9 +648,11 @@ 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_INTEL )) { + ret =intel_pstate_init (); +if (ret) + ret =cpufreq_register_driver (&acpi_cpufreq_driver);
This looks like a mess (although not your fault to start with).
We shouldn't have multiple different top level command line options. In particular, having "mwait-idle" and "intel_pstate" seems wrong, given a perfectly good "cpufreq=" option.
Is the driver in use available to change at runtime from xenpm ?
xen/arch/x86/acpi/cpufreq/intel_pstate.c int __init intel_pstate_init(void) · line 846
Quoted 13 lines
xen/arch/x86/acpi/cpufreq/cpufreq.c +1
-0+ }else if ((cpufreq_controller == FREQCTL_xen) && (boot_cpu_data.x86_vendor ==X86_VENDOR_AMD )) ret =powernow_register_driver ();xen/arch/x86/acpi/cpufreq/intel_pstate.c +1
-0@@ -843,7 +843,11 @@ int __init intel_pstate_init(void)int cpu, rc =0 ;const struct x86_cpu_id *id;struct cpu_defaults *cpu_info; +static bool_t __read_mostly load;
__initdata
~Andrew
Re: [PATCH v5 7/9] x86/intel_pstate: add a booting param to select the driver to load
Quoted 4 lines
Quoted 1 line
Quoted 1 line · On 17.09.15 at 18:08, <[email protected]> wrote:
We shouldn't have multiple different top level command line options. In particular, having "mwait-idle" and "
intel_pstate" seems wrong, given a perfectly good "cpufreq=" option.
"mwait-idle" is C-state related.
Jan
Re: [PATCH v5 7/9] x86/intel_pstate: add a booting param to select the driver to load
In reply to Wei Wangxen/arch/x86/acpi/cpufreq/cpufreq.c line 43
Quoted 8 lines
Quoted 1 line
Quoted 1 line · On 14.09.15 at 04:32, <[email protected]> wrote:
xen/arch/x86/acpi/cpufreq/cpufreq.c +1
-0@@ -40,6 +40,7 @@#include <asm/processor.h> #include <asm/percpu.h> #include <asm/cpufeature.h> +#include <asm/cpufreq.h>
I think to make clear why this include is needed here it would be better if you added the declaration of intel_pstate_init() also in this patch instead of in the previous one.
Quoted 4 lines
xen/arch/x86/acpi/cpufreq/cpufreq.c +0
-0@@ -647,9 +648,11 @@ static int __init cpufreq_driver_init(void)int ret =0 ; if ((cpufreq_controller == FREQCTL_xen) &&
xen/arch/x86/acpi/cpufreq/cpufreq.c static int __init cpufreq_driver_init(void) · line 657
Quoted 10 lines
xen/arch/x86/acpi/cpufreq/cpufreq.c +5
-3- (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_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 ();
Since you're basically modifying the entire body of the function, please gets its coding style corrected as you fiddle with it.
xen/arch/x86/acpi/cpufreq/intel_pstate.c int __init intel_pstate_init(void) · line 846
Quoted 7 lines
xen/arch/x86/acpi/cpufreq/intel_pstate.c +1
-0@@ -843,7 +843,11 @@ int __init intel_pstate_init(void)int cpu, rc =0 ;const struct x86_cpu_id *id;struct cpu_defaults *cpu_info; +static bool_t __read_mostly load;
Why __read_mostly when the function is __init?
Jan
Re: [PATCH v5 7/9] x86/intel_pstate: add a booting param to select the driver to load
Quoted 6 lines · Jan Beulich
Quoted 6 lines
Quoted 1 line
Quoted 1 line · On 14.09.15 at 04:32, <[email protected]> wrote:
xen/arch/x86/acpi/cpufreq/cpufreq.c +0
-0@@ -647,9 +648,11 @@ static int __init cpufreq_driver_init(void)int ret =0 ;
Quoted 15 lines
xen/arch/x86/acpi/cpufreq/cpufreq.c static int __init cpufreq_driver_init(void) · line 657
Quoted 11 lines
xen/arch/x86/acpi/cpufreq/cpufreq.c +5
-3if ((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_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 ();Since you're basically modifying the entire body of the function, please gets its coding style corrected as you fiddle with it.
The coding style here is aligned with the existing piece of code in this file. If it's also good to make the file have two coding styles, please let me know.
Best,
Wei
Re: [PATCH v5 7/9] x86/intel_pstate: add a booting param to select the driver to load
In reply to Jan BeulichQuoted 6 lines · Jan Beulich
Quoted 6 lines
Quoted 1 line
Quoted 1 line · On 14.09.15 at 04:32, <[email protected]> wrote:
xen/arch/x86/acpi/cpufreq/cpufreq.c +0
-0@@ -647,9 +648,11 @@ static int __init cpufreq_driver_init(void)int ret =0 ;
Quoted 14 lines
xen/arch/x86/acpi/cpufreq/cpufreq.c static int __init cpufreq_driver_init(void) · line 657
Quoted 11 lines
xen/arch/x86/acpi/cpufreq/cpufreq.c +5
-3if ((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_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 ();Since you're basically modifying the entire body of the function, please gets its coding style corrected as you fiddle with it.
OK, I guess you was probably referring to the this line - (boot_cpu_data.x86_vendor == X86_VENDOR_AMD)). Will align that as well.
Best,
Wei
Re: [PATCH v5 7/9] x86/intel_pstate: add a booting param to select the driver to load
In reply to Jan BeulichQuoted 6 lines · Jan Beulich
Quoted 6 lines
Quoted 1 line
Quoted 1 line · On 14.09.15 at 04:32, <[email protected]> wrote:
xen/arch/x86/acpi/cpufreq/cpufreq.c +0
-0@@ -647,9 +648,11 @@ static int __init cpufreq_driver_init(void)int ret =0 ;
Quoted 14 lines
xen/arch/x86/acpi/cpufreq/cpufreq.c static int __init cpufreq_driver_init(void) · line 657
Quoted 11 lines
xen/arch/x86/acpi/cpufreq/cpufreq.c +5
-3if ((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_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 ();Since you're basically modifying the entire body of the function, please gets its coding style corrected as you fiddle with it.
Ok, I guess you was probably referring to the remaining lines in the function - "(boot_cpu_data.x86_vendor == X86_VENDOR_AMD))".. Will align them as well.
Best,
Wei
Re: [PATCH v5 7/9] x86/intel_pstate: add a booting param to select the driver to load
Quoted 21 lines
Quoted 1 line
Quoted 1 line · On 23.10.15 at 10:18, <[email protected]> wrote:
Quoted 6 lines · Jan Beulich
Quoted 6 lines
Quoted 1 line
Quoted 1 line · On 14.09.15 at 04:32, <[email protected]> wrote:
xen/arch/x86/acpi/cpufreq/cpufreq.c +0
-0@@ -647,9 +648,11 @@ static int __init cpufreq_driver_init(void)int ret =0 ;Quoted 13 lines
xen/arch/x86/acpi/cpufreq/cpufreq.c static int __init cpufreq_driver_init(void) · line 657
Quoted 11 lines
xen/arch/x86/acpi/cpufreq/cpufreq.c +5
-3if ((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_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 ();Since you're basically modifying the entire body of the function, please
Quoted 6 lines
gets its
Quoted 1 line
coding style corrected as you fiddle with it.
Ok, I guess you was probably referring to the remaining lines in the function - "(boot_cpu_data.x86_vendor ==
X86_VENDOR_AMD))".. Will align them as well.
No, I'm not just talking about alignment. And the coding style of the file is mixed already (see e.g. the following function, which admittedly has even more blanks than needed), so getting this function into proper shape since you modify it in its entirety is a step in the right direction.
Jan
Re: [PATCH v5 7/9] x86/intel_pstate: add a booting param to select the driver to load
Quoted 25 lines · Jan Beulich
Quoted 25 lines
Quoted 1 line
Quoted 1 line · On 23.10.15 at 10:18, <[email protected]> wrote:
Quoted 6 lines · Jan Beulich
Quoted 6 lines
Quoted 1 line
Quoted 1 line · On 14.09.15 at 04:32, <[email protected]> wrote:
xen/arch/x86/acpi/cpufreq/cpufreq.c +0
-0@@ -647,9 +648,11 @@ static int __init cpufreq_driver_init(void)int ret =0 ;Quoted 14 lines
xen/arch/x86/acpi/cpufreq/cpufreq.c static int __init cpufreq_driver_init(void) · line 657
Quoted 11 lines
xen/arch/x86/acpi/cpufreq/cpufreq.c +5
-3if ((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_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 ();Since you're basically modifying the entire body of the function, please
gets its
Quoted 1 line
coding style corrected as you fiddle with it.
Quoted 9 lines
Quoted 3 lines
Ok, I guess you was probably referring to the remaining lines in the function - "(boot_cpu_data.x86_vendor ==
X86_VENDOR_AMD))".. Will align them as well.No, I'm not just talking about alignment. And the coding style of the file is mixed already (see e.g. the following function, which admittedly has even more blanks than needed), so getting this function into proper shape since you modify it in its entirety is a step in the right direction.
Ok. I will use Xen style brackets for this function.
Best,
Wei
PATCH v5 8/9 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.
--- tools/libxc/xc_pm.c | 15 ++-- xen/drivers/acpi/pmstat.c | 179 +++++++++++++++++++++++++++++++++++++------- xen/include/public/sysctl.h | 29 ++++--- 3 files changed, 179 insertions(+), 44 deletions(-)
changes in v5:
1) remove a intermediate variable, "scaling_avail_governors";
2) add condition checks in set_cpufreq_para();
3) replace the previous union "scaling_max" and "scaling_min" with "uint32_t scaling_max_perf" and "uint32_t scaling_min_perf";
4) add "enum perf_alias", to indicate the meaning of "scaling_max_perf" and "scaling_min_perf" - holding Percentage values or Frequency values;
5) re-organize the xen_get_cpufreq_para structure to make it less than 128Byte;
6) coding style changes.
tools/libxc/xc_pm.c +8 -7
@@ -260,13 +260,14 @@ 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.pct = sys_para->scaling_max_perf; + user_para->scaling_min.pct = sys_para->scaling_min_perf; + user_para->scaling_turbo_pct = sys_para->scaling_turbo_pct; + user_para->turbo_enabled = sys_para->turbo_enabled;memcpy (user_para->scaling_driver, sys_para->scaling_driver,CPUFREQ_NAME_LEN );
xen/drivers/acpi/pmstat.c +152 -27
272 lines not on this page · raw message
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 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 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 v5 8/9] x86/intel_pstate: support the use of intel_pstate in pmstat.c
Quoted 12 lines
Quoted 1 line
Quoted 1 line · On 14.09.15 at 04:32, <[email protected]> wrote:
tools/libxc/xc_pm.c +0
-5@@ -260,13 +260,14 @@ 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;
tools/libxc/xc_pm.c int xc_get_cpufreq_para(xc_interface *xch, int cpuid, · line 270
Quoted 10 lines
tools/libxc/xc_pm.c +8
-2- 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.pct = sys_para->scaling_max_perf; + user_para->scaling_min.pct = sys_para->scaling_min_perf; + user_para->scaling_turbo_pct = sys_para->scaling_turbo_pct; + user_para->turbo_enabled = sys_para->turbo_enabled;
You fail to communicate perf_alias here. How will the caller know? It's also odd that you initialize .pct fields from _perf ones - can't the naming and layout be arranged to match as much as possible?
Quoted 14 lines
xen/drivers/acpi/pmstat.c +3
-1@@ -191,7 +191,9 @@ static int get_cpufreq_para(struct xen_sysctl_pm_op *op)uint32_t ret =0 ;const struct processor_pminfo *pmpt;struct cpufreq_policy *policy; -uint32_t gov_num =0 ; +struct perf_limits *limits; +struct internal_governor *internal_gov; +uint32_t cur_gov, gov_num =0 ;uint32_t *affected_cpus;uint32_t *scaling_available_frequencies;char *scaling_available_governors;
xen/drivers/acpi/pmstat.c static int get_cpufreq_para(struct xen_sysctl_pm_op *op) · line 207
Quoted 6 lines
xen/drivers/acpi/pmstat.c +3
-0@@ -200,13 +202,21 @@ static int get_cpufreq_para(struct xen_sysctl_pm_op *op) pmpt = processor_pminfo[op->cpuid]; policy =per_cpu (cpufreq_cpu_policy, op->cpuid); + limits = &policy->limits; + internal_gov = policy->internal_gov; + cur_gov = internal_gov ? internal_gov->cur_gov :0 ;
Bad literal 0 (see the uses of cur_gov below).
xen/drivers/acpi/pmstat.c static int get_cpufreq_para(struct xen_sysctl_pm_op *op) · line 210
Quoted 3 lines
xen/drivers/acpi/pmstat.c +1
-1if ( !pmpt || !pmpt->perf.states || - !policy || !policy->governor ) + !policy || (!policy->governor && !policy->internal_gov) )
Either you think you need the local variable internal_gov - then use it everywhere, or drop it.
xen/drivers/acpi/pmstat.c static int get_cpufreq_para(struct xen_sysctl_pm_op *op) · line 214
Quoted 6 lines
xen/drivers/acpi/pmstat.c +2
-2return -EINVAL ; -list_for_each (pos, &cpufreq_governor_list) - gov_num++; +if ( internal_gov ) + gov_num = internal_gov->gov_num;
Since you don't need cur_gov ahead of this, please limit the number of conditionals by moving its initialization here.
Quoted 18 lines
xen/drivers/acpi/pmstat.c +11
-2op->u.get_para.cpuinfo_cur_freq = cpufreq_driver->get ? cpufreq_driver->get (op->cpuid) : policy->cur; op->u.get_para.cpuinfo_max_freq = policy->cpuinfo.max_freq; op->u.get_para.cpuinfo_min_freq = policy->cpuinfo.min_freq; op->u.get_para.scaling_cur_freq = policy->cur; - 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; + } +else + { + op->u.get_para.scaling_max_perf = policy->max; + op->u.get_para.scaling_min_perf = policy->min; + }
xen/drivers/acpi/pmstat.c static int get_cpufreq_para(struct xen_sysctl_pm_op *op) · line 302
Quoted 10 lines
xen/drivers/acpi/pmstat.c +6
-0if ( cpufreq_driver->name[0 ] ) + {strlcpy (op->u.get_para.scaling_driver, cpufreq_driver->name,CPUFREQ_NAME_LEN ); +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 ;
This should be done together with the other things in the if/else right above.
Quoted 20 lines
xen/drivers/acpi/pmstat.c +6
-3@@ -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);if ( !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 ) + { +if ( !strnicmp (op->u.set_gov.scaling_governor, +"performance" ,CPUFREQ_NAME_LEN ) )
xen/drivers/acpi/pmstat.c static int get_cpufreq_para(struct xen_sysctl_pm_op *op) · line 382
Quoted 10 lines
xen/drivers/acpi/pmstat.c +10
-0+ 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 ;
Wouldn't this better be done by the internal governor's code, so it can also for itself decide which of the kinds it may not want to support?
xen/drivers/acpi/pmstat.c static int set_cpufreq_para(struct xen_sysctl_pm_op *op) · line 426
Quoted 5 lines
xen/drivers/acpi/pmstat.c +1
-0@@ -340,6 +423,9 @@ static int set_cpufreq_para(struct xen_sysctl_pm_op *op) {struct cpufreq_policy new_policy; +if ( !policy->governor || internal_gov )
Hard tab.
Quoted 5 lines
xen/drivers/acpi/pmstat.c +1
-0@@ -347,10 +433,43 @@ static int set_cpufreq_para(struct xen_sysctl_pm_op *op)break ; } +case SCALING_MAX_PCT :
xen/drivers/acpi/pmstat.c static int set_cpufreq_para(struct xen_sysctl_pm_op *op) · line 446
Quoted 10 lines
xen/drivers/acpi/pmstat.c +10
-0+ { +struct cpufreq_policy new_policy; +struct perf_limits *limits = &new_policy.limits; + +if ( policy->governor || !internal_gov ) +return -EINVAL ; + + new_policy = *policy; + limits->max_perf_pct =clamp_t (uint32_t, op->u.set_para.ctrl_value, + limits->min_policy_pct, limits->max_policy_pct);
Afaict all three values are of the same type - why clamp_t() then instead of clamp()?
xen/include/public/sysctl.h typedef struct xen_ondemand xen_ondemand_t; · line 301
Quoted 8 lines
xen/include/public/sysctl.h +2
-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 perf_alias {
xen_ prefix missing.
Quoted 35 lines
xen/include/public/sysctl.h +15
-7+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 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;
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; };
Is all of this re-arrangement really needed? Also can't turbo_enabled and scaling_turbo_pct be combined into a single field?
Jan
Re: [PATCH v5 8/9] x86/intel_pstate: support the use of intel_pstate in pmstat.c
Quoted 24 lines · Jan Beulich
tools/libxc/xc_pm.c int xc_get_cpufreq_para(xc_interface *xch, int
Quoted 4 lines
Quoted 1 line
Quoted 1 line · On 14.09.15 at 04:32, <[email protected]> wrote:
tools/libxc/xc_pm.c +0
-0@@ -260,13 +260,14 @@ int xc_get_cpufreq_para(xc_interface *xch, intcpuid,
Quoted 8 lines
tools/libxc/xc_pm.c +0
-5}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;tools/libxc/xc_pm.c int xc_get_cpufreq_para(xc_interface *xch, int cpuid, · line 270
Quoted 10 lines
tools/libxc/xc_pm.c +8
-2- 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.pct = sys_para->scaling_max_perf; + user_para->scaling_min.pct = sys_para->scaling_min_perf; + user_para->scaling_turbo_pct = sys_para->scaling_turbo_pct; + user_para->turbo_enabled = sys_para->turbo_enabled;
Quoted 1 line
You fail to communicate
perf_aliashere. How will the caller know?
"user_para->perf_alias = sys_para->perf_alias" is added in the next patch "tools:...", because the the "perf_alias " field is added to xc_get_cpufreq_para there.
Quoted 29 lines
Quoted 18 lines
tools/libxc/xc_pm.c +0
-0op->u.get_para.cpuinfo_cur_freq = cpufreq_driver->get ? cpufreq_driver->get (op->cpuid) : policy->cur; op->u.get_para.cpuinfo_max_freq = policy->cpuinfo.max_freq;op->u.get_para.cpuinfo_min_freq = policy->cpuinfo.min_freq; op->u.get_para.scaling_cur_freq = policy->cur;tools/libxc/xc_pm.c +11
-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; + } +else + { + op->u.get_para.scaling_max_perf = policy->max; + op->u.get_para.scaling_min_perf = policy->min; + }tools/libxc/xc_pm.c
Quoted 10 lines
tools/libxc/xc_pm.c +6
-0if ( cpufreq_driver->name[0 ] ) + {strlcpy (op->u.get_para.scaling_driver, cpufreq_driver->name,CPUFREQ_NAME_LEN ); +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 ;
Quoted 1 line
This should be done together with the other things in the if/else right above.
The code here is in a different file from the if/else above. Here in pmstat.c , we set the perf_alias ("meaning of data"). In the above if/else(xc_pm.c), we pass the value from one end to another.
Quoted 25 lines
Quoted 25 lines
tools/libxc/xc_pm.c +0
-0@@ -299,16 +357,36 @@ static int get_cpufreq_para(struct
xen_sysctl_pm_op*op) static int set_cpufreq_gov(structxen_sysctl_pm_op *op) { struct cpufreq_policy new_policy, *old_policy;tools/libxc/xc_pm.c +11
-3+ struct internal_governor *internal_gov; old_policy = per_cpu(cpufreq_cpu_policy, op->cpuid); if ( !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 ) + { + 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,
Quoted 8 lines
tools/libxc/xc_pm.c
Quoted 5 lines
tools/libxc/xc_pm.c +5
-0+ "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 ;Wouldn't this better be done by the internal governor's code, so it can also for itself decide which of the kinds it may not want to support?
I think it should be pmstat.c-'s job here to set " internal_gov->cur_gov", which is later passed to the internal governor's implementation code, who then decides how to deal with the requested governor in "internal_gov->cur_gov".
Quoted 10 lines
xen/include/public/sysctl.h typedef struct xen_ondemand xen_ondemand_t; · line 301
Quoted 8 lines
xen/include/public/sysctl.h +2
-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 perf_alias {
xen_prefix missing.
This enum will also be used in xc_get_cpufreq_para, should we still add xen_ prefix here?
Quoted 37 lines
Quoted 37 lines
xen/include/public/sysctl.h +15
-8+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 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;
Quoted 11 lines
xen/include/public/sysctl.h struct xen_get_cpufreq_para { · line 336
Quoted 8 lines
xen/include/public/sysctl.h +0
-2/* for specific governor */ union {struct xen_userspace userspace;struct xen_ondemand ondemand; } u; - -int32_t turbo_enabled; };Is all of this re-arrangement really needed? Also can't
turbo_enabledandscaling_turbo_pctbe combined into a single field?
Personally, we should not combine the two.turbo_enabled is used by both the old pstate driver and intel_pstate, but scaling_turbo_pct is only used in intel_pstate. If we use "scaling_turbo_pct=0" to represent "turbo_enabled=0", and "scaling_turbo_pct>0" to represent " turbo_enabled=1", then we will need to modify the old driver to use scaling_turbo_pct, i.e. changing the old driver to be aware of the "percentage" concept, which is proposed in intel_pstate. On the other side, I think keeping turbo_enabled and scaling_turbo_pct separated makes the code easier to read. After re-arranging the layout, we got enough space to keep the two. I think the re-arranged layout doesn't look bad.
Best,
Wei
Re: [PATCH v5 8/9] x86/intel_pstate: support the use of intel_pstate in pmstat.c
Quoted 26 lines
Quoted 1 line
Quoted 1 line
"Wang, Wei W" <[email protected]> 10/26/15 7:27 AM >>>
Quoted 24 lines · Jan Beulich
tools/libxc/xc_pm.c int xc_get_cpufreq_para(xc_interface *xch, int
Quoted 4 lines
Quoted 1 line
Quoted 1 line · On 14.09.15 at 04:32, <[email protected]> wrote:
tools/libxc/xc_pm.c +0
-0@@ -260,13 +260,14 @@ int xc_get_cpufreq_para(xc_interface *xch, intcpuid,
Quoted 18 lines
tools/libxc/xc_pm.c +8
-7}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.pct = sys_para->scaling_max_perf; + user_para->scaling_min.pct = sys_para->scaling_min_perf; + user_para->scaling_turbo_pct = sys_para->scaling_turbo_pct; + user_para->turbo_enabled = sys_para->turbo_enabled;
Quoted 3 lines
Quoted 1 line
You fail to communicate
perf_aliashere. How will the caller know?"
user_para->perf_alias=sys_para->perf_alias" is added in the next patch "tools:...", because the the "perf_alias" field is added toxc_get_cpufreq_parathere.
But the public interface structure field is already there at this point. I.e. at this point in the series you're hiding information - another sign for improper breakup of the series.
Quoted 29 lines
Quoted 29 lines
Quoted 28 lines
tools/libxc/xc_pm.c +0
-0op->u.get_para.cpuinfo_cur_freq = cpufreq_driver->get ? cpufreq_driver->get (op->cpuid) : policy->cur; op->u.get_para.cpuinfo_max_freq = policy->cpuinfo.max_freq;op->u.get_para.cpuinfo_min_freq = policy->cpuinfo.min_freq; op->u.get_para.scaling_cur_freq = policy->cur;tools/libxc/xc_pm.c +17
-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; + } +else + { + op->u.get_para.scaling_max_perf = policy->max; + op->u.get_para.scaling_min_perf = policy->min; + }if ( cpufreq_driver->name[0 ] ) + {strlcpy (op->u.get_para.scaling_driver, cpufreq_driver->name,CPUFREQ_NAME_LEN ); +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 ;
Quoted 3 lines
Quoted 1 line
This should be done together with the other things in the if/else right above.
The code here is in a different file from the if/else above.
Huh? I don't think I trimmed the original mail this badly. From the context above, both are directly adjacent in the same source file.
Quoted 18 lines
Quoted 18 lines
Quoted 17 lines
tools/libxc/xc_pm.c +14
-3- 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 ) + { +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 ;
Quoted 6 lines
Quoted 2 lines
Wouldn't this better be done by the internal governor's code, so it can also for itself decide which of the kinds it may not want to support?
I think it should be pmstat.c-'s job here to set "
internal_gov->cur_gov", which is later passed to the internal governor's implementation code, who then decides how to deal with the requested governor in "internal_gov->cur_gov".
Which it then is able to communicate in which way? Without itself adjusting ->cur_gov again?
Quoted 11 lines
Quoted 10 lines
xen/include/public/sysctl.h typedef struct xen_ondemand xen_ondemand_t; · line 301
Quoted 8 lines
xen/include/public/sysctl.h +2
-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 perf_alias {
xen_prefix missing.
Quoted 1 line
This enum will also be used in
xc_get_cpufreq_para, should we still addxen_prefix here?
Of course: This is a public interface type, and hence should be prefixed suitably to limit possible name clashes with non-Xen code.
Quoted 45 lines
Quoted 33 lines
Quoted 33 lines
xen/include/public/sysctl.h +11
-8struct 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 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;Quoted 11 lines
xen/include/public/sysctl.h struct xen_get_cpufreq_para { · line 336
Quoted 8 lines
xen/include/public/sysctl.h +0
-2/* for specific governor */ union {struct xen_userspace userspace;struct xen_ondemand ondemand; } u; - -int32_t turbo_enabled; };Is all of this re-arrangement really needed? Also can't
turbo_enabledandscaling_turbo_pctbe combined into a single field?
Quoted 8 lines
Personally, we should not combine the two.
turbo_enabledis used by both the old pstate driver andintel_pstate, butscaling_turbo_pctis only used inintel_pstate. If we use "scaling_turbo_pct=0" to represent "turbo_enabled=0", and "scaling_turbo_pct>0" to represent " turbo_enabled=1", then we will need to modify the old driver to usescaling_turbo_pct, i.e. changing the old driver to be aware of the "percentage" concept, which is proposed inintel_pstate. On the other side, I think keepingturbo_enabledandscaling_turbo_pctseparated makes the code easier to read.
Note that "combine" doesn't necessarily mean "eliminate the old one" - they could as well become field of a union. The basic question you should ask yourself in such cases is: "Do both fields have a meaning at the same time?" If the answer is yes, then of course they should remain separate. If the answer is no _and_ their purpose is reasonably similar, then combining them should at least be considered.
Quoted 2 lines
After re-arranging the layout, we got enough space to keep the two. I think the re-arranged layout doesn't look bad.
And I didn't say it would - I asked whether it's necessary.
Jan
Re: [PATCH v5 8/9] x86/intel_pstate: support the use of intel_pstate in pmstat.c
Quoted 25 lines · Jan Beulich
Quoted 25 lines
Quoted 1 line
Quoted 1 line
"Wang, Wei W" <[email protected]> 10/26/15 7:27 AM >>>
Quoted 2 lines · Jan Beulich
Quoted 2 lines
Quoted 1 line
Quoted 1 line · On 14.09.15 at 04:32, <[email protected]> wrote:
- new_policy.governor = __find_governor(op-
u.set_gov.scaling_governor);
Quoted 19 lines
Quoted 6 lines
xen/drivers/acpi/pmstat.c +4
-2-if (new_policy.governor ==NULL ) -return -EINVAL ; +if ( internal_gov && internal_gov->cur_gov ) + { +if ( !strnicmp (op->u.set_gov.scaling_governor, +"performance" ,CPUFREQ_NAME_LEN ) )xen/drivers/acpi/pmstat.c static int get_cpufreq_para(struct xen_sysctl_pm_op *op) · line 382
Quoted 10 lines
xen/drivers/acpi/pmstat.c +10
-0+ 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 ;Wouldn't this better be done by the internal governor's code, so it can also for itself decide which of the kinds it may not want to support?
Quoted 7 lines
Quoted 3 lines
I think it should be pmstat.c-'s job here to set "
internal_gov->cur_gov", which is later passed to the internal governor's implementation code, who then decides how to deal with therequested governor in "
internal_gov->cur_gov".Which it then is able to communicate in which way? Without itself adjusting ->cur_gov again?
In this way:pmstat.c gets the string-represented governor adjusting request from the upper layer, parses it to the number-represented value (INTERNAL_GOV_), and feeds the number-represented one to the driver's internal governor implementation. If the internal governor implementation doesn't support the passed INTERNAL_GOV_XX, i.e. INTERNAL_GOV_XX goes to the "default:" section of the "switch()", which adjusts the internal_gov->cur_gov to the default one, e.g. INTERNAL_GOV_ONDEMAND.
Quoted 54 lines
Quoted 45 lines
Quoted 33 lines
Quoted 33 lines
struct xen_get_cpufreq_para { /* IN/OUT variable */ uint32_t cpu_num; uint32_t freq_num; uint32_t gov_num;xen/include/public/sysctl.h +11
-8+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 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;Quoted 11 lines
xen/include/public/sysctl.h struct xen_get_cpufreq_para { · line 336
Quoted 8 lines
xen/include/public/sysctl.h +0
-2/* for specific governor */ union {struct xen_userspace userspace;struct xen_ondemand ondemand; } u; - -int32_t turbo_enabled; };Is all of this re-arrangement really needed? Also can't
turbo_enabledandscaling_turbo_pctbe combined into a single field?Quoted 3 lines
Personally, we should not combine the two.
turbo_enabledis used by both the old pstate driver andintel_pstate, butscaling_turbo_pctis only used inintel_pstate. If we use"scaling_turbo_pct=0"
Quoted 5 lines
to represent "turbo_enabled=0", and "scaling_turbo_pct>0" to represent " turbo_enabled=1", then we will need to modify the old driver to use
scaling_turbo_pct, i.e. changing the old driver to be aware of the "percentage" concept, which is proposed inintel_pstate. On the other side, I think keepingturbo_enabledandscaling_turbo_pctseparated makes the code
Quoted 8 lines
easier to read.
Note that "combine" doesn't necessarily mean "eliminate the old one" - they could as well become field of a union. The basic question you should ask yourself in such cases is: "Do both fields have a meaning at the same time?" If the answer is yes, then of course they should remain separate. If the answer is no _and_ their purpose is reasonably similar, then combining them should at least be considered.
Ok. I will keep the two separated, since they do have their own meaning at the same time.
Best,
Wei
Re: [PATCH v5 8/9] x86/intel_pstate: support the use of intel_pstate in pmstat.c
Quoted 35 lines
Quoted 1 line
Quoted 1 line · On 26.10.15 at 08:59, <[email protected]> wrote:
Quoted 25 lines · Jan Beulich
Quoted 25 lines
Quoted 1 line
Quoted 1 line
"Wang, Wei W" <[email protected]> 10/26/15 7:27 AM >>>
Quoted 2 lines · Jan Beulich
Quoted 2 lines
Quoted 1 line
Quoted 1 line · On 14.09.15 at 04:32, <[email protected]> wrote:
- new_policy.governor = __find_governor(op-
u.set_gov.scaling_governor);
Quoted 19 lines
Quoted 6 lines
xen/drivers/acpi/pmstat.c +4
-2-if (new_policy.governor ==NULL ) -return -EINVAL ; +if ( internal_gov && internal_gov->cur_gov ) + { +if ( !strnicmp (op->u.set_gov.scaling_governor, +"performance" ,CPUFREQ_NAME_LEN ) )xen/drivers/acpi/pmstat.c static int get_cpufreq_para(struct xen_sysctl_pm_op *op) · line 382
Quoted 10 lines
xen/drivers/acpi/pmstat.c +10
-0+ 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 ;Wouldn't this better be done by the internal governor's code, so it can also for itself decide which of the kinds it may not want to support?
Quoted 7 lines
Quoted 3 lines
I think it should be pmstat.c-'s job here to set "
internal_gov->cur_gov", which is later passed to the internal governor's implementation code, who then decides how to deal with therequested governor in "
internal_gov->cur_gov".Which it then is able to communicate in which way? Without itself adjusting ->cur_gov again?
Quoted 8 lines
In this way:
pmstat.cgets the string-represented governor adjusting request from the upper layer, parses it to the number-represented value (INTERNAL_GOV_), and feeds the number-represented one to the driver's internal governor implementation. If the internal governor implementation doesn't support the passedINTERNAL_GOV_XX, i.e.INTERNAL_GOV_XXgoes to the "default:" section of the "switch()", which adjusts theinternal_gov->cur_govto the default one, e.g.INTERNAL_GOV_ONDEMAND.
Well, okay, I'm tired of discussing issues like this.
Quoted 44 lines
Quoted 35 lines
Quoted 26 lines
Quoted 14 lines
Quoted 14 lines
xen/include/public/sysctl.h +0
-8@@ -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;Quoted 11 lines
xen/include/public/sysctl.h struct xen_get_cpufreq_para { · line 336
Quoted 8 lines
xen/include/public/sysctl.h +0
-2/* for specific governor */ union {struct xen_userspace userspace;struct xen_ondemand ondemand; } u; - -int32_t turbo_enabled; };Is all of this re-arrangement really needed? Also can't
turbo_enabledandscaling_turbo_pctbe combined into a single field?Quoted 3 lines
Personally, we should not combine the two.
turbo_enabledis used by both the old pstate driver andintel_pstate, butscaling_turbo_pctis only used inintel_pstate. If we use"scaling_turbo_pct=0"
Quoted 5 lines
to represent "turbo_enabled=0", and "scaling_turbo_pct>0" to represent " turbo_enabled=1", then we will need to modify the old driver to use
scaling_turbo_pct, i.e. changing the old driver to be aware of the "percentage" concept, which is proposed inintel_pstate. On the other side, I think keepingturbo_enabledandscaling_turbo_pctseparated makes the codeQuoted 8 lines
easier to read.
Note that "combine" doesn't necessarily mean "eliminate the old one" - they could as well become field of a union. The basic question you should ask yourself in such cases is: "Do both fields have a meaning at the same time?" If the answer is yes, then of course they should remain separate. If the answer is no _and_ their purpose is reasonably similar, then combining them should at least be considered.
Quoted 2 lines
Ok. I will keep the two separated, since they do have their own meaning at the same time.
Being which?
Jan
Re: [PATCH v5 8/9] x86/intel_pstate: support the use of intel_pstate in pmstat.c
Quoted 52 lines · Jan Beulich
Quoted 31 lines
Quoted 1 line
Quoted 1 line · On 26.10.15 at 08:59, <[email protected]> wrote:
Quoted 29 lines · Jan Beulich
Quoted 29 lines
Quoted 1 line
Quoted 1 line
"Wang, Wei W" <[email protected]> 10/26/15 7:27 AM >>>
Quoted 26 lines · Jan Beulich
Quoted 13 lines
Quoted 1 line
Quoted 1 line · On 14.09.15 at 04:32, <[email protected]> wrote:
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; };Is all of this re-arrangement really needed? Also can't
turbo_enabledandscaling_turbo_pctbe combined into a single field?Quoted 8 lines
Quoted 8 lines
Quoted 4 lines
Personally, we should not combine the two.
turbo_enabledis used by both the old pstate driver andintel_pstate, butscaling_turbo_pctis only used inintel_pstate. If we use"scaling_turbo_pct=0"
Quoted 3 lines
to represent "turbo_enabled=0", and "scaling_turbo_pct>0" to represent " turbo_enabled=1", then we will need to modify the old driver to use
scaling_turbo_pct, i.e. changing the old driver to be aware ofthe "percentage"
Quoted 12 lines
Quoted 11 lines
Quoted 3 lines
concept, which is proposed in
intel_pstate. On the other side, I think keepingturbo_enabledandscaling_turbo_pctseparated makes the codeeasier to read.
Note that "combine" doesn't necessarily mean "eliminate the old one"
- they could as well become field of a union. The basic question you should ask yourself in such cases is: "Do both fields have a meaning at the same time?" If the answer is yes, then of course they should remain separate. If the answer is no _and_ their purpose is reasonably similar, then combining them should at least be considered.
Quoted 4 lines
Quoted 2 lines
Ok. I will keep the two separated, since they do have their own meaning at the same time.
Being which?
Keeping both of the two there. Just as what they are now - two independent fields.
Best,
Wei
Re: [PATCH v5 8/9] x86/intel_pstate: support the use of intel_pstate in pmstat.c
Quoted 55 lines
Quoted 1 line
Quoted 1 line · On 26.10.15 at 10:48, <[email protected]> wrote:
Quoted 31 lines · Jan Beulich
Quoted 31 lines
Quoted 1 line
Quoted 1 line · On 26.10.15 at 08:59, <[email protected]> wrote:
Quoted 29 lines · Jan Beulich
Quoted 29 lines
Quoted 1 line
Quoted 1 line
"Wang, Wei W" <[email protected]> 10/26/15 7:27 AM >>>
Quoted 26 lines · Jan Beulich
Quoted 13 lines
Quoted 1 line
Quoted 1 line · On 14.09.15 at 04:32, <[email protected]> wrote:
(quoted diff) +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;Review struct xen_get_cpufreq_para { · line 336
Quoted 10 lines
(quoted diff) +0
-3- uint32_t scaling_min_freq; /* for specific governor */ union { struct xen_userspace userspace; struct xen_ondemand ondemand; } u; - - int32_t turbo_enabled; };Is all of this re-arrangement really needed? Also can't
turbo_enabledandscaling_turbo_pctbe combined into a single field?Quoted 8 lines
Quoted 8 lines
Quoted 8 lines
Quoted 4 lines
Personally, we should not combine the two.
turbo_enabledis used by both the old pstate driver andintel_pstate, butscaling_turbo_pctis only used inintel_pstate. If we use"scaling_turbo_pct=0"
Quoted 3 lines
to represent "turbo_enabled=0", and "scaling_turbo_pct>0" to represent " turbo_enabled=1", then we will need to modify the old driver to use
scaling_turbo_pct, i.e. changing the old driver to be awareof
Quoted 13 lines
the "percentage"
Quoted 12 lines
Quoted 11 lines
Quoted 3 lines
concept, which is proposed in
intel_pstate. On the other side, I think keepingturbo_enabledandscaling_turbo_pctseparated makes the codeeasier to read.
Note that "combine" doesn't necessarily mean "eliminate the old one"
- they could as well become field of a union. The basic question you should ask yourself in such cases is: "Do both fields have a meaning at the same time?" If the answer is yes, then of course they should remain separate. If the answer is no _and_ their purpose is reasonably similar, then combining them should at least be considered.
Quoted 7 lines
Quoted 4 lines
Quoted 2 lines
Ok. I will keep the two separated, since they do have their own meaning at the same time.
Being which?
Keeping both of the two there. Just as what they are now - two independent fields.
That wasn't the question; I rather inquired what "meaning at the same time" both fields have.
Jan
Re: [PATCH v5 8/9] x86/intel_pstate: support the use of intel_pstate in pmstat.c
Quoted 60 lines · Jan Beulich
Quoted 37 lines
Quoted 1 line
Quoted 1 line · On 26.10.15 at 10:48, <[email protected]> wrote:
Quoted 35 lines · Jan Beulich
Quoted 35 lines
Quoted 1 line
Quoted 1 line · On 26.10.15 at 08:59, <[email protected]> wrote:
Quoted 33 lines · Jan Beulich
Quoted 33 lines
Quoted 1 line
Quoted 1 line
"Wang, Wei W" <[email protected]> 10/26/15 7:27 AM >>>
Quoted 15 lines · Jan Beulich
Quoted 15 lines
Quoted 1 line
Quoted 1 line · On 14.09.15 at 04:32, <[email protected]> wrote:
(quoted diff) +0
-8@@ -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;Quoted 11 lines
Review struct xen_get_cpufreq_para { · line 336
Quoted 8 lines
(quoted diff) +0
-2/* for specific governor */ union { struct xen_userspace userspace; struct xen_ondemand ondemand; } u; - - int32_t turbo_enabled; };Is all of this re-arrangement really needed? Also can't
turbo_enabledandscaling_turbo_pctbe combined into a single field?Personally, we should not combine the two.
turbo_enabledis used by both the old pstate driver andintel_pstate, butscaling_turbo_pctis only used inintel_pstate. If we useQuoted 18 lines
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
"scaling_turbo_pct=0"
Quoted 4 lines
to represent "turbo_enabled=0", and "scaling_turbo_pct>0" to represent " turbo_enabled=1", then we will need to modify the old driver to use
scaling_turbo_pct, i.e. changing the old driver to be awareof
Quoted 12 lines
the "percentage"
Quoted 11 lines
Quoted 11 lines
Quoted 3 lines
concept, which is proposed in
intel_pstate. On the other side, I think keepingturbo_enabledandscaling_turbo_pctseparated makes the codeeasier to read.
Note that "combine" doesn't necessarily mean "eliminate the old one"
- they could as well become field of a union. The basic question you should ask yourself in such cases is: "Do both fields have a meaning at the same time?" If the answer is yes, then of course they should remain separate. If the answer is no _and_ their purpose is reasonably similar, then combining them should at least beconsidered.
Quoted 4 lines
Quoted 4 lines
Quoted 3 lines
Ok. I will keep the two separated, since they do have their own meaning at the same time.
Quoted 7 lines
Quoted 4 lines
Quoted 1 line
Being which?
Keeping both of the two there. Just as what they are now - two independent fields.
That wasn't the question; I rather inquired what "meaning at the same time" both fields have.
turbo_enable: indicates if turbo is enabled or not.turbo_pct: shows the capability of turbo in percentage. For example if the CPU has the following operating frequency range:
Quoted 1 line
From [Min] to [Max]: 1.2GH, 1.3GHZ,1.4GHZ,...,2.3GHZ, so 1.2--->2.3, length = 12
Turbo frequency: 3.7GHZ, so 1.2---->3.7, length = 26 Then turbo_pct = (26 - 12) / 26 = 54%, that is turbo_pct = 54.
Best,
Wei
Re: [PATCH v5 8/9] x86/intel_pstate: support the use of intel_pstate in pmstat.c
Quoted 11 lines
Quoted 1 line
Quoted 1 line · On 26.10.15 at 11:19, <[email protected]> wrote:
Quoted 2 lines · Jan Beulich
That wasn't the question; I rather inquired what "meaning at the same time" both fields have.
turbo_enable: indicates if turbo is enabled or not.turbo_pct: shows the capability of turbo in percentage. For example if the CPU has the following operating frequency range:
From [Min] to [Max]: 1.2GH, 1.3GHZ,1.4GHZ,...,2.3GHZ, so 1.2--->2.3, length = 12 Turbo frequency: 3.7GHZ, so 1.2---->3.7, length = 26
Thenturbo_pct= (26 - 12) / 26 = 54%, that isturbo_pct= 54.
So what meaning does e.g. "turbo_enabled = 1" plus "turbo_pct = 0" have? Or "turbo_enabled = 0" plus "turbo_pct > 0"?
Jan
Re: [PATCH v5 8/9] x86/intel_pstate: support the use of intel_pstate in pmstat.c
Quoted 12 lines · Jan Beulich
Quoted 11 lines
Quoted 1 line
Quoted 1 line · On 26.10.15 at 11:19, <[email protected]> wrote:
Quoted 2 lines · Jan Beulich
That wasn't the question; I rather inquired what "meaning at the same time" both fields have.
turbo_enable: indicates if turbo is enabled or not.turbo_pct: shows the capability of turbo in percentage. For example if the CPU has the following operating frequency range:
From [Min] to [Max]: 1.2GH, 1.3GHZ,1.4GHZ,...,2.3GHZ, so 1.2--->2.3, length = 12 Turbo frequency: 3.7GHZ, so 1.2---->3.7, length = 26 Thenturbo_pct= (26 - 12) / 26 = 54%, that isturbo_pct= 54.
Quoted 2 lines
So what meaning does e.g. "
turbo_enabled= 1" plus "turbo_pct= 0" have? Or "turbo_enabled= 0" plus "turbo_pct> 0"?
" turbo_pct = 54" is the property of the CPU, just like MAX=3.7GHZ. It should be something engraved on stone, not changeable. So, I think it's not a good idea to put them together.
Best,
Wei
Re: [PATCH v5 8/9] x86/intel_pstate: support the use of intel_pstate in pmstat.c
Quoted 14 lines
Quoted 1 line
Quoted 1 line · On 26.10.15 at 11:45, <[email protected]> wrote:
Quoted 12 lines · Jan Beulich
Quoted 11 lines
Quoted 1 line
Quoted 1 line · On 26.10.15 at 11:19, <[email protected]> wrote:
Quoted 2 lines · Jan Beulich
That wasn't the question; I rather inquired what "meaning at the same time" both fields have.
turbo_enable: indicates if turbo is enabled or not.turbo_pct: shows the capability of turbo in percentage. For example if the CPU has the following operating frequency range:
From [Min] to [Max]: 1.2GH, 1.3GHZ,1.4GHZ,...,2.3GHZ, so 1.2--->2.3, length = 12 Turbo frequency: 3.7GHZ, so 1.2---->3.7, length = 26 Thenturbo_pct= (26 - 12) / 26 = 54%, that isturbo_pct= 54.
Quoted 6 lines
Quoted 2 lines
So what meaning does e.g. "
turbo_enabled= 1" plus "turbo_pct= 0" have? Or "turbo_enabled= 0" plus "turbo_pct> 0"?"
turbo_pct= 54" is the property of the CPU, just like MAX=3.7GHZ. It should be something engraved on stone, not changeable. So, I think it's not a good idea to put them together.
That's a valid statement, but not really an answer to the question(s) raised.
Jan
Re: [PATCH v5 8/9] x86/intel_pstate: support the use of intel_pstate in pmstat.c
Quoted 15 lines · Jan Beulich
Quoted 5 lines
Quoted 1 line
Quoted 1 line · On 26.10.15 at 11:45, <[email protected]> wrote:
Quoted 3 lines · Jan Beulich
Quoted 3 lines
Quoted 1 line
Quoted 1 line · On 26.10.15 at 11:19, <[email protected]> wrote:
Quoted 1 line · Jan Beulich
That wasn't the question; I rather inquired what "meaning at the same
time"
Quoted 9 lines
Quoted 9 lines
Quoted 8 lines
Quoted 1 line
both fields have.
turbo_enable: indicates if turbo is enabled or not.turbo_pct: shows the capability of turbo in percentage. For example if the CPU has the following operating frequency range:
From [Min] to [Max]: 1.2GH, 1.3GHZ,1.4GHZ,...,2.3GHZ, so 1.2--->2.3, length = 12 Turbo frequency: 3.7GHZ, so 1.2---->3.7, length = 26 Thenturbo_pct= (26 - 12) / 26 = 54%, that isturbo_pct= 54.
Quoted 8 lines
Quoted 6 lines
Quoted 2 lines
So what meaning does e.g. "
turbo_enabled= 1" plus "turbo_pct= 0" have? Or "turbo_enabled= 0" plus "turbo_pct> 0"?"
turbo_pct= 54" is the property of the CPU, just like MAX=3.7GHZ. It should be something engraved on stone, not changeable. So, I think it's not a good idea to put them together.That's a valid statement, but not really an answer to the question(s) raised.
My point is that we should not discuss the two together, since they are not related to each other.
The answer is obvious:
"turbo_enabled = 1" plus "turbo_pct = 0", is not a possible case, since turbo_pct always equals to 54, as the valid statement states. However, if the CPU doesn't support turbo, then turbo_enabled won't be 1. "turbo_enabled = 0" plus "turbo_pct =54", simply means that the CPU has its turbo function switched off, though it has a turbo capability of 54%.
Best,
Wei
PATCH v5 9/9 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.
--- tools/libxc/include/xenctrl.h | 21 ++++---- tools/libxc/xc_pm.c | 5 +- tools/misc/xenpm.c | 116 ++++++++++++++++++++++++++++++++++-------- 3 files changed, 108 insertions(+), 34 deletions(-)
changes in v5:
1) re-organize "struct xc_get_cpufreq_para" to make it less than 128Byte;
2) change to use switch() and enum based perf_alias, instead of string comparisons.
tools/libxc/include/xenctrl.h +11 -10
@@ -2249,6 +2249,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 perf_alias perf_alias;/* for all governors */ /* OUT variable */
@@ -2256,23 +2267,13 @@ struct xc_get_cpufreq_para {uint32_t *scaling_available_frequencies;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 +3 -2
@@ -264,8 +264,9 @@ int xc_get_cpufreq_para(xc_interface *xch, int cpuid, 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.pct = sys_para->scaling_max_perf; - user_para->scaling_min.pct = sys_para->scaling_min_perf; + user_para->scaling_max_perf = sys_para->scaling_max_perf; + user_para->scaling_min_perf = sys_para->scaling_min_perf; + user_para->perf_alias = sys_para->perf_alias; user_para->scaling_turbo_pct = sys_para->scaling_turbo_pct; user_para->turbo_enabled = sys_para->turbo_enabled;
tools/misc/xenpm.c +94 -22
165 lines not on this page · raw message
Signature · 1 line
1.9.1