Re: [PATCH v7 11/15] xenpm: Print HWP/CPPC parameters

[Jason Andryuk <jandryuk@xxxxxxxxx>]

On Thu, Jul 27, 2023 at 7:32 AM Anthony PERARD
<anthony.perard@xxxxxxxxxx> wrote:
>
> On Wed, Jul 26, 2023 at 01:09:41PM -0400, Jason Andryuk wrote:
> > @@ -772,6 +812,32 @@ static void print_cpufreq_para(int cpuid, struct 
> > xc_get_cpufreq_para *p_cpufreq)
> >                 p_cpufreq->u.s.scaling_min_freq,
> >                 p_cpufreq->u.s.scaling_cur_freq);
> >      }
> > +    else
> > +    {
>
> I feel like this could be confusing. In this function, we have both:
>     if ( hwp ) { this; } else { that; }
> and
>     if ( !hwp ) { that; } else { this; }
>
> If we could have the condition in the same order, or use the same
> condition for both "true" blocks, that would be nice.

Makes sense.  From your comment on patch 8, I was thinking of
switching to a bool scaling_governor and using that.  But now I think
hwp is better since it's the specific governor - not the default one.
In light of that, I would just re-organize everything to be "if ( hwp
)".

With that, patch 8 is more of a transitive step, and I would leave the
"if ( !hwp )".  Then here in patch 11 the HWP code would be added
first inside "if ( hwp )".  Does that sound good?

> > +        const xc_cppc_para_t *cppc = &p_cpufreq->u.cppc_para;
> > +
> > +        printf("cppc variables       :\n");
> > +        printf("  hardware limits    : lowest [%u] lowest nonlinear 
> > [%u]\n",
> > +               cppc->lowest, cppc->lowest_nonlinear);
>
> All these %u should be %"PRIu32", right? Even if the rest of the
> function is bogus... and even if it's probably be a while before %PRIu32
> is different from %u.

Yes, you are correct.  In patch 8 "xenpm: Change get-cpufreq-para
output for hwp", some existing %u-s are moved and a few more added.
Do you want all of those converted to %PRIu32?

Thanks,
Jason