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
