Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.38 as permitted sender) client-ip=23.128.96.38;
MIME-Version: 1.0
References: <20231109101438.1139696-1-vincent.guittot@linaro.org>
 <20231109101438.1139696-2-vincent.guittot@linaro.org> <ZWYM0hn28RHjAalh@arm.com>
 <ZWYOg1gIIgI1qXn7@arm.com>
In-Reply-To: <ZWYOg1gIIgI1qXn7@arm.com>
From:   Vincent Guittot <vincent.guittot@linaro.org>
Date:   Wed, 29 Nov 2023 15:57:08 +0100
Message-ID: <CAKfTPtA2g-rDPBJkoXpcHrz4vcYAivequkLvWUXgwQpqFcZkOQ@mail.gmail.com>
Subject: Re: [PATCH v6 1/7] topology: Add a new arch_scale_freq_reference
To:     Ionela Voinescu <ionela.voinescu@arm.com>
Cc:     linux@armlinux.org.uk, catalin.marinas@arm.com, will@kernel.org,
        paul.walmsley@sifive.com, palmer@dabbelt.com,
        aou@eecs.berkeley.edu, sudeep.holla@arm.com,
        gregkh@linuxfoundation.org, rafael@kernel.org, mingo@redhat.com,
        peterz@infradead.org, juri.lelli@redhat.com,
        dietmar.eggemann@arm.com, rostedt@goodmis.org, bsegall@google.com,
        mgorman@suse.de, bristot@redhat.com, vschneid@redhat.com,
        viresh.kumar@linaro.org, lenb@kernel.org, robert.moore@intel.com,
        lukasz.luba@arm.com, pierre.gondois@arm.com,
        beata.michalska@arm.com, linux-arm-kernel@lists.infradead.org,
        linux-kernel@vger.kernel.org, linux-riscv@lists.infradead.org,
        linux-pm@vger.kernel.org, linux-acpi@vger.kernel.org,
        conor.dooley@microchip.com, suagrfillet@gmail.com,
        ajones@ventanamicro.com, lftan@kernel.org
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

Hi Ionela,

On Tue, 28 Nov 2023 at 17:00, Ionela Voinescu <ionela.voinescu@arm.com> wrote:
>
> On Tuesday 28 Nov 2023 at 15:52:52 (+0000), Ionela Voinescu wrote:
> > Hi Vincent,
> >
> > I have a small request on this patch, which is useful for [1].
> > I'll detail what is needed lower in the code.
> >
> > [1] https://lore.kernel.org/lkml/ZWYDr6JJJzBvsqf0@arm.com/
> >
> > On Thursday 09 Nov 2023 at 11:14:32 (+0100), Vincent Guittot wrote:
> > > @@ -279,13 +278,13 @@ void topology_normalize_cpu_scale(void)
> > >
> > >     capacity_scale = 1;
> > >     for_each_possible_cpu(cpu) {
> > > -           capacity = raw_capacity[cpu] * per_cpu(freq_factor, cpu);
> > > +           capacity = raw_capacity[cpu] * per_cpu(capacity_freq_ref, cpu);
> >
> > The only affected code that I could find is here and below.
> >
> > The above line would have to change to:
> > capacity = raw_capacity[cpu] * per_cpu(capacity_freq_ref, cpu) ?: 1;
> >
> > >             capacity_scale = max(capacity, capacity_scale);
> > >     }
> > >
> > >     pr_debug("cpu_capacity: capacity_scale=%llu\n", capacity_scale);
> > >     for_each_possible_cpu(cpu) {
> > > -           capacity = raw_capacity[cpu] * per_cpu(freq_factor, cpu);
> > > +           capacity = raw_capacity[cpu] * per_cpu(capacity_freq_ref, cpu);
> >
> > and here:
> > capacity = raw_capacity[cpu] * per_cpu(capacity_freq_ref, cpu) ?: 1;
> >
> > I think it's nicer to start with capacity_freq_ref as 0 and compensate here
> > for uninitialized capacity_freq_ref.
> >
> > Let me know if this is alright of if you'd prefer us to make this change
> > in a separate patch.
> >
> > Thanks,
> > Ionela.
> >
>
> Correction - both will need to be:
> capacity = raw_capacity[cpu] * (per_cpu(capacity_freq_ref, cpu) ?: 1);

Yes, this one is better to keep the same behavior when cpufreq driver
is never loaded

I think this change should come as an additional patch either on top
of this series or directly as part of [1] which is the main user of
the change. I assume that's because you want arch_freq_get_on_cpu()
returns 0 until everything has been initialized

Vincent

>
> Thanks,
> Ionela.