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
