RE: [PATCH v3 12/15] xen/x86: implement EPP support for the amd-cppc driver in active mode

["Penny, Zheng" <penny.zheng@xxxxxxx>]

[Public]

Hi,

> -----Original Message-----
> From: Jan Beulich <jbeulich@xxxxxxxx>
> Sent: Friday, March 28, 2025 3:18 PM
> To: Penny, Zheng <penny.zheng@xxxxxxx>
> Cc: Huang, Ray <Ray.Huang@xxxxxxx>; Andrew Cooper
> <andrew.cooper3@xxxxxxxxxx>; Anthony PERARD <anthony.perard@xxxxxxxxxx>;
> Orzel, Michal <Michal.Orzel@xxxxxxx>; Julien Grall <julien@xxxxxxx>; Roger
> Pau Monné <roger.pau@xxxxxxxxxx>; Stefano Stabellini <sstabellini@xxxxxxxxxx>;
> xen-devel@xxxxxxxxxxxxxxxxxxxx
> Subject: Re: [PATCH v3 12/15] xen/x86: implement EPP support for the amd-cppc
> driver in active mode
>
> On 28.03.2025 05:07, Penny, Zheng wrote:
> >> -----Original Message-----
> >> From: Jan Beulich <jbeulich@xxxxxxxx>
> >> Sent: Tuesday, March 25, 2025 6:49 PM
> >>
> >> On 06.03.2025 09:39, Penny Zheng wrote:
>
> >>> +    {
> >>> +        /* Force the epp value to be zero for performance policy */
> >>> +        epp = CPPC_ENERGY_PERF_MAX_PERFORMANCE;
> >>> +        min_perf = max_perf;
> >>> +    }
> >>> +    else if ( policy->policy == CPUFREQ_POLICY_POWERSAVE )
> >>> +        /* Force the epp value to be 0xff for powersave policy */
> >>> +        /*
> >>> +         * If set max_perf = min_perf = lowest_perf, we are putting
> >>> +         * cpu cores in idle.
> >>> +         */
> >>
> >> Nit: Such two successive comments want combining. (Same near the top
> >> of the function, as I notice only now.)
> >>
> >> Furthermore I'm in trouble with interpreting this comment: To me "lowest"
> >> doesn't mean "doing nothing" but "doing things as efficiently in
> >> terms of power use as possible". IOW that's not idle. Yet the comment
> >> reads as if it was meant to be an explanation of why we can't set
> >> max_perf from min_perf here. That is, not matter what's meant to be
> >> said, I think this needs re- wording (and possibly using subjunctive mood).
> >
> > How about:
> > The lowest non-linear perf is equivalent as P2 frequency. Reducing
> > performance below this point does not lead to total energy savings for a 
> > given
> computation (although it reduces momentary power).
> > So we are not suggesting to set max_perf smaller than lowest non-linear 
> > perf, or
> even the lowest perf.
>
> In an abstract way I think I can follow this. In the context of the code being
> commented, however, I'm afraid I still can't make sense of it. Main point 
> being that
> the code commented doesn't use any of the *_perf values. It only sets the 
> "epp"
> local variable. Maybe the point of the comment is to explain why non of the 
> *_perf
> are used here, but I can't read this out of either of the proposed texts.
>

I've checked some internal test suites for CPPC in windows. Maybe setting 
max_perf = nominal_perf
is a fair option for powersave mode

> Jan