Re: [PATCH v2 09/19] xen/pmstat: clean up pmstat.c

[Jan Beulich <jbeulich@xxxxxxxx>]

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.Zheng@xxxxxxx>
> ---
> 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