| Message ID | 20210503192810.36084-13-jandryuk@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Intel Hardware P-States (HWP) support | expand |
On 03.05.2021 21:28, Jason Andryuk wrote: > @@ -1309,6 +1328,226 @@ void disable_turbo_mode(int argc, char *argv[]) > errno, strerror(errno)); > } > > +/* > + * Parse activity_window:NNN{us,ms,s} and validate range. > + * > + * Activity window is a 7bit mantissa (0-127) with a 3bit exponent (0-7) base > + * 10 in microseconds. So the range is 1 microsecond to 1270 seconds. A value > + * of 0 lets the hardware autonomously select the window. > + * > + * Return 0 on success > + * -1 on error > + * 1 Not activity_window. i.e. try parsing as another argument > + */ > +static int parse_activity_window(xc_set_hwp_para_t *set_hwp, char *p) > +{ > + char *param = NULL, *val = NULL, *suffix = NULL; > + unsigned int u; > + unsigned int exponent = 0; > + unsigned int multiplier = 1; > + int ret; > + > + ret = sscanf(p, "%m[a-z_A-Z]:%ms", ¶m, &val); I have to confess that I first needed to look up availability of the m modifier. It looks to be in POSIX.1-2008, but not in C11 and older. I'm therefore not sure if you can legitimately use it; I've not been able to spot pre-existing uses throughout tools/. Also, following the naming of other options of this tool (including the new set-cpufreq-hwp subcommand you add here), instead of _ options should use - (and the pattern here and in the other similar sscanf() further down then wants adjusting). > + if ( ret != 2 ) > + { > + return -1; No error message at all in this case? > + } > + > + if ( strncasecmp(param, "act", 3) != 0 ) > + { > + ret = 1; > + > + goto out; > + } > + > + free(param); > + param = NULL; > + > + ret = sscanf(val, "%u%ms", &u, &suffix); Can't you parse this right in the first sscanf()? > + if ( ret != 1 && ret != 2 ) > + { > + fprintf(stderr, "invalid activity window: %s\n", val); > + > + ret = -1; > + > + goto out; > + } > + > + if ( ret == 2 && suffix ) The help text doesn't clarify what an omitted suffix means; it's unambiguous only when the value is zero. (While looking at that I also started wondering whether the range there starting at 0us is actually appropriate - the range really starts at 1us afaict, with 0 having special meaning.) > + { > + if ( strcasecmp(suffix, "s") == 0 ) > + { > + multiplier = 1000 * 1000; > + exponent = 6; > + } > + else if ( strcasecmp(suffix, "ms") == 0 ) > + { > + multiplier = 1000; > + exponent = 3; > + } > + else if ( strcasecmp(suffix, "us") == 0 ) > + { > + multiplier = 1; > + exponent = 0; > + } > + else > + { > + fprintf(stderr, "invalid activity window units: %s\n", suffix); I think you want to generally quote %s in such cases, to make clear what is actually part of a malformed string. > + ret = -1; > + goto out; > + } > + } > + > + if ( u > 1270 * 1000 * 1000 / multiplier ) > + { > + fprintf(stderr, "activity window %s too large\n", val); > + > + ret = -1; > + goto out; > + } > + > + /* looking for 7 bits of mantissa and 3 bits of exponent */ > + while ( u > 127 ) Prior to this loop, don't you need to multiply by "multiplier"? > + { > + u /= 10; Fractions get silently chopped off - may want spelling out in the help text. > + exponent += 1; > + } > + > + set_hwp->activity_window = ( exponent & 0x7 ) << 7 | ( u & 0x7f ); Excess blanks inside parentheses again. > +static int parse_hwp_opts(xc_set_hwp_para_t *set_hwp, int *cpuid, > + int argc, char *argv[]) > +{ > + int i = 0; > + > + if ( argc < 1 ) > + return -1; > + > + if ( parse_cpuid_non_fatal(argv[i], cpuid) == 0 ) > + { > + i++; > + } Unnecessary braces again, the more that you ... > + if ( i == argc ) > + return -1; ... don't have any here. > + if ( strcasecmp(argv[i], "powersave") == 0 ) > + { > + set_hwp->set_params = XEN_SYSCTL_HWP_SET_PRESET_POWERSAVE; > + i++; > + } > + else if ( strcasecmp(argv[i], "performance") == 0 ) > + { > + set_hwp->set_params = XEN_SYSCTL_HWP_SET_PRESET_PERFORMANCE; > + i++; > + } > + else if ( strcasecmp(argv[i], "balance") == 0 ) > + { > + set_hwp->set_params = XEN_SYSCTL_HWP_SET_PRESET_BALANCE; > + i++; > + } > + > + for ( ; i < argc; i++) > + { > + unsigned int val; > + char *param; > + int ret; > + > + ret = parse_activity_window(set_hwp, argv[i]); > + switch ( ret ) > + { > + case -1: > + return -1; > + case 0: > + continue; > + break; Why "break" after "continue"? I can see compilers legitimately warning in such a case. > + case 1: This may better be "default:", or could be omitted altogether. Or alternatively you may want to have a "default:" with assert(). > + /* try other parsing */ > + break; > + } > + > + /* sscanf can't handle split on ':' for "%ms:%u' */ > + ret = sscanf(argv[i], "%m[a-zA-Z_]:%u", ¶m, &val); > + if ( ret != 2 ) > + { > + fprintf(stderr, "%s is an invalid hwp parameter.\n", argv[i]); Outside of this function you omit full stops from error messages. Elsewhere in the tool full stops are also absent except in two or three deprecation warnings. Hence I think you want to drop them from messages in this function. > + return -1; > + } > + > + if ( val > 255 ) > + { > + fprintf(stderr, "%s value %u is out of range.\n", param, val); > + return -1; > + } > + > + if ( strncasecmp(param, "min", 3) == 0 ) > + { > + set_hwp->minimum = val; > + set_hwp->set_params |= XEN_SYSCTL_HWP_SET_MINIMUM; > + } > + else if ( strncasecmp(param, "max", 3) == 0 ) > + { > + set_hwp->maximum = val; > + set_hwp->set_params |= XEN_SYSCTL_HWP_SET_MAXIMUM; > + } > + else if ( strncasecmp(param, "des", 3) == 0 ) > + { > + set_hwp->desired = val; > + set_hwp->set_params |= XEN_SYSCTL_HWP_SET_DESIRED; > + } > + else if ( strncasecmp(param, "ene", 3) == 0 ) > + { > + set_hwp->energy_perf = val; > + set_hwp->set_params |= XEN_SYSCTL_HWP_SET_ENERGY_PERF; > + } While I can see the point of limiting to 3 characters, you would better not accept longer but e.g. typoed strings. > + else > + { > + fprintf(stderr, "%s is an invalid parameter\n.", param); > + return -1; > + } > + > + free(param); > + } > + > + return 0; Should you perhaps return an error here if set_hwp->set_params is still zero? > +} > + > +static void hwp_set_func(int argc, char *argv[]) > +{ > + xc_set_hwp_para_t set_hwp = {}; > + int cpuid = -1; > + int i = 0; > + > + if ( parse_hwp_opts(&set_hwp, &cpuid, argc, argv) ) > + { > + fprintf(stderr, "Missing, excess, or invalid argument(s)\n"); Isn't this redundant with earlier logged messages, which are also more specific (with the one exception noted)? Jan
diff --git a/tools/misc/xenpm.c b/tools/misc/xenpm.c index a686f8f46e..d3bcaf3b58 100644 --- a/tools/misc/xenpm.c +++ b/tools/misc/xenpm.c @@ -67,6 +67,25 @@ void show_help(void) " set-max-cstate <num>|'unlimited' [<num2>|'unlimited']\n" " set the C-State limitation (<num> >= 0) and\n" " optionally the C-sub-state limitation (<num2> >= 0)\n" + " set-cpufreq-hwp [cpuid] [balance|performance|powersave] <param:val>*\n" + " set Hardware P-State (HWP) parameters\n" + " optionally a preset of one of\n" + " balance|performance|powersave\n" + " an optional list of param:val arguments\n" + " minimum:N hw_lowest ... hw_highest\n" + " maximum:N hw_lowest ... hw_highest\n" + " desired:N hw_lowest ... hw_highest\n" + " Set explicit performance target.\n" + " non-zero disables auto-HWP mode.\n" + " energy_perf:0-255 (or 0-15)\n" + " energy/performance hint\n" + " lower favor performance\n" + " higher favor powersave\n" + " 127 (or 7) balance\n" + " act_window:N{,m,u}s range 0us-1270s\n" + " window for internal calculations.\n" + " 0 lets the hardware decide.\n" + " get-cpufreq-para returns hw_lowest/highest.\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" @@ -1309,6 +1328,226 @@ void disable_turbo_mode(int argc, char *argv[]) errno, strerror(errno)); } +/* + * Parse activity_window:NNN{us,ms,s} and validate range. + * + * Activity window is a 7bit mantissa (0-127) with a 3bit exponent (0-7) base + * 10 in microseconds. So the range is 1 microsecond to 1270 seconds. A value + * of 0 lets the hardware autonomously select the window. + * + * Return 0 on success + * -1 on error + * 1 Not activity_window. i.e. try parsing as another argument + */ +static int parse_activity_window(xc_set_hwp_para_t *set_hwp, char *p) +{ + char *param = NULL, *val = NULL, *suffix = NULL; + unsigned int u; + unsigned int exponent = 0; + unsigned int multiplier = 1; + int ret; + + ret = sscanf(p, "%m[a-z_A-Z]:%ms", ¶m, &val); + if ( ret != 2 ) + { + return -1; + } + + if ( strncasecmp(param, "act", 3) != 0 ) + { + ret = 1; + + goto out; + } + + free(param); + param = NULL; + + ret = sscanf(val, "%u%ms", &u, &suffix); + if ( ret != 1 && ret != 2 ) + { + fprintf(stderr, "invalid activity window: %s\n", val); + + ret = -1; + + goto out; + } + + if ( ret == 2 && suffix ) + { + if ( strcasecmp(suffix, "s") == 0 ) + { + multiplier = 1000 * 1000; + exponent = 6; + } + else if ( strcasecmp(suffix, "ms") == 0 ) + { + multiplier = 1000; + exponent = 3; + } + else if ( strcasecmp(suffix, "us") == 0 ) + { + multiplier = 1; + exponent = 0; + } + else + { + fprintf(stderr, "invalid activity window units: %s\n", suffix); + + ret = -1; + goto out; + } + } + + if ( u > 1270 * 1000 * 1000 / multiplier ) + { + fprintf(stderr, "activity window %s too large\n", val); + + ret = -1; + goto out; + } + + /* looking for 7 bits of mantissa and 3 bits of exponent */ + while ( u > 127 ) + { + u /= 10; + exponent += 1; + } + + set_hwp->activity_window = ( exponent & 0x7 ) << 7 | ( u & 0x7f ); + set_hwp->set_params |= XEN_SYSCTL_HWP_SET_ACT_WINDOW; + + ret = 0; + + out: + free(suffix); + free(param); + free(val); + + return ret; +} + +static int parse_hwp_opts(xc_set_hwp_para_t *set_hwp, int *cpuid, + int argc, char *argv[]) +{ + int i = 0; + + if ( argc < 1 ) + return -1; + + if ( parse_cpuid_non_fatal(argv[i], cpuid) == 0 ) + { + i++; + } + + if ( i == argc ) + return -1; + + if ( strcasecmp(argv[i], "powersave") == 0 ) + { + set_hwp->set_params = XEN_SYSCTL_HWP_SET_PRESET_POWERSAVE; + i++; + } + else if ( strcasecmp(argv[i], "performance") == 0 ) + { + set_hwp->set_params = XEN_SYSCTL_HWP_SET_PRESET_PERFORMANCE; + i++; + } + else if ( strcasecmp(argv[i], "balance") == 0 ) + { + set_hwp->set_params = XEN_SYSCTL_HWP_SET_PRESET_BALANCE; + i++; + } + + for ( ; i < argc; i++) + { + unsigned int val; + char *param; + int ret; + + ret = parse_activity_window(set_hwp, argv[i]); + switch ( ret ) + { + case -1: + return -1; + case 0: + continue; + break; + case 1: + /* try other parsing */ + break; + } + + /* sscanf can't handle split on ':' for "%ms:%u' */ + ret = sscanf(argv[i], "%m[a-zA-Z_]:%u", ¶m, &val); + if ( ret != 2 ) + { + fprintf(stderr, "%s is an invalid hwp parameter.\n", argv[i]); + return -1; + } + + if ( val > 255 ) + { + fprintf(stderr, "%s value %u is out of range.\n", param, val); + return -1; + } + + if ( strncasecmp(param, "min", 3) == 0 ) + { + set_hwp->minimum = val; + set_hwp->set_params |= XEN_SYSCTL_HWP_SET_MINIMUM; + } + else if ( strncasecmp(param, "max", 3) == 0 ) + { + set_hwp->maximum = val; + set_hwp->set_params |= XEN_SYSCTL_HWP_SET_MAXIMUM; + } + else if ( strncasecmp(param, "des", 3) == 0 ) + { + set_hwp->desired = val; + set_hwp->set_params |= XEN_SYSCTL_HWP_SET_DESIRED; + } + else if ( strncasecmp(param, "ene", 3) == 0 ) + { + set_hwp->energy_perf = val; + set_hwp->set_params |= XEN_SYSCTL_HWP_SET_ENERGY_PERF; + } + else + { + fprintf(stderr, "%s is an invalid parameter\n.", param); + return -1; + } + + free(param); + } + + return 0; +} + +static void hwp_set_func(int argc, char *argv[]) +{ + xc_set_hwp_para_t set_hwp = {}; + int cpuid = -1; + int i = 0; + + if ( parse_hwp_opts(&set_hwp, &cpuid, argc, argv) ) + { + fprintf(stderr, "Missing, excess, or invalid argument(s)\n"); + exit(EINVAL); + } + + if ( cpuid != -1 ) + { + i = cpuid; + max_cpu_nr = i + 1; + } + + for ( ; i < max_cpu_nr; i++ ) + if ( xc_set_cpufreq_hwp(xc_handle, i, &set_hwp) ) + fprintf(stderr, "[CPU%d] failed to set hwp params (%d - %s)\n", + i, errno, strerror(errno)); +} + struct { const char *name; void (*function)(int argc, char *argv[]); @@ -1319,6 +1558,7 @@ struct { { "get-cpufreq-average", cpufreq_func }, { "start", start_gather_func }, { "get-cpufreq-para", cpufreq_para_func }, + { "set-cpufreq-hwp", hwp_set_func }, { "set-scaling-maxfreq", scaling_max_freq_func }, { "set-scaling-minfreq", scaling_min_freq_func }, { "set-scaling-governor", scaling_governor_func },
set-cpufreq-hwp allows setting the Hardware P-State (HWP) parameters. It can be run on all or just a single cpu. There are presets of balance, powersave & performance. Those can be further tweaked by param:val arguments as explained in the usage description. Parameter names are just checked to the first 3 characters to shorten typing. Some options are hardware dependent, and ranges can be found in get-cpufreq-para. Signed-off-by: Jason Andryuk <jandryuk@gmail.com> --- tools/misc/xenpm.c | 240 +++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 240 insertions(+)