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

[Jan Beulich <jbeulich@xxxxxxxx>]

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