Re: [PATCH v3 09/14 RESEND] xenpm: Print HWP parameters

[Jan Beulich <jbeulich@xxxxxxxx>]

On 01.05.2023 21:30, Jason Andryuk wrote:
> Print HWP-specific parameters.  Some are always present, but others
> depend on hardware support.
> 
> Signed-off-by: Jason Andryuk <jandryuk@xxxxxxxxx>
> ---
> v2:
> Style fixes
> Declare i outside loop
> Replace repearted hardware/configured limits with spaces
> Fixup for hw_ removal
> Use XEN_HWP_GOVERNOR
> Use HWP_ACT_WINDOW_EXPONENT_*
> Remove energy_perf hw autonomous - 0 doesn't mean autonomous
> ---
>  tools/misc/xenpm.c | 65 ++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 65 insertions(+)
> 
> diff --git a/tools/misc/xenpm.c b/tools/misc/xenpm.c
> index ce8d7644d0..b2defde0d4 100644
> --- a/tools/misc/xenpm.c
> +++ b/tools/misc/xenpm.c
> @@ -708,6 +708,44 @@ void start_gather_func(int argc, char *argv[])
>      pause();
>  }
>  
> +static void calculate_hwp_activity_window(const xc_hwp_para_t *hwp,
> +                                          unsigned int *activity_window,
> +                                          const char **units)

The function's return value would be nice to use for one of the two
values that are being returned.

> +{
> +    unsigned int mantissa = hwp->activity_window & 
> HWP_ACT_WINDOW_MANTISSA_MASK;
> +    unsigned int exponent =
> +        (hwp->activity_window >> HWP_ACT_WINDOW_EXPONENT_SHIFT) &
> +            HWP_ACT_WINDOW_EXPONENT_MASK;

I wish we had MASK_EXTR() in common-macros.h. While really a comment on
patch 7 - HWP_ACT_WINDOW_EXPONENT_SHIFT is redundant information and
should imo be omitted from the public interface, in favor of just a
(suitably shifted) mask value. Also note how those constants all lack
proper XEN_ prefixes.

> +    unsigned int multiplier = 1;
> +    unsigned int i;
> +
> +    if ( hwp->activity_window == 0 )
> +    {
> +        *units = "hardware selected";
> +        *activity_window = 0;
> +
> +        return;
> +    }

While in line with documentation, any mantissa of 0 results in a 0us
window, which I assume would then also mean "hardware selected".

> @@ -773,6 +811,33 @@ static void print_cpufreq_para(int cpuid, struct 
> xc_get_cpufreq_para *p_cpufreq)
>                 p_cpufreq->scaling_cur_freq);
>      }
>  
> +    if ( strcmp(p_cpufreq->scaling_governor, XEN_HWP_GOVERNOR) == 0 )
> +    {
> +        const xc_hwp_para_t *hwp = &p_cpufreq->u.hwp_para;
> +
> +        printf("hwp variables        :\n");
> +        printf("  hardware limits    : lowest [%u] most_efficient [%u]\n",

Here and ...

> +               hwp->lowest, hwp->most_efficient);
> +        printf("                     : guaranteed [%u] highest [%u]\n",
> +               hwp->guaranteed, hwp->highest);
> +        printf("  configured limits  : min [%u] max [%u] energy_perf [%u]\n",

... here I wonder what use the underscores are in produced output. I'd
use blanks. If you really want a separator there, then please use
dashes.

Jan