| Message ID | 20240124205922.67266-3-jandryuk@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Improve xenpm output under HWP | expand |
On 24.01.2024 21:59, Jason Andryuk wrote: > xenpm get-cpufreq-states currently just prints no output when cpufreq is > disabled or HWP is running. Have it print an appropriate message. The > cpufreq disabled one mirros the cpuidle disabled one. > > cpufreq disabled: > $ xenpm get-cpufreq-states > Either Xen cpufreq is disabled or no valid information is registered! > > Under HWP: > $ xenpm get-cpufreq-states > P-State information not supported. Try get-cpufreq-average or start. > > Also allow xenpm to handle EOPNOTSUPP from the pmstat hypercalls. > EOPNOTSUPP is returned when HWP is active in some cases and allows the > differentiation from cpufreq being disabled. > > Signed-off-by: Jason Andryuk <jandryuk@gmail.com> Largely okay, but a number of cosmetic remarks / nits: > --- a/tools/misc/xenpm.c > +++ b/tools/misc/xenpm.c > @@ -362,7 +362,15 @@ static int show_pxstat_by_cpuid(xc_interface *xc_handle, int cpuid) > > ret = get_pxstat_by_cpuid(xc_handle, cpuid, &pxstatinfo); > if ( ret ) > + { > + if ( ret == -ENODEV ) > + fprintf(stderr, > + "Either Xen cpufreq is disabled or no valid information is registered!\n"); > + else if ( ret == -EOPNOTSUPP ) One too few blanks for indentation. > + fprintf(stderr, > + "P-State information not supported. Try get-cpufreq-average or start.\n"); Especially the "or start" part reads odd without any quotation. > @@ -382,9 +390,11 @@ void pxstat_func(int argc, char *argv[]) > { > /* show pxstates on all cpus */ > int i; > - for ( i = 0; i < max_cpu_nr; i++ ) > - if ( show_pxstat_by_cpuid(xc_handle, i) == -ENODEV ) > + for ( i = 0; i < max_cpu_nr; i++ ) { This file tries to follow hypervisor style, so the brace ought to go on its own line. While there may I ask that you also add the missing blank line (separating declaration from statements)? This then also applies ... > + int ret = show_pxstat_by_cpuid(xc_handle, i); > + if ( ret == -ENODEV || ret == -EOPNOTSUPP ) ... between these two new lines. > break; > + } Hard tab? > @@ -473,7 +483,8 @@ static void signal_int_handler(int signo) > } > } > > - if ( get_pxstat_by_cpuid(xc_handle, 0, NULL) != -ENODEV ) > + ret = get_pxstat_by_cpuid(xc_handle, 0, NULL); > + if ( ret != -ENODEV && ret != -EOPNOTSUPP ) While looking odd, I can see why it wants to be this way. Jan
On Thu, Jan 25, 2024 at 9:31 AM Jan Beulich <jbeulich@suse.com> wrote: > > On 24.01.2024 21:59, Jason Andryuk wrote: > > xenpm get-cpufreq-states currently just prints no output when cpufreq is > > disabled or HWP is running. Have it print an appropriate message. The > > cpufreq disabled one mirros the cpuidle disabled one. > > > > cpufreq disabled: > > $ xenpm get-cpufreq-states > > Either Xen cpufreq is disabled or no valid information is registered! > > > > Under HWP: > > $ xenpm get-cpufreq-states > > P-State information not supported. Try get-cpufreq-average or start. > > > > Also allow xenpm to handle EOPNOTSUPP from the pmstat hypercalls. > > EOPNOTSUPP is returned when HWP is active in some cases and allows the > > differentiation from cpufreq being disabled. > > > > Signed-off-by: Jason Andryuk <jandryuk@gmail.com> > > Largely okay, but a number of cosmetic remarks / nits: Thanks for taking a look. Sorry about the cosmetic issues. All your suggestions are good with me. Thanks, Jason
diff --git a/tools/misc/xenpm.c b/tools/misc/xenpm.c index d982482a3f..79c618590b 100644 --- a/tools/misc/xenpm.c +++ b/tools/misc/xenpm.c @@ -362,7 +362,15 @@ static int show_pxstat_by_cpuid(xc_interface *xc_handle, int cpuid) ret = get_pxstat_by_cpuid(xc_handle, cpuid, &pxstatinfo); if ( ret ) + { + if ( ret == -ENODEV ) + fprintf(stderr, + "Either Xen cpufreq is disabled or no valid information is registered!\n"); + else if ( ret == -EOPNOTSUPP ) + fprintf(stderr, + "P-State information not supported. Try get-cpufreq-average or start.\n"); return ret; + } print_pxstat(cpuid, &pxstatinfo); @@ -382,9 +390,11 @@ void pxstat_func(int argc, char *argv[]) { /* show pxstates on all cpus */ int i; - for ( i = 0; i < max_cpu_nr; i++ ) - if ( show_pxstat_by_cpuid(xc_handle, i) == -ENODEV ) + for ( i = 0; i < max_cpu_nr; i++ ) { + int ret = show_pxstat_by_cpuid(xc_handle, i); + if ( ret == -ENODEV || ret == -EOPNOTSUPP ) break; + } } else show_pxstat_by_cpuid(xc_handle, cpuid); @@ -432,7 +442,7 @@ static uint64_t *sum, *sum_cx, *sum_px; static void signal_int_handler(int signo) { - int i, j, k; + int i, j, k, ret; struct timeval tv; int cx_cap = 0, px_cap = 0; xc_cputopo_t *cputopo = NULL; @@ -473,7 +483,8 @@ static void signal_int_handler(int signo) } } - if ( get_pxstat_by_cpuid(xc_handle, 0, NULL) != -ENODEV ) + ret = get_pxstat_by_cpuid(xc_handle, 0, NULL); + if ( ret != -ENODEV && ret != -EOPNOTSUPP ) { px_cap = 1; for ( i = 0; i < max_cpu_nr; i++ )
xenpm get-cpufreq-states currently just prints no output when cpufreq is disabled or HWP is running. Have it print an appropriate message. The cpufreq disabled one mirros the cpuidle disabled one. cpufreq disabled: $ xenpm get-cpufreq-states Either Xen cpufreq is disabled or no valid information is registered! Under HWP: $ xenpm get-cpufreq-states P-State information not supported. Try get-cpufreq-average or start. Also allow xenpm to handle EOPNOTSUPP from the pmstat hypercalls. EOPNOTSUPP is returned when HWP is active in some cases and allows the differentiation from cpufreq being disabled. Signed-off-by: Jason Andryuk <jandryuk@gmail.com> --- v2: New tools/misc/xenpm.c | 19 +++++++++++++++---- 1 file changed, 15 insertions(+), 4 deletions(-)