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

[Anthony PERARD <anthony.perard@xxxxxxxxxx>]

On Thu, Jul 27, 2023 at 09:54:17AM -0400, Jason Andryuk wrote:
> 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?

Yes, that sounds fine.

> > > +        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?

For the newly added %u, yes, that would be nice.

As for the existing one, maybe as a separated patch, if you wish. At the
moment, patch 8 is easy to review with "--ignore-space-change".

Cheers,

-- 
Anthony PERARD