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