* [PATCH v4 11/11] tools: enable xenpm to control the intel_pstate driver
@ 2015-06-25 11:19 Wei Wang
2015-07-07 8:55 ` Wei Liu
0 siblings, 1 reply; 7+ messages in thread
From: Wei Wang @ 2015-06-25 11:19 UTC (permalink / raw)
To: ian.campbell, wei.liu2, ian.jackson, stefano.stabellini,
jbeulich, andrew.cooper3, xen-devel
Cc: Wei Wang
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.wang@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-topology get thread/core/socket topology info\n"
- " set-sched-smt enable|disable enable/disable scheduler smt power saving\n"
+ " set-sched-smt enable|disable enable/disable scheduler smt power saving\n"
" set-vcpu-migration-delay <num> set scheduler vcpu migration delay in us\n"
" get-vcpu-migration-delay get scheduler vcpu migration delay\n"
- " set-max-cstate <num> set the C-State limitation (<num> >= 0)\n"
+ " set-max-cstate <num> set the C-State limitation (<num> >= 0)\n"
" start [seconds] start collect Cx/Px statistics,\n"
" output after CTRL-C or SIGINT or several seconds.\n"
" enable-turbo-mode [cpuid] enable Turbo Mode for processors that support it.\n"
@@ -678,38 +686,50 @@ static void print_cpufreq_para(int cpuid, struct xc_get_cpufreq_para *p_cpufreq)
printf("current_governor : %s\n", p_cpufreq->scaling_governor);
if ( !strncmp(p_cpufreq->scaling_governor,
- "userspace", CPUFREQ_NAME_LEN) )
+ "userspace", CPUFREQ_NAME_LEN) &&
+ strncmp(p_cpufreq->scaling_driver,
+ "intel_pstate", CPUFREQ_NAME_LEN) )
{
- printf(" userspace specific :\n");
- printf(" scaling_setspeed : %u\n",
+ printf("userspace specific :\n");
+ printf("scaling_setspeed : %u\n",
p_cpufreq->u.userspace.scaling_setspeed);
}
else if ( !strncmp(p_cpufreq->scaling_governor,
- "ondemand", CPUFREQ_NAME_LEN) )
+ "ondemand", CPUFREQ_NAME_LEN) &&
+ strncmp(p_cpufreq->scaling_driver,
+ "intel_pstate", CPUFREQ_NAME_LEN) )
{
- printf(" ondemand specific :\n");
- printf(" sampling_rate : max [%u] min [%u] cur [%u]\n",
+ printf("ondemand specific :\n");
+ printf("sampling_rate : max [%u] min [%u] cur [%u]\n",
p_cpufreq->u.ondemand.sampling_rate_max,
p_cpufreq->u.ondemand.sampling_rate_min,
p_cpufreq->u.ondemand.sampling_rate);
- printf(" up_threshold : %u\n",
+ printf("up_threshold : %u\n",
p_cpufreq->u.ondemand.up_threshold);
}
- printf("scaling_avail_freq :");
- for ( i = 0; i < p_cpufreq->freq_num; i++ )
- if ( p_cpufreq->scaling_available_frequencies[i] ==
- p_cpufreq->scaling_cur_freq )
- printf(" *%d", p_cpufreq->scaling_available_frequencies[i]);
- else
- printf(" %d", p_cpufreq->scaling_available_frequencies[i]);
- printf("\n");
-
- printf("scaling frequency : max [%u] min [%u] cur [%u]\n",
- p_cpufreq->scaling_max_freq,
- p_cpufreq->scaling_min_freq,
- p_cpufreq->scaling_cur_freq);
-
+ if (!strncmp(p_cpufreq->scaling_driver,
+ "intel_pstate", CPUFREQ_NAME_LEN) )
+ {
+ printf("max_perf_pct : %d\n", p_cpufreq->scaling_max.pct);
+ printf("min_perf_pct : %d\n", p_cpufreq->scaling_min.pct);
+ printf("turbo_pct : %d\n", p_cpufreq->scaling_turbo_pct);
+ }
+ else
+ {
+ printf("scaling_avail_freq :");
+ for ( i = 0; i < p_cpufreq->freq_num; i++ )
+ if ( p_cpufreq->scaling_available_frequencies[i] ==
+ p_cpufreq->scaling_cur_freq )
+ printf(" *%d", p_cpufreq->scaling_available_frequencies[i]);
+ else
+ printf(" %d", p_cpufreq->scaling_available_frequencies[i]);
+ printf("\n");
+ printf("scaling frequency : max [%u] min [%u] cur [%u]\n",
+ p_cpufreq->scaling_max.freq,
+ p_cpufreq->scaling_min.freq,
+ p_cpufreq->scaling_cur_freq);
+ }
printf("turbo mode : %s\n",
p_cpufreq->turbo_enabled ? "enabled" : "disabled or n/a");
printf("\n");
@@ -855,6 +875,54 @@ void scaling_min_freq_func(int argc, char *argv[])
}
}
+void scaling_max_pct_func(int argc, char *argv[])
+{
+ int cpuid = -1, pct = -1;
+
+ parse_cpuid_and_int(argc, argv, &cpuid, &pct, "percentage");
+ pct = clamp_t(int, pct, 0, 100);
+
+ if ( cpuid < 0 )
+ {
+ int i;
+ for ( i = 0; i < max_cpu_nr; i++ )
+ if ( xc_set_cpufreq_para(xc_handle, i, SCALING_MAX_PCT, pct) )
+ fprintf(stderr,
+ "[CPU%d] failed to set scaling max freq (%d - %s)\n",
+ i, errno, strerror(errno));
+ }
+ else
+ {
+ if ( xc_set_cpufreq_para(xc_handle, cpuid, SCALING_MAX_PCT, pct) )
+ fprintf(stderr, "failed to set scaling max freq (%d - %s)\n",
+ errno, strerror(errno));
+ }
+}
+
+void scaling_min_pct_func(int argc, char *argv[])
+{
+ int cpuid = -1, pct = -1;
+
+ parse_cpuid_and_int(argc, argv, &cpuid, &pct, "percentage");
+ pct = clamp_t(int, pct, 0, 100);
+
+ if ( cpuid < 0 )
+ {
+ int i;
+ for ( i = 0; i < max_cpu_nr; i++ )
+ if ( xc_set_cpufreq_para(xc_handle, i, SCALING_MIN_PCT, pct) )
+ fprintf(stderr,
+ "[CPU%d] failed to set scaling max pct (%d - %s)\n",
+ i, errno, strerror(errno));
+ }
+ else
+ {
+ if ( xc_set_cpufreq_para(xc_handle, cpuid, SCALING_MIN_PCT, pct) )
+ fprintf(stderr, "failed to set scaling min pct (%d - %s)\n",
+ errno, strerror(errno));
+ }
+}
+
void scaling_speed_func(int argc, char *argv[])
{
int cpuid = -1, speed = -1;
@@ -1134,6 +1202,8 @@ struct {
{ "get-cpufreq-para", cpufreq_para_func },
{ "set-scaling-maxfreq", scaling_max_freq_func },
{ "set-scaling-minfreq", scaling_min_freq_func },
+ { "set-scaling-max-pct", scaling_max_pct_func},
+ { "set-scaling-min-pct", scaling_min_pct_func},
{ "set-scaling-governor", scaling_governor_func },
{ "set-scaling-speed", scaling_speed_func },
{ "set-sampling-rate", scaling_sampling_rate_func },
--
1.9.1
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH v4 11/11] tools: enable xenpm to control the intel_pstate driver
2015-06-25 11:19 [PATCH v4 11/11] tools: enable xenpm to control the intel_pstate driver Wei Wang
@ 2015-07-07 8:55 ` Wei Liu
2015-07-07 12:05 ` Jan Beulich
0 siblings, 1 reply; 7+ messages in thread
From: Wei Liu @ 2015-07-07 8:55 UTC (permalink / raw)
To: Wei Wang
Cc: wei.liu2, ian.campbell, stefano.stabellini, andrew.cooper3,
ian.jackson, xen-devel, jbeulich
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.wang@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-topology get thread/core/socket topology info\n"
> - " set-sched-smt enable|disable enable/disable scheduler smt power saving\n"
> + " set-sched-smt enable|disable enable/disable scheduler smt power saving\n"
Please add a line in commit message saying that you also fixed some
indentation issues in help string.
Preferably all indentation changes (including the ones below) should be
separated from functional changes.
> " set-vcpu-migration-delay <num> set scheduler vcpu migration delay in us\n"
> " get-vcpu-migration-delay get scheduler vcpu migration delay\n"
> - " set-max-cstate <num> set the C-State limitation (<num> >= 0)\n"
> + " set-max-cstate <num> set the C-State limitation (<num> >= 0)\n"
> " start [seconds] start collect Cx/Px statistics,\n"
> " output after CTRL-C or SIGINT or several seconds.\n"
> " enable-turbo-mode [cpuid] enable Turbo Mode for processors that support it.\n"
> @@ -678,38 +686,50 @@ static void print_cpufreq_para(int cpuid, struct xc_get_cpufreq_para *p_cpufreq)
>
> printf("current_governor : %s\n", p_cpufreq->scaling_governor);
> if ( !strncmp(p_cpufreq->scaling_governor,
> - "userspace", CPUFREQ_NAME_LEN) )
> + "userspace", CPUFREQ_NAME_LEN) &&
> + strncmp(p_cpufreq->scaling_driver,
> + "intel_pstate", CPUFREQ_NAME_LEN) )
> {
> - printf(" userspace specific :\n");
> - printf(" scaling_setspeed : %u\n",
> + printf("userspace specific :\n");
> + printf("scaling_setspeed : %u\n",
And this is?
If the change in indentation is necessary and agreed upon, that's fine
by me. Otherwise this is not necessary.
If this change stays, you also need to say that in commit message,
something like "fix (improve?) the output by moving indentation".
> p_cpufreq->u.userspace.scaling_setspeed);
> }
> else if ( !strncmp(p_cpufreq->scaling_governor,
> - "ondemand", CPUFREQ_NAME_LEN) )
> + "ondemand", CPUFREQ_NAME_LEN) &&
> + strncmp(p_cpufreq->scaling_driver,
> + "intel_pstate", CPUFREQ_NAME_LEN) )
> {
> - printf(" ondemand specific :\n");
> - printf(" sampling_rate : max [%u] min [%u] cur [%u]\n",
> + printf("ondemand specific :\n");
> + printf("sampling_rate : max [%u] min [%u] cur [%u]\n",
> p_cpufreq->u.ondemand.sampling_rate_max,
> p_cpufreq->u.ondemand.sampling_rate_min,
> p_cpufreq->u.ondemand.sampling_rate);
> - printf(" up_threshold : %u\n",
> + printf("up_threshold : %u\n",
> p_cpufreq->u.ondemand.up_threshold);
> }
>
> - printf("scaling_avail_freq :");
> - for ( i = 0; i < p_cpufreq->freq_num; i++ )
> - if ( p_cpufreq->scaling_available_frequencies[i] ==
> - p_cpufreq->scaling_cur_freq )
> - printf(" *%d", p_cpufreq->scaling_available_frequencies[i]);
> - else
> - printf(" %d", p_cpufreq->scaling_available_frequencies[i]);
> - printf("\n");
> -
> - printf("scaling frequency : max [%u] min [%u] cur [%u]\n",
> - p_cpufreq->scaling_max_freq,
> - p_cpufreq->scaling_min_freq,
> - p_cpufreq->scaling_cur_freq);
> -
> + if (!strncmp(p_cpufreq->scaling_driver,
Coding style, space after '(' please.
Wei.
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v4 11/11] tools: enable xenpm to control the intel_pstate driver
2015-07-07 8:55 ` Wei Liu
@ 2015-07-07 12:05 ` Jan Beulich
2015-07-07 12:13 ` Wei Liu
0 siblings, 1 reply; 7+ messages in thread
From: Jan Beulich @ 2015-07-07 12:05 UTC (permalink / raw)
To: wei.liu2, Wei Wang
Cc: andrew.cooper3, xen-devel, ian.jackson, ian.campbell, stefano.stabellini
>>> On 07.07.15 at 10:55, <wei.liu2@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
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v4 11/11] tools: enable xenpm to control the intel_pstate driver
2015-07-07 12:05 ` Jan Beulich
@ 2015-07-07 12:13 ` Wei Liu
2015-07-08 5:15 ` Wang, Wei W
0 siblings, 1 reply; 7+ messages in thread
From: Wei Liu @ 2015-07-07 12:13 UTC (permalink / raw)
To: Jan Beulich
Cc: wei.liu2, ian.campbell, stefano.stabellini, andrew.cooper3,
ian.jackson, xen-devel, Wei Wang
On Tue, Jul 07, 2015 at 01:05:21PM +0100, Jan Beulich wrote:
> >>> On 07.07.15 at 10:55, <wei.liu2@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
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v4 11/11] tools: enable xenpm to control the intel_pstate driver
2015-07-07 12:13 ` Wei Liu
@ 2015-07-08 5:15 ` Wang, Wei W
2015-07-08 6:23 ` Jan Beulich
0 siblings, 1 reply; 7+ messages in thread
From: Wang, Wei W @ 2015-07-08 5:15 UTC (permalink / raw)
To: Wei Liu, Jan Beulich
Cc: andrew.cooper3, xen-devel, ian.jackson, ian.campbell, stefano.stabellini
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.liu2@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
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v4 11/11] tools: enable xenpm to control the intel_pstate driver
2015-07-08 5:15 ` Wang, Wei W
@ 2015-07-08 6:23 ` Jan Beulich
2015-07-08 6:50 ` Wang, Wei W
0 siblings, 1 reply; 7+ messages in thread
From: Jan Beulich @ 2015-07-08 6:23 UTC (permalink / raw)
To: Wei W Wang
Cc: Wei Liu, ian.campbell, stefano.stabellini, andrew.cooper3,
ian.jackson, xen-devel
>>> On 08.07.15 at 07:15, <wei.w.wang@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.liu2@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.
Jan
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v4 11/11] tools: enable xenpm to control the intel_pstate driver
2015-07-08 6:23 ` Jan Beulich
@ 2015-07-08 6:50 ` Wang, Wei W
0 siblings, 0 replies; 7+ messages in thread
From: Wang, Wei W @ 2015-07-08 6:50 UTC (permalink / raw)
To: Jan Beulich
Cc: Wei Liu, ian.campbell, stefano.stabellini, andrew.cooper3,
ian.jackson, xen-devel
On 08/07/2015 14:24, Jan Beulich wrote:
> >>> On 08.07.15 at 07:15, <wei.w.wang@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.liu2@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
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2015-07-08 6:50 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-06-25 11:19 [PATCH v4 11/11] tools: enable xenpm to control the intel_pstate driver Wei Wang
2015-07-07 8:55 ` Wei Liu
2015-07-07 12:05 ` Jan Beulich
2015-07-07 12:13 ` Wei Liu
2015-07-08 5:15 ` Wang, Wei W
2015-07-08 6:23 ` Jan Beulich
2015-07-08 6:50 ` Wang, Wei W
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).