| Message ID | 20230726170945.34961-12-jandryuk@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Intel Hardware P-States (HWP) support | expand |
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 = &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. Thanks,
On Thu, Jul 27, 2023 at 7:32 AM Anthony PERARD <anthony.perard@citrix.com> 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
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@citrix.com> 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,
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 = &p_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, &units); + 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");