On 26.03.2025 06:50, Penny Zheng wrote:
> We intend to move the following functions into drivers/acpi/pmstat.c, as they
> are all designed for performance statistic:
"We intend to ..." describes future plans. Yet this is what you're doing in this
very patch.
> - cpufreq_residency_update
> - cpufreq_statistic_reset
> - cpufreq_statistic_update
> - cpufreq_statistic_init
> - cpufreq_statistic_exit
> and moving out acpi_set_pdc_bits(), as it is the handler for sub-hypercall
> XEN_PM_PDC, and shall stay with the other handlers together in
> drivers/cpufreq/cpufreq.c.
> This commit also applies various style corrections while moving these
> functions
Nit - I'd like to remind you of how to (not) word commit messages.
As to what the sentence says - you staying vague leaves unclear which
style violations may have been left in place, for perhaps a good
reason. For example I observe u32 in code being moved.
> Signed-off-by: Penny Zheng <penny.zh...@amd.com>
> ---
> v1 -> v2:
> - new commit
> ---
> xen/drivers/acpi/pmstat.c | 199 ++++++++++++++++++----
> xen/drivers/cpufreq/cpufreq.c | 31 ++++
> xen/drivers/cpufreq/utility.c | 162 ------------------
> xen/include/acpi/cpufreq/processor_perf.h | 2 -
> 4 files changed, 199 insertions(+), 195 deletions(-)
Nit: Considering that code is being moved _into_ pmstat.c, "clean up"
in the subject is somewhat misleading. Maybe "consolidate code into
pmstat.c"?
> --- a/xen/drivers/acpi/pmstat.c
> +++ b/xen/drivers/acpi/pmstat.c
> @@ -43,6 +43,174 @@
>
> DEFINE_PER_CPU_READ_MOSTLY(struct pm_px *, cpufreq_statistic_data);
>
> +DEFINE_PER_CPU(spinlock_t, cpufreq_statistic_lock);
If you really move everything that's statistics-related, then both of
the above items ought to be possible to become static now.
> +/*********************************************************************
> + * Px STATISTIC INFO *
> + *********************************************************************/
> +
> +static void cpufreq_residency_update(unsigned int cpu, uint8_t state)
> +{
> + uint64_t now, total_idle_ns;
> + int64_t delta;
> + struct pm_px *pxpt = per_cpu(cpufreq_statistic_data, cpu);
> +
> + total_idle_ns = get_cpu_idle_time(cpu);
> + now = NOW();
> +
> + delta = (now - pxpt->prev_state_wall) -
> + (total_idle_ns - pxpt->prev_idle_wall);
> +
> + if ( likely(delta >= 0) )
> + pxpt->u.pt[state].residency += delta;
> +
> + pxpt->prev_state_wall = now;
> + pxpt->prev_idle_wall = total_idle_ns;
> +}
> +
> +void cpufreq_statistic_update(unsigned int cpu, uint8_t from, uint8_t to)
> +{
> + struct pm_px *pxpt;
> + const struct processor_pminfo *pmpt = processor_pminfo[cpu];
> + spinlock_t *cpufreq_statistic_lock =
> + &per_cpu(cpufreq_statistic_lock, cpu);
> +
> + spin_lock(cpufreq_statistic_lock);
> +
> + pxpt = per_cpu(cpufreq_statistic_data, cpu);
> + if ( !pxpt || !pmpt ) {
Hmm, you said style corrections are being made, yet the brace here is
misplaced.
> + spin_unlock(cpufreq_statistic_lock);
> + return;
> + }
> +
> + pxpt->u.last = from;
> + pxpt->u.cur = to;
> + pxpt->u.pt[to].count++;
> +
> + cpufreq_residency_update(cpu, from);
> +
> + (*(pxpt->u.trans_pt + from * pmpt->perf.state_count + to))++;
I came across this line the other day, iirc when reviewing you other
series. I find it expremely odd that this doesn't use array notation:
pxpt->u.trans_pt[from * pmpt->perf.state_count + to]++;
Could you please switch to that, unless of course you see an issue with
it?
> + spin_unlock(cpufreq_statistic_lock);
> +}
> +
> +int cpufreq_statistic_init(unsigned int cpu)
> +{
> + uint32_t i, count;
Here any elsewhere - converting to unsigned int would also fall under
style corrections.
> +static void cpufreq_statistic_reset(unsigned int cpu)
> +{
> + uint32_t i, j, count;
> + struct pm_px *pxpt;
> + const struct processor_pminfo *pmpt = processor_pminfo[cpu];
> + spinlock_t *cpufreq_statistic_lock = &per_cpu(cpufreq_statistic_lock,
> cpu);
> +
> + spin_lock(cpufreq_statistic_lock);
> +
> + pxpt = per_cpu(cpufreq_statistic_data, cpu);
> + if ( !pmpt || !pxpt || !pxpt->u.pt || !pxpt->u.trans_pt )
> + {
> + spin_unlock(cpufreq_statistic_lock);
> + return;
> + }
> +
> + count = pmpt->perf.state_count;
> +
> + for ( i = 0; i < count; i++ )
> + {
> + pxpt->u.pt[i].residency = 0;
> + pxpt->u.pt[i].count = 0;
> +
> + for ( j = 0; j < count; j++ )
> + *(pxpt->u.trans_pt + i * count + j) = 0;
Preferrably array notation again, please.
Jan
