[Public]
Hi,
> -----Original Message-----
> From: Jan Beulich <jbeul...@suse.com>
> Sent: Friday, March 28, 2025 3:18 PM
> To: Penny, Zheng <penny.zh...@amd.com>
> Cc: Huang, Ray <ray.hu...@amd.com>; Andrew Cooper
> <andrew.coop...@citrix.com>; Anthony PERARD <anthony.per...@vates.tech>;
> Orzel, Michal <michal.or...@amd.com>; Julien Grall <jul...@xen.org>; Roger
> Pau Monné <roger....@citrix.com>; Stefano Stabellini <sstabell...@kernel.org>;
> xen-devel@lists.xenproject.org
> 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 <jbeul...@suse.com>
> >> 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
