| Message ID | 20230501193034.88575-14-jandryuk@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Intel Hardware P-States (HWP) support | expand |
On 01.05.2023 21:30, Jason Andryuk wrote: > @@ -67,6 +68,27 @@ 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 lowest ... highest\n" > + " maximum:N lowest ... highest\n" > + " desired:N lowest ... highest\n" Personally I consider these three uses of "lowest ... highest" confusing: It's not clear at all whether they're part of the option or merely mean to express the allowable range for N (which I think they do). Perhaps ... > + " Set explicit performance target.\n" > + " non-zero disables auto-HWP mode.\n" > + " energy-perf:0-255 (or 0-15)\n" ..., also taking this into account: " energy-perf:N (0-255 or 0-15)\n" and then use parentheses as well for the earlier value range explanations (and again below)? Also up from here you suddenly start having full stops on the lines. I guess you also want to be consistent in your use of capital letters at the start of lines (I didn't go check how consistent pre-existing code is in this regard). > @@ -1299,6 +1321,213 @@ 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 > + */ > +static int parse_activity_window(xc_set_hwp_para_t *set_hwp, unsigned long u, > + const char *suffix) > +{ > + unsigned int exponent = 0; > + unsigned int multiplier = 1; > + > + if ( suffix && suffix[0] ) > + { > + 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; > + } Considering the initializers, this "else if" body isn't really needed, and ... > + else ... instead this could become "else if ( strcmp() != 0 )". Note also that I use strcmp() there - none of s, ms, or us are commonly expressed by capital letters. (I wonder though whether μs shouldn't also be recognized.) > + { > + fprintf(stderr, "invalid activity window units: \"%s\"\n", suffix); > + > + return -1; > + } > + } > + > + /* u * multipler > 1270 * 1000 * 1000 transformed to avoid overflow. */ > + if ( u > 1270 * 1000 * 1000 / multiplier ) > + { > + fprintf(stderr, "activity window is too large\n"); > + > + return -1; > + } > + > + /* looking for 7 bits of mantissa and 3 bits of exponent */ > + while ( u > 127 ) > + { > + u += 5; /* Round up to mitigate truncation rounding down > + e.g. 128 -> 120 vs 128 -> 130. */ > + u /= 10; > + exponent += 1; > + } > + > + set_hwp->activity_window = (exponent & HWP_ACT_WINDOW_EXPONENT_MASK) << > + HWP_ACT_WINDOW_EXPONENT_SHIFT | The shift wants parenthesizing against the | and the shift amount wants indenting slightly less. (Really this would want to be MASK_INSR().) > + (u & HWP_ACT_WINDOW_MANTISSA_MASK); > + set_hwp->set_params |= XEN_SYSCTL_HWP_SET_ACT_WINDOW; > + > + return 0; > +} > + > +static int parse_hwp_opts(xc_set_hwp_para_t *set_hwp, int *cpuid, > + int argc, char *argv[]) > +{ > + int i = 0; > + > + if ( argc < 1 ) { > + fprintf(stderr, "Missing arguments\n"); > + return -1; > + } > + > + if ( parse_cpuid_non_fatal(argv[i], cpuid) == 0 ) > + { > + i++; > + } I don't think you need the earlier patch and the separate helper: Whether a CPU number is present can be told by checking isdigit(argv[i][0]). Also (nit) note how you're mixing brace placement throughout this function. Jan
On 08.05.2023 13:56, Jan Beulich wrote: > On 01.05.2023 21:30, Jason Andryuk wrote: >> +static int parse_hwp_opts(xc_set_hwp_para_t *set_hwp, int *cpuid, >> + int argc, char *argv[]) >> +{ >> + int i = 0; >> + >> + if ( argc < 1 ) { >> + fprintf(stderr, "Missing arguments\n"); >> + return -1; >> + } >> + >> + if ( parse_cpuid_non_fatal(argv[i], cpuid) == 0 ) >> + { >> + i++; >> + } > > I don't think you need the earlier patch and the separate helper: > Whether a CPU number is present can be told by checking > isdigit(argv[i][0]). Hmm, yes, there is "all", but your help text doesn't mention it and since you're handling a variable number of arguments anyway, there's not need for anyone to say "all" - they can simply omit the optional argument. Jan
On Mon, May 8, 2023 at 7:56 AM Jan Beulich <jbeulich@suse.com> wrote: > > On 01.05.2023 21:30, Jason Andryuk wrote: > > @@ -67,6 +68,27 @@ 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 lowest ... highest\n" > > + " maximum:N lowest ... highest\n" > > + " desired:N lowest ... highest\n" > > Personally I consider these three uses of "lowest ... highest" confusing: > It's not clear at all whether they're part of the option or merely mean > to express the allowable range for N (which I think they do). Perhaps ... > > > + " Set explicit performance target.\n" > > + " non-zero disables auto-HWP mode.\n" > > + " energy-perf:0-255 (or 0-15)\n" > > ..., also taking this into account: > > " energy-perf:N (0-255 or 0-15)\n" > > and then use parentheses as well for the earlier value range explanations > (and again below)? lowest and highest were supposed to reference the values from `xenpm get-cpufreq-para`. You removed some later lines that state "get-cpufreq-para returns lowest/highest". However, they aren't enforced limits. You can program from the range 0-255 and the hardware is supposed to clip internally, so your idea of "energy-perf:N (0-255)" seems good to me. > Also up from here you suddenly start having full stops on the lines. I > guess you also want to be consistent in your use of capital letters at > the start of lines (I didn't go check how consistent pre-existing code > is in this regard). Looks like the existing code is consistently non-capital letters, but the full stops are inconsistent. I'll go with non-capital and full stop for this addition. > > @@ -1299,6 +1321,213 @@ 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 > > + */ > > +static int parse_activity_window(xc_set_hwp_para_t *set_hwp, unsigned long u, > > + const char *suffix) > > +{ > > + unsigned int exponent = 0; > > + unsigned int multiplier = 1; > > + > > + if ( suffix && suffix[0] ) > > + { > > + 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; > > + } > > Considering the initializers, this "else if" body isn't really needed, > and ... > > > + else > > ... instead this could become "else if ( strcmp() != 0 )". > > Note also that I use strcmp() there - none of s, ms, or us are commonly > expressed by capital letters. That sounds fine. > (I wonder though whether μs shouldn't also > be recognized.) While that makes sense, I do not plan to change it. I don't know the proper way to deal with unicode from C. (I suppose a memcmp with the UTF-8 encoding would be possible, but I don't know if there are corner cases I'm overlooking.) > > + { > > + fprintf(stderr, "invalid activity window units: \"%s\"\n", suffix); > > + > > + return -1; > > + } > > + } > > + > > + /* u * multipler > 1270 * 1000 * 1000 transformed to avoid overflow. */ > > + if ( u > 1270 * 1000 * 1000 / multiplier ) > > + { > > + fprintf(stderr, "activity window is too large\n"); > > + > > + return -1; > > + } > > + > > + /* looking for 7 bits of mantissa and 3 bits of exponent */ > > + while ( u > 127 ) > > + { > > + u += 5; /* Round up to mitigate truncation rounding down > > + e.g. 128 -> 120 vs 128 -> 130. */ > > + u /= 10; > > + exponent += 1; > > + } > > + > > + set_hwp->activity_window = (exponent & HWP_ACT_WINDOW_EXPONENT_MASK) << > > + HWP_ACT_WINDOW_EXPONENT_SHIFT | > > The shift wants parenthesizing against the | and the shift amount wants > indenting slightly less. (Really this would want to be MASK_INSR().) I'll use MASK_INSR. > > + (u & HWP_ACT_WINDOW_MANTISSA_MASK); > > + set_hwp->set_params |= XEN_SYSCTL_HWP_SET_ACT_WINDOW; > > + > > + return 0; > > +} > > + > > +static int parse_hwp_opts(xc_set_hwp_para_t *set_hwp, int *cpuid, > > + int argc, char *argv[]) > > +{ > > + int i = 0; > > + > > + if ( argc < 1 ) { > > + fprintf(stderr, "Missing arguments\n"); > > + return -1; > > + } > > + > > + if ( parse_cpuid_non_fatal(argv[i], cpuid) == 0 ) > > + { > > + i++; > > + } > > I don't think you need the earlier patch and the separate helper: > Whether a CPU number is present can be told by checking > isdigit(argv[i][0]). > Hmm, yes, there is "all", but your help text doesn't mention it and > since you're handling a variable number of arguments anyway, there's > not need for anyone to say "all" - they can simply omit the optional > argument. Most xenpm commands take "all" or a numeric cpuid, so I intended to be consistent with them. That was the whole point of parse_cpuid_non_fatal() - to reuse the existing parsing code for consistency. I didn't read the other help text carefully enough to see that the numeric cpuid and "all" handling was repeated. For consistency, I would retain parse_cpuid_non_fatal() and expand the help text. If you don't want that, I'll switch to isdigit(argv[i][0]) and have the omission of a digit indicate all CPUs as you suggest. Just let me know what you want. > Also (nit) note how you're mixing brace placement throughout this > function. Will fix. Regards, Jason
On 22.05.2023 14:59, Jason Andryuk wrote: > On Mon, May 8, 2023 at 7:56 AM Jan Beulich <jbeulich@suse.com> wrote: >> On 01.05.2023 21:30, Jason Andryuk wrote: >>> +static int parse_hwp_opts(xc_set_hwp_para_t *set_hwp, int *cpuid, >>> + int argc, char *argv[]) >>> +{ >>> + int i = 0; >>> + >>> + if ( argc < 1 ) { >>> + fprintf(stderr, "Missing arguments\n"); >>> + return -1; >>> + } >>> + >>> + if ( parse_cpuid_non_fatal(argv[i], cpuid) == 0 ) >>> + { >>> + i++; >>> + } >> >> I don't think you need the earlier patch and the separate helper: >> Whether a CPU number is present can be told by checking >> isdigit(argv[i][0]). > >> Hmm, yes, there is "all", but your help text doesn't mention it and >> since you're handling a variable number of arguments anyway, there's >> not need for anyone to say "all" - they can simply omit the optional >> argument. > > Most xenpm commands take "all" or a numeric cpuid, so I intended to be > consistent with them. That was the whole point of > parse_cpuid_non_fatal() - to reuse the existing parsing code for > consistency. > > I didn't read the other help text carefully enough to see that the > numeric cpuid and "all" handling was repeated. > > For consistency, I would retain parse_cpuid_non_fatal() and expand the > help text. If you don't want that, I'll switch to isdigit(argv[i][0]) > and have the omission of a digit indicate all CPUs as you suggest. > Just let me know what you want. While I don't want to push you towards something you don't like yourself, my view on the "all" has been "Why did they introduce that?" It makes some sense when it's a placeholder to avoid needing to deal with a variable number of arguments, but already that doesn't apply to all the pre-existing operations. Note how many functions already have if ( argc > 0 ) parse_cpuid(argv[0], &cpuid); and {en,dis}able-turbo-mode don't properly mention "all" in their help text either. Jan
diff --git a/tools/misc/xenpm.c b/tools/misc/xenpm.c index 6e74606970..8d99c78670 100644 --- a/tools/misc/xenpm.c +++ b/tools/misc/xenpm.c @@ -16,6 +16,7 @@ */ #define MAX_NR_CPU 512 +#include <limits.h> #include <stdio.h> #include <stdlib.h> #include <unistd.h> @@ -67,6 +68,27 @@ 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 lowest ... highest\n" + " maximum:N lowest ... highest\n" + " desired:N lowest ... 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" + " 128 (or 7) - balance\n" + " act-window:N{,m,u}s range 1us-1270s\n" + " window for internal calculations.\n" + " Defaults to us without units.\n" + " Truncates un-representable values.\n" + " 0 lets the hardware decide.\n" + " get-cpufreq-para returns 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" @@ -1299,6 +1321,213 @@ 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 + */ +static int parse_activity_window(xc_set_hwp_para_t *set_hwp, unsigned long u, + const char *suffix) +{ + unsigned int exponent = 0; + unsigned int multiplier = 1; + + if ( suffix && suffix[0] ) + { + 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); + + return -1; + } + } + + /* u * multipler > 1270 * 1000 * 1000 transformed to avoid overflow. */ + if ( u > 1270 * 1000 * 1000 / multiplier ) + { + fprintf(stderr, "activity window is too large\n"); + + return -1; + } + + /* looking for 7 bits of mantissa and 3 bits of exponent */ + while ( u > 127 ) + { + u += 5; /* Round up to mitigate truncation rounding down + e.g. 128 -> 120 vs 128 -> 130. */ + u /= 10; + exponent += 1; + } + + set_hwp->activity_window = (exponent & HWP_ACT_WINDOW_EXPONENT_MASK) << + HWP_ACT_WINDOW_EXPONENT_SHIFT | + (u & HWP_ACT_WINDOW_MANTISSA_MASK); + set_hwp->set_params |= XEN_SYSCTL_HWP_SET_ACT_WINDOW; + + return 0; +} + +static int parse_hwp_opts(xc_set_hwp_para_t *set_hwp, int *cpuid, + int argc, char *argv[]) +{ + int i = 0; + + if ( argc < 1 ) { + fprintf(stderr, "Missing arguments\n"); + return -1; + } + + if ( parse_cpuid_non_fatal(argv[i], cpuid) == 0 ) + { + i++; + } + + if ( i == argc ) { + fprintf(stderr, "Missing arguments\n"); + 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 long val; + char *param = argv[i]; + char *value; + char *suffix; + int ret; + + value = strchr(param, ':'); + if ( value == NULL ) + { + fprintf(stderr, "\"%s\" is an invalid hwp parameter\n", argv[i]); + return -1; + } + + value[0] = '\0'; + value++; + + errno = 0; + val = strtoul(value, &suffix, 10); + if ( (errno && val == ULONG_MAX) || value == suffix ) + { + fprintf(stderr, "Could not parse number \"%s\"\n", value); + return -1; + } + + if ( strncasecmp(param, "act-window", strlen(param)) == 0 ) + { + ret = parse_activity_window(set_hwp, val, suffix); + if (ret) + return -1; + + continue; + } + + if ( val > 255 ) + { + fprintf(stderr, "\"%s\" value \"%lu\" is out of range\n", param, + val); + return -1; + } + + if ( suffix && suffix[0] ) + { + fprintf(stderr, "Suffix \"%s\" is invalid\n", suffix); + return -1; + } + + if ( strncasecmp(param, "minimum", MAX(2, strlen(param))) == 0 ) + { + set_hwp->minimum = val; + set_hwp->set_params |= XEN_SYSCTL_HWP_SET_MINIMUM; + } + else if ( strncasecmp(param, "maximum", MAX(2, strlen(param))) == 0 ) + { + set_hwp->maximum = val; + set_hwp->set_params |= XEN_SYSCTL_HWP_SET_MAXIMUM; + } + else if ( strncasecmp(param, "desired", strlen(param)) == 0 ) + { + set_hwp->desired = val; + set_hwp->set_params |= XEN_SYSCTL_HWP_SET_DESIRED; + } + else if ( strncasecmp(param, "energy-perf", strlen(param)) == 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; + } + } + + if ( set_hwp->set_params == 0 ) + { + fprintf(stderr, "No parameters set in request\n"); + return -1; + } + + 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) ) + 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[]); @@ -1309,6 +1538,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> --- v2: Compare provided parameter name and not just 3 characters. Use "-" in parameter names Remove hw_ Replace sscanf with strchr & strtoul. Remove toplevel error message with lower level ones. Help text s/127/128/ Help text mention truncation. Avoid some truncation rounding down by adding 5 before division. Help test mention default microseconds Also comment the limit check written to avoid overflow. --- tools/misc/xenpm.c | 230 +++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 230 insertions(+)