Re: [PATCH v7 11/15] xenpm: Print HWP/CPPC parameters
On Thu, Jul 27, 2023 at 09:54:17AM -0400, Jason Andryuk wrote: > On Thu, Jul 27, 2023 at 7:32 AM Anthony PERARD > 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 = _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
Re: [PATCH v7 11/15] xenpm: Print HWP/CPPC parameters
On Thu, Jul 27, 2023 at 7:32 AM Anthony PERARD 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 = _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
Re: [PATCH v7 11/15] xenpm: Print HWP/CPPC parameters
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. > +const xc_cppc_para_t *cppc = _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. Thanks, -- Anthony PERARD
[PATCH v7 11/15] xenpm: Print HWP/CPPC parameters
Print HWP-specific parameters. Some are always present, but others depend on hardware support. Signed-off-by: Jason Andryuk Reviewed-by: Jan Beulich --- 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 v4: Return activity_window from calculate_hwp_activity_window Use blanks instead of _ in output Use MASK_EXTR Check XEN_HWP_DRIVER name since governor is no longer returned s/hwp/cppc v5: Add Jan's Reviewed-by --- tools/misc/xenpm.c | 66 ++ 1 file changed, 66 insertions(+) diff --git a/tools/misc/xenpm.c b/tools/misc/xenpm.c index 21c93386de..3abd99fd20 100644 --- a/tools/misc/xenpm.c +++ b/tools/misc/xenpm.c @@ -708,6 +708,46 @@ void start_gather_func(int argc, char *argv[]) pause(); } +static unsigned int calculate_activity_window(const xc_cppc_para_t *cppc, + const char **units) +{ +unsigned int mantissa = MASK_EXTR(cppc->activity_window, + XEN_CPPC_ACT_WINDOW_MANTISSA_MASK); +unsigned int exponent = MASK_EXTR(cppc->activity_window, + XEN_CPPC_ACT_WINDOW_EXPONENT_MASK); +unsigned int multiplier = 1; +unsigned int i; + +/* + * SDM only states a 0 register is hardware selected, and doesn't mention + * a 0 mantissa with a non-0 exponent. Only special case a 0 register. + */ +if ( cppc->activity_window == 0 ) +{ +*units = "hardware selected"; + +return 0; +} + +if ( exponent >= 6 ) +{ +*units = "s"; +exponent -= 6; +} +else if ( exponent >= 3 ) +{ +*units = "ms"; +exponent -= 3; +} +else +*units = "us"; + +for ( i = 0; i < exponent; i++ ) +multiplier *= 10; + +return mantissa * multiplier; +} + /* print out parameters about cpu frequency */ static void print_cpufreq_para(int cpuid, struct xc_get_cpufreq_para *p_cpufreq) { @@ -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 +{ +const xc_cppc_para_t *cppc = _cpufreq->u.cppc_para; + +printf("cppc variables :\n"); +printf(" hardware limits: lowest [%u] lowest nonlinear [%u]\n", + cppc->lowest, cppc->lowest_nonlinear); +printf(" : nominal [%u] highest [%u]\n", + cppc->nominal, cppc->highest); +printf(" configured limits : min [%u] max [%u] energy perf [%u]\n", + cppc->minimum, cppc->maximum, cppc->energy_perf); + +if ( cppc->features & XEN_SYSCTL_CPPC_FEAT_ACT_WINDOW ) +{ +unsigned int activity_window; +const char *units; + +activity_window = calculate_activity_window(cppc, ); +printf(" : activity_window [%u %s]\n", + activity_window, units); +} + +printf(" : desired [%u%s]\n", + cppc->desired, + cppc->desired ? "" : " hw autonomous"); +} printf("turbo mode : %s\n", p_cpufreq->turbo_enabled ? "enabled" : "disabled or n/a"); -- 2.41.0