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

[Jason Andryuk <jandryuk@xxxxxxxxx>]

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

Ok, I'll return activity_window.

> > +{
> > +    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.

I'll add a patch adding MASK_EXTR() & MASK_INSR() to common-macros.h
and use those - is there any reason not to do that?

I'll also add 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".

I hadn't considered that.  The hardware seems to allow you to write a
0 mantissa, non-0 exponent.  From the SDM, it's unclear what that
would mean.  The code as written would display "0 us", "0 ms", or "0
s" - not "0 hardware selected".  Do you want more explicity printing
for those cases?  I think it's fine to have a distinction between the
output.  "0 hardware selected" is the known valid value that is
working as expected.  The other ones being something different seems
good to me since we don't really know what they mean.

> > @@ -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.

I'll use blanks.

Regards,
Jason