Re: [Xen-devel] [PATCH v4 11/11] tools: enable xenpm to control the intel_pstate driver
On 08/07/2015 14:24, Jan Beulich wrote:
On 08.07.15 at 07:15, wei.w.w...@intel.com wrote:
On 07/07/2015 20:14, Wei Liu wrote:
On Tue, Jul 07, 2015 at 01:05:21PM +0100, Jan Beulich wrote:
On 07.07.15 at 10:55, wei.l...@citrix.com wrote:
On Thu, Jun 25, 2015 at 07:19:05PM +0800, Wei Wang wrote:
--- a/tools/libxc/include/xenctrl.h
+++ b/tools/libxc/include/xenctrl.h
@@ -2266,8 +2266,18 @@ struct xc_get_cpufreq_para {
uint32_t scaling_cur_freq;
char scaling_governor[CPUFREQ_NAME_LEN];
-uint32_t scaling_max_freq;
-uint32_t scaling_min_freq;
+
+union {
+uint32_t freq;
+uint32_t pct;
+} scaling_max;
+
+union {
+uint32_t freq;
+uint32_t pct;
+} scaling_min;
+
Don't you need struct? I don't see how union would work for you,
you clearly need bot freq and pct at the same time.
Why? The current driver uses freq; intel_pstate uses pct. What
looks wrong is the code below using both fields at once.
I only looked at this single patch.
I got that impression from here:
+user_para-scaling_max.freq =
sys_para-scaling_max.freq;
+user_para-scaling_min.freq =
sys_para-scaling_min.freq;
+user_para-scaling_max.pct =
sys_para-scaling_max.pct;
+user_para-scaling_min.pct =
sys_para-scaling_min.pct;
So using union is OK, just the code is confusing.
This actually functions well, we just have a redundant copy here. For
example:
user_para-scaling_max.freq = sys_para-scaling_max.freq;
user_para-scaling_max.pct = sys_para-scaling_max.pct;
The two sentences are doing the same thing (the memory location is the
same for freq and pct, it just depends on the driver to put what kind
of value (freq or pct) into the memory).
If this causes some confusion, how about removing the
user_para-scaling_max.pct = sys_para-scaling_max.pct; ?
Better make it
user_para-scaling_max = sys_para-scaling_max;
if at all possible. If not possible, something BUILD_BUG_ON()-like should imo
be added to make sure the fields are of equal size.
Ok. I will wait for your comments on the other remaining patches to send out
the next revised version. Thanks.
Best,
Wei
___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v4 11/11] tools: enable xenpm to control the intel_pstate driver
On Thu, Jun 25, 2015 at 07:19:05PM +0800, Wei Wang wrote:
The 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.
Also, the get-cpufreq-para is modified to show percentage
based feedback info.
This patch does more than adding the interface. See below.
v4 changes:
None.
Signed-off-by: Wei Wang wei.w.w...@intel.com
---
tools/libxc/include/xenctrl.h | 14 -
tools/libxc/xc_pm.c | 17 ---
tools/misc/xenpm.c| 116
+-
3 files changed, 115 insertions(+), 32 deletions(-)
diff --git a/tools/libxc/include/xenctrl.h b/tools/libxc/include/xenctrl.h
index 100b89c..a79494a 100644
--- a/tools/libxc/include/xenctrl.h
+++ b/tools/libxc/include/xenctrl.h
@@ -2266,8 +2266,18 @@ struct xc_get_cpufreq_para {
uint32_t scaling_cur_freq;
char scaling_governor[CPUFREQ_NAME_LEN];
-uint32_t scaling_max_freq;
-uint32_t scaling_min_freq;
+
+union {
+uint32_t freq;
+uint32_t pct;
+} scaling_max;
+
+union {
+uint32_t freq;
+uint32_t pct;
+} scaling_min;
+
Don't you need struct? I don't see how union would work for you, you
clearly need bot freq and pct at the same time.
+uint32_t scaling_turbo_pct;
/* for specific governor */
union {
diff --git a/tools/libxc/xc_pm.c b/tools/libxc/xc_pm.c
index 823bab6..300de33 100644
--- a/tools/libxc/xc_pm.c
+++ b/tools/libxc/xc_pm.c
@@ -261,13 +261,16 @@ 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.freq = sys_para-scaling_max.freq;
+user_para-scaling_min.freq = sys_para-scaling_min.freq;
+user_para-scaling_max.pct = sys_para-scaling_max.pct;
+user_para-scaling_min.pct = sys_para-scaling_min.pct;
+user_para-scaling_turbo_pct=
sys_para-scaling_turbo_pct;
+user_para-turbo_enabled= sys_para-turbo_enabled;
The new indentation looks wrong. You don't need so many spaces.
memcpy(user_para-scaling_driver,
sys_para-scaling_driver, CPUFREQ_NAME_LEN);
diff --git a/tools/misc/xenpm.c b/tools/misc/xenpm.c
index 2f9bd8e..ea6a32f 100644
--- a/tools/misc/xenpm.c
+++ b/tools/misc/xenpm.c
@@ -33,6 +33,11 @@
#define MAX_CORE_RESIDENCIES 8
#define ARRAY_SIZE(a) (sizeof (a) / sizeof ((a)[0]))
+#define min_t(type,x,y) \
+({ type __x = (x); type __y = (y); __x __y ? __x: __y; })
+#define max_t(type,x,y) \
+({ type __x = (x); type __y = (y); __x __y ? __x: __y; })
+#define clamp_t(type, val, lo, hi) min_t(type, max_t(type, val, lo), hi)
static xc_interface *xc_handle;
static unsigned int max_cpu_nr;
@@ -47,6 +52,9 @@ void show_help(void)
get-cpuidle-states[cpuid] list cpu idle info of CPU
cpuid or all\n
get-cpufreq-states[cpuid] list cpu freq info of CPU
cpuid or all\n
get-cpufreq-para [cpuid] list cpu freq parameter of
CPU cpuid or all\n
+ set-scaling-max-pct [cpuid] num set max performance limit
in percentage\n
+ or as scaling speed in
percentage in userspace governor\n
+ set-scaling-min-pct [cpuid] num set min performance limit
in percentage\n
set-scaling-maxfreq [cpuid] HZ set max cpu frequency HZ
on CPU cpuid\n
or all CPUs\n
set-scaling-minfreq [cpuid] HZ set min cpu frequency HZ
on CPU cpuid\n
@@ -60,10 +68,10 @@ void show_help(void)
set-up-threshold [cpuid] num set up threshold on CPU
cpuid or all\n
it is used in ondemand
governor.\n
get-cpu-topologyget thread/core/socket
topology info\n
- set-sched-smt enable|disable enable/disable
scheduler smt power saving\n
+
Re: [Xen-devel] [PATCH v4 11/11] tools: enable xenpm to control the intel_pstate driver
On 07.07.15 at 10:55, wei.l...@citrix.com wrote:
On Thu, Jun 25, 2015 at 07:19:05PM +0800, Wei Wang wrote:
--- a/tools/libxc/include/xenctrl.h
+++ b/tools/libxc/include/xenctrl.h
@@ -2266,8 +2266,18 @@ struct xc_get_cpufreq_para {
uint32_t scaling_cur_freq;
char scaling_governor[CPUFREQ_NAME_LEN];
-uint32_t scaling_max_freq;
-uint32_t scaling_min_freq;
+
+union {
+uint32_t freq;
+uint32_t pct;
+} scaling_max;
+
+union {
+uint32_t freq;
+uint32_t pct;
+} scaling_min;
+
Don't you need struct? I don't see how union would work for you, you
clearly need bot freq and pct at the same time.
Why? The current driver uses freq; intel_pstate uses pct. What looks
wrong is the code below using both fields at once.
Jan
___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v4 11/11] tools: enable xenpm to control the intel_pstate driver
On Tue, Jul 07, 2015 at 01:05:21PM +0100, Jan Beulich wrote:
On 07.07.15 at 10:55, wei.l...@citrix.com wrote:
On Thu, Jun 25, 2015 at 07:19:05PM +0800, Wei Wang wrote:
--- a/tools/libxc/include/xenctrl.h
+++ b/tools/libxc/include/xenctrl.h
@@ -2266,8 +2266,18 @@ struct xc_get_cpufreq_para {
uint32_t scaling_cur_freq;
char scaling_governor[CPUFREQ_NAME_LEN];
-uint32_t scaling_max_freq;
-uint32_t scaling_min_freq;
+
+union {
+uint32_t freq;
+uint32_t pct;
+} scaling_max;
+
+union {
+uint32_t freq;
+uint32_t pct;
+} scaling_min;
+
Don't you need struct? I don't see how union would work for you, you
clearly need bot freq and pct at the same time.
Why? The current driver uses freq; intel_pstate uses pct. What looks
wrong is the code below using both fields at once.
I only looked at this single patch.
I got that impression from here:
+user_para-scaling_max.freq = sys_para-scaling_max.freq;
+user_para-scaling_min.freq = sys_para-scaling_min.freq;
+user_para-scaling_max.pct = sys_para-scaling_max.pct;
+user_para-scaling_min.pct = sys_para-scaling_min.pct;
So using union is OK, just the code is confusing.
Wei.
Jan
___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v4 11/11] tools: enable xenpm to control the intel_pstate driver
On 07/07/2015 20:14, Wei Liu wrote:
On Tue, Jul 07, 2015 at 01:05:21PM +0100, Jan Beulich wrote:
On 07.07.15 at 10:55, wei.l...@citrix.com wrote:
On Thu, Jun 25, 2015 at 07:19:05PM +0800, Wei Wang wrote:
--- a/tools/libxc/include/xenctrl.h
+++ b/tools/libxc/include/xenctrl.h
@@ -2266,8 +2266,18 @@ struct xc_get_cpufreq_para {
uint32_t scaling_cur_freq;
char scaling_governor[CPUFREQ_NAME_LEN];
-uint32_t scaling_max_freq;
-uint32_t scaling_min_freq;
+
+union {
+uint32_t freq;
+uint32_t pct;
+} scaling_max;
+
+union {
+uint32_t freq;
+uint32_t pct;
+} scaling_min;
+
Don't you need struct? I don't see how union would work for you, you
clearly need bot freq and pct at the same time.
Why? The current driver uses freq; intel_pstate uses pct. What looks
wrong is the code below using both fields at once.
I only looked at this single patch.
I got that impression from here:
+user_para-scaling_max.freq = sys_para-scaling_max.freq;
+user_para-scaling_min.freq = sys_para-scaling_min.freq;
+user_para-scaling_max.pct = sys_para-scaling_max.pct;
+user_para-scaling_min.pct = sys_para-scaling_min.pct;
So using union is OK, just the code is confusing.
This actually functions well, we just have a redundant copy here. For example:
user_para-scaling_max.freq = sys_para-scaling_max.freq;
user_para-scaling_max.pct = sys_para-scaling_max.pct;
The two sentences are doing the same thing (the memory location is the same for
freq and pct, it just depends on the driver to put what kind of value (freq or
pct) into the memory).
If this causes some confusion, how about removing the
user_para-scaling_max.pct = sys_para-scaling_max.pct; ?
Best,
Wei
___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
[Xen-devel] [PATCH v4 11/11] tools: enable xenpm to control the intel_pstate driver
The 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.
Also, the get-cpufreq-para is modified to show percentage
based feedback info.
v4 changes:
None.
Signed-off-by: Wei Wang wei.w.w...@intel.com
---
tools/libxc/include/xenctrl.h | 14 -
tools/libxc/xc_pm.c | 17 ---
tools/misc/xenpm.c| 116 +-
3 files changed, 115 insertions(+), 32 deletions(-)
diff --git a/tools/libxc/include/xenctrl.h b/tools/libxc/include/xenctrl.h
index 100b89c..a79494a 100644
--- a/tools/libxc/include/xenctrl.h
+++ b/tools/libxc/include/xenctrl.h
@@ -2266,8 +2266,18 @@ struct xc_get_cpufreq_para {
uint32_t scaling_cur_freq;
char scaling_governor[CPUFREQ_NAME_LEN];
-uint32_t scaling_max_freq;
-uint32_t scaling_min_freq;
+
+union {
+uint32_t freq;
+uint32_t pct;
+} scaling_max;
+
+union {
+uint32_t freq;
+uint32_t pct;
+} scaling_min;
+
+uint32_t scaling_turbo_pct;
/* for specific governor */
union {
diff --git a/tools/libxc/xc_pm.c b/tools/libxc/xc_pm.c
index 823bab6..300de33 100644
--- a/tools/libxc/xc_pm.c
+++ b/tools/libxc/xc_pm.c
@@ -261,13 +261,16 @@ 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.freq = sys_para-scaling_max.freq;
+user_para-scaling_min.freq = sys_para-scaling_min.freq;
+user_para-scaling_max.pct = sys_para-scaling_max.pct;
+user_para-scaling_min.pct = sys_para-scaling_min.pct;
+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);
diff --git a/tools/misc/xenpm.c b/tools/misc/xenpm.c
index 2f9bd8e..ea6a32f 100644
--- a/tools/misc/xenpm.c
+++ b/tools/misc/xenpm.c
@@ -33,6 +33,11 @@
#define MAX_CORE_RESIDENCIES 8
#define ARRAY_SIZE(a) (sizeof (a) / sizeof ((a)[0]))
+#define min_t(type,x,y) \
+({ type __x = (x); type __y = (y); __x __y ? __x: __y; })
+#define max_t(type,x,y) \
+({ type __x = (x); type __y = (y); __x __y ? __x: __y; })
+#define clamp_t(type, val, lo, hi) min_t(type, max_t(type, val, lo), hi)
static xc_interface *xc_handle;
static unsigned int max_cpu_nr;
@@ -47,6 +52,9 @@ void show_help(void)
get-cpuidle-states[cpuid] list cpu idle info of CPU
cpuid or all\n
get-cpufreq-states[cpuid] list cpu freq info of CPU
cpuid or all\n
get-cpufreq-para [cpuid] list cpu freq parameter of
CPU cpuid or all\n
+ set-scaling-max-pct [cpuid] num set max performance limit in
percentage\n
+ or as scaling speed in
percentage in userspace governor\n
+ set-scaling-min-pct [cpuid] num set min performance limit in
percentage\n
set-scaling-maxfreq [cpuid] HZ set max cpu frequency HZ
on CPU cpuid\n
or all CPUs\n
set-scaling-minfreq [cpuid] HZ set min cpu frequency HZ
on CPU cpuid\n
@@ -60,10 +68,10 @@ void show_help(void)
set-up-threshold [cpuid] num set up threshold on CPU
cpuid or all\n
it is used in ondemand
governor.\n
get-cpu-topologyget thread/core/socket
topology info\n
- set-sched-smt enable|disable enable/disable scheduler
smt power saving\n
+ set-sched-smt enable|disable
enable/disable scheduler smt power saving\n
set-vcpu-migration-delay num set scheduler vcpu migration
delay in us\n
get-vcpu-migration-delayget scheduler vcpu migration
delay\n
- set-max-cstatenum set the C-State limitation
(num = 0)\n
+ set-max-cstatenum set
