In `xenpm start` command, the monitor of average frequency shall
not depend on the existence of legacy P-states, so removing "px_cap"
dependancy check.

Signed-off-by: Penny Zheng <Penny.Zheng@amd.com>
---
v3 -> v4:
- new commit
---
 tools/misc/xenpm.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/misc/xenpm.c b/tools/misc/xenpm.c
index 29fffebebd..b823e5c433 100644
--- a/tools/misc/xenpm.c
+++ b/tools/misc/xenpm.c
@@ -539,7 +539,7 @@ static void signal_int_handler(int signo)
                         res / 1000000UL, 100UL * res / (double)sum_px[i]);
             }
         }
-        if ( px_cap && avgfreq[i] )
+        if ( avgfreq[i] )
             printf("  Avg freq\t%d\tKHz\n", avgfreq[i]);
     }
 
-- 
2.34.1

On 14.04.2025 09:40, Penny Zheng wrote:
> In `xenpm start` command, the monitor of average frequency shall
> not depend on the existence of legacy P-states, so removing "px_cap"
> dependancy check.

Well, yes, I agree there. But can you explain to me what the file scope
"avgfreq" is good for? Shouldn't we go farther and tidy things more
thoroughly? Much like show_cpufreq_by_cpuid(), we could call
get_avgfreq_by_cpuid() right before printing. (It escapes me altogether
why start_gather_func() would pre-fill the array. Unlike cxstat_start[]
and pxstat_start[] that's not incrementally or differentially used data.)

Doing that would make yet more obvious that the px_cap part of the
condition was bogus presumably from the very start. (I'm further inclined
to say that this change should also have a Fixes: tag.)

Jan

[Public]

Hi,

> -----Original Message-----
> From: Jan Beulich <jbeulich@suse.com>
> Sent: Wednesday, April 30, 2025 10:42 PM
> To: Penny, Zheng <penny.zheng@amd.com>
> Cc: Huang, Ray <Ray.Huang@amd.com>; Anthony PERARD
> <anthony.perard@vates.tech>; xen-devel@lists.xenproject.org
> Subject: Re: [PATCH v4 14/15] tools/xenpm: remove px_cap dependency check
> for average frequency
>
> On 14.04.2025 09:40, Penny Zheng wrote:
> > In `xenpm start` command, the monitor of average frequency shall not
> > depend on the existence of legacy P-states, so removing "px_cap"
> > dependancy check.
>
> Well, yes, I agree there. But can you explain to me what the file scope "avgfreq" is
> good for? Shouldn't we go farther and tidy things more thoroughly? Much like
> show_cpufreq_by_cpuid(), we could call
> get_avgfreq_by_cpuid() right before printing. (It escapes me altogether why
> start_gather_func() would pre-fill the array. Unlike cxstat_start[] and pxstat_start[]
> that's not incrementally or differentially used data.)
>

Right, I think avgfreq[] shall be just like cxstat_start[] and pxstat_start[], getting pre-filled in
start_gather_func() too, maybe we don't need a avgfreq_start[] to actually record the numbers.
What we achieve there is that in get_messured_perf(), we could let per_cpu(usr_perf_pair, cpu)
save APERF/MPERF value at  start_gather_func() point.

Then, later, in signal_int_handler(), running get_avgfreq_by_cpuid() again could actually let get_messured_perf()
calculate APERF/MPERF incrementation happened between `xenpm start` duration. Right now, we are
calculating incrementation between boot time, or last `xenpm start` command till now

> Doing that would make yet more obvious that the px_cap part of the condition was
> bogus presumably from the very start. (I'm further inclined to say that this change
> should also have a Fixes: tag.)
>
> Jan