On Mon, May 8, 2023 at 7:56 AM Jan Beulich <jbeul...@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
