Re: [PATCH v3 13/14 RESEND] xenpm: Add set-cpufreq-hwp subcommand

[Jason Andryuk <jandryuk@xxxxxxxxx>]

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