arch/arm64/kernel/topology.c | 145 +++++++++++++++++++++++++++++------ drivers/base/arch_topology.c | 8 +- 2 files changed, 127 insertions(+), 26 deletions(-)
Introducing arm64 specific version of arch_freq_get_on_cpu, cashing on existing implementation for FIE and AMUv1 support: the frequency scale factor, updated on each sched tick, serves as a base for retrieving the frequency for a given CPU, representing an average frequency reported between the ticks - thus its accuracy is limited. The changes have been rather lightly (due to some limitations) tested on an FVP model. Note that some small discrepancies have been observed while testing (on the model) and this is currently being investigated, though it should not have any significant impact on the overall results. Relevant discussions: [1] https://lore.kernel.org/all/20240229162520.970986-1-vanshikonda@os.amperecomputing.com/ [2] https://lore.kernel.org/all/7eozim2xnepacnnkzxlbx34hib4otycnbn4dqymfziqou5lw5u@5xzpv3t7sxo3/ [3] https://lore.kernel.org/all/20231212072617.14756-1-lihuisong@huawei.com/ [4] https://lore.kernel.org/lkml/ZIHpd6unkOtYVEqP@e120325.cambridge.arm.com/T/#m4e74cb5a0aaa353c60fedc6cfb95ab7a6e381e3c v6: - delay allocating cpumask for AMU FIE support instead of invalidating the mask upon failure to register cpufreq policy notifications - drop the change to cpufreq core (for cpuinfo_cur_freq) as this one will be sent as a separate change v5: - Fix invalid access to cpumask - Reworked finding reference cpu when getting the freq v4: - dropping seqcount - fixing identifying active cpu within given policy - skipping full dynticks cpus when retrieving the freq - bringing back plugging in arch_freq_get_on_cpu into cpuinfo_cur_freq v3: - dropping changes to cpufreq_verify_current_freq - pulling in changes from Ionela initializing capacity_freq_ref to 0 (thanks for that!) and applying suggestions made by her during last review: - switching to arch_scale_freq_capacity and arch_scale_freq_ref when reversing freq scale factor computation - swapping shift with multiplication - adding time limit for considering last scale update as valid - updating frequency scale factor upon entering idle v2: - Splitting the patches - Adding comment for full dyntick mode - Plugging arch_freq_get_on_cpu into cpufreq_verify_current_freq instead of in show_cpuinfo_cur_freq to allow the framework to stay more in sync with potential freq changes Beata Michalska (3): arm64: amu: Delay allocating cpumask for AMU FIE support arm64: Provide an AMU-based version of arch_freq_get_on_cpu arm64: Update AMU-based frequency scale factor on entering idle Ionela Voinescu (1): arch_topology: init capacity_freq_ref to 0 arch/arm64/kernel/topology.c | 145 +++++++++++++++++++++++++++++------ drivers/base/arch_topology.c | 8 +- 2 files changed, 127 insertions(+), 26 deletions(-) -- 2.25.1
On Mon, Jun 03, 2024 at 09:21:50AM +0100, Beata Michalska wrote: >Introducing arm64 specific version of arch_freq_get_on_cpu, cashing on >existing implementation for FIE and AMUv1 support: the frequency scale >factor, updated on each sched tick, serves as a base for retrieving >the frequency for a given CPU, representing an average frequency >reported between the ticks - thus its accuracy is limited. > >The changes have been rather lightly (due to some limitations) tested on >an FVP model. Note that some small discrepancies have been observed while I've tested these changes (v6) on AmpereOne system and the results look correct. The frequency reported by scaling_cur_freq is as expected for housekeeping cpus, idle as well as isolated cpus. Thanks, Vanshi >testing (on the model) and this is currently being investigated, though it >should not have any significant impact on the overall results. > >Relevant discussions: >[1] https://lore.kernel.org/all/20240229162520.970986-1-vanshikonda@os.amperecomputing.com/ >[2] https://lore.kernel.org/all/7eozim2xnepacnnkzxlbx34hib4otycnbn4dqymfziqou5lw5u@5xzpv3t7sxo3/ >[3] https://lore.kernel.org/all/20231212072617.14756-1-lihuisong@huawei.com/ >[4] https://lore.kernel.org/lkml/ZIHpd6unkOtYVEqP@e120325.cambridge.arm.com/T/#m4e74cb5a0aaa353c60fedc6cfb95ab7a6e381e3c > >v6: > - delay allocating cpumask for AMU FIE support instead of invalidating the mask > upon failure to register cpufreq policy notifications > - drop the change to cpufreq core (for cpuinfo_cur_freq) as this one will be > sent as a separate change > >v5: > - Fix invalid access to cpumask > - Reworked finding reference cpu when getting the freq > >v4: >- dropping seqcount >- fixing identifying active cpu within given policy >- skipping full dynticks cpus when retrieving the freq >- bringing back plugging in arch_freq_get_on_cpu into cpuinfo_cur_freq > >v3: >- dropping changes to cpufreq_verify_current_freq >- pulling in changes from Ionela initializing capacity_freq_ref to 0 > (thanks for that!) and applying suggestions made by her during last review: > - switching to arch_scale_freq_capacity and arch_scale_freq_ref when > reversing freq scale factor computation > - swapping shift with multiplication >- adding time limit for considering last scale update as valid >- updating frequency scale factor upon entering idle > >v2: >- Splitting the patches >- Adding comment for full dyntick mode >- Plugging arch_freq_get_on_cpu into cpufreq_verify_current_freq instead > of in show_cpuinfo_cur_freq to allow the framework to stay more in sync > with potential freq changes > > > >Beata Michalska (3): > arm64: amu: Delay allocating cpumask for AMU FIE support > arm64: Provide an AMU-based version of arch_freq_get_on_cpu > arm64: Update AMU-based frequency scale factor on entering idle > >Ionela Voinescu (1): > arch_topology: init capacity_freq_ref to 0 > > arch/arm64/kernel/topology.c | 145 +++++++++++++++++++++++++++++------ > drivers/base/arch_topology.c | 8 +- > 2 files changed, 127 insertions(+), 26 deletions(-) > >-- >2.25.1 >
On Sat, Jul 13, 2024 at 05:32:43PM -0700, Vanshidhar Konda wrote: > On Mon, Jun 03, 2024 at 09:21:50AM +0100, Beata Michalska wrote: > > Introducing arm64 specific version of arch_freq_get_on_cpu, cashing on > > existing implementation for FIE and AMUv1 support: the frequency scale > > factor, updated on each sched tick, serves as a base for retrieving > > the frequency for a given CPU, representing an average frequency > > reported between the ticks - thus its accuracy is limited. > > > > The changes have been rather lightly (due to some limitations) tested on > > an FVP model. Note that some small discrepancies have been observed while > > I've tested these changes (v6) on AmpereOne system and the results look correct. > The frequency reported by scaling_cur_freq is as expected for housekeeping cpus, > idle as well as isolated cpus. > That's greatly appreciated - thank you for that! This series depends on [1] so once the relevant changes are ready will send an updated to expose the average freq through a relevant sysfs attrib. --- [1] https://lore.kernel.org/all/ZmrB_DqtmVpvG30l@arm.com/ --- Best Regards Beata > Thanks, > Vanshi > > > testing (on the model) and this is currently being investigated, though it > > should not have any significant impact on the overall results. > > > > Relevant discussions: > > [1] https://lore.kernel.org/all/20240229162520.970986-1-vanshikonda@os.amperecomputing.com/ > > [2] https://lore.kernel.org/all/7eozim2xnepacnnkzxlbx34hib4otycnbn4dqymfziqou5lw5u@5xzpv3t7sxo3/ > > [3] https://lore.kernel.org/all/20231212072617.14756-1-lihuisong@huawei.com/ > > [4] https://lore.kernel.org/lkml/ZIHpd6unkOtYVEqP@e120325.cambridge.arm.com/T/#m4e74cb5a0aaa353c60fedc6cfb95ab7a6e381e3c > > > > v6: > > - delay allocating cpumask for AMU FIE support instead of invalidating the mask > > upon failure to register cpufreq policy notifications > > - drop the change to cpufreq core (for cpuinfo_cur_freq) as this one will be > > sent as a separate change > > > > v5: > > - Fix invalid access to cpumask > > - Reworked finding reference cpu when getting the freq > > > > v4: > > - dropping seqcount > > - fixing identifying active cpu within given policy > > - skipping full dynticks cpus when retrieving the freq > > - bringing back plugging in arch_freq_get_on_cpu into cpuinfo_cur_freq > > > > v3: > > - dropping changes to cpufreq_verify_current_freq > > - pulling in changes from Ionela initializing capacity_freq_ref to 0 > > (thanks for that!) and applying suggestions made by her during last review: > > - switching to arch_scale_freq_capacity and arch_scale_freq_ref when > > reversing freq scale factor computation > > - swapping shift with multiplication > > - adding time limit for considering last scale update as valid > > - updating frequency scale factor upon entering idle > > > > v2: > > - Splitting the patches > > - Adding comment for full dyntick mode > > - Plugging arch_freq_get_on_cpu into cpufreq_verify_current_freq instead > > of in show_cpuinfo_cur_freq to allow the framework to stay more in sync > > with potential freq changes > > > > > > > > Beata Michalska (3): > > arm64: amu: Delay allocating cpumask for AMU FIE support > > arm64: Provide an AMU-based version of arch_freq_get_on_cpu > > arm64: Update AMU-based frequency scale factor on entering idle > > > > Ionela Voinescu (1): > > arch_topology: init capacity_freq_ref to 0 > > > > arch/arm64/kernel/topology.c | 145 +++++++++++++++++++++++++++++------ > > drivers/base/arch_topology.c | 8 +- > > 2 files changed, 127 insertions(+), 26 deletions(-) > > > > -- > > 2.25.1 > >
Hi Beata, On Mon, Jun 03, 2024 at 09:21:50AM +0100, Beata Michalska wrote: > Introducing arm64 specific version of arch_freq_get_on_cpu, cashing on > existing implementation for FIE and AMUv1 support: the frequency scale > factor, updated on each sched tick, serves as a base for retrieving > the frequency for a given CPU, representing an average frequency > reported between the ticks - thus its accuracy is limited. > > The changes have been rather lightly (due to some limitations) tested on > an FVP model. Note that some small discrepancies have been observed while > testing (on the model) and this is currently being investigated, though it > should not have any significant impact on the overall results. What's the plan with this series? Are you still investigating those discrepancies or is it good to go? -- Catalin
Hi Catalin, On Mon, Jul 08, 2024 at 06:10:02PM +0100, Catalin Marinas wrote: > Hi Beata, > > On Mon, Jun 03, 2024 at 09:21:50AM +0100, Beata Michalska wrote: > > Introducing arm64 specific version of arch_freq_get_on_cpu, cashing on > > existing implementation for FIE and AMUv1 support: the frequency scale > > factor, updated on each sched tick, serves as a base for retrieving > > the frequency for a given CPU, representing an average frequency > > reported between the ticks - thus its accuracy is limited. > > > > The changes have been rather lightly (due to some limitations) tested on > > an FVP model. Note that some small discrepancies have been observed while > > testing (on the model) and this is currently being investigated, though it > > should not have any significant impact on the overall results. > > What's the plan with this series? Are you still investigating those > discrepancies or is it good to go? > Overall it should be good to go with small caveat: as per discussion [1] we might need to provide new sysfs attribute exposing an average frequency instead of plugging new code under existing cpuinfo_cur_freq. This is to avoid messing up with other archs and make a clean distinction on which attribute provides what information. As such, the arch_freq_get_on_cpu implementation provided within this series [PATCH v6 3/4] will most probably be shifted to a new function. Hopefully will be able to send those changes soon. --- [1] https://lore.kernel.org/all/ZmrB_DqtmVpvG30l@arm.com/ --- BR Beata > -- > Catalin
On 11/07/2024 21:59, Beata Michalska wrote: > Hi Catalin, > > On Mon, Jul 08, 2024 at 06:10:02PM +0100, Catalin Marinas wrote: >> Hi Beata, >> >> On Mon, Jun 03, 2024 at 09:21:50AM +0100, Beata Michalska wrote: >>> Introducing arm64 specific version of arch_freq_get_on_cpu, cashing on >>> existing implementation for FIE and AMUv1 support: the frequency scale >>> factor, updated on each sched tick, serves as a base for retrieving >>> the frequency for a given CPU, representing an average frequency >>> reported between the ticks - thus its accuracy is limited. >>> >>> The changes have been rather lightly (due to some limitations) tested on >>> an FVP model. Note that some small discrepancies have been observed while >>> testing (on the model) and this is currently being investigated, though it >>> should not have any significant impact on the overall results. >> What's the plan with this series? Are you still investigating those >> discrepancies or is it good to go? >> > Overall it should be good to go with small caveat: > as per discussion [1] we might need to provide new sysfs attribute exposing an > average frequency instead of plugging new code under existing cpuinfo_cur_freq. > This is to avoid messing up with other archs and make a clean distinction on > which attribute provides what information. > As such, the arch_freq_get_on_cpu implementation provided within this series > [PATCH v6 3/4] will most probably be shifted to a new function. > > Hopefully will be able to send those changes soon. > > --- > [1] https://lore.kernel.org/all/ZmrB_DqtmVpvG30l@arm.com/ > --- > BR > Beata > >> -- >> Catalin >> Hi Beata, I've recently tested this patchset on a Kunpeng system. It works as expected when reading scaling_cur_freq. The frequency samples are much stabler than what cppc_cpufreq returns. A few minor things. 1. I observed larger errors on idle cpus than busy cpus, though it's just up to 1%. Not sure if this comes from the uncertain time interval between the last tick and entering idle. The shorter averaging interval, the larger error, I supposed. 2. In the current implementation, the resolution of frequency would be: max_freq_khz / SCHED_CAPACITY_SCALE This looks a bit unnecessary to me. It's supposed to get a better resolution if we can do this in arch_freq_get_on_cpu(): freq = delta_cycle_cnt * max_freq_khz / delta_const_cnt which may require caching both current and previous sets of counts in the per-cpu struct amu_cntr_sample. Kind regards, Jie
On Wed, Aug 14, 2024 at 04:05:24PM +0800, Jie Zhan wrote: > > On 11/07/2024 21:59, Beata Michalska wrote: > > Hi Catalin, > > > > On Mon, Jul 08, 2024 at 06:10:02PM +0100, Catalin Marinas wrote: > > > Hi Beata, > > > > > > On Mon, Jun 03, 2024 at 09:21:50AM +0100, Beata Michalska wrote: > > > > Introducing arm64 specific version of arch_freq_get_on_cpu, cashing on > > > > existing implementation for FIE and AMUv1 support: the frequency scale > > > > factor, updated on each sched tick, serves as a base for retrieving > > > > the frequency for a given CPU, representing an average frequency > > > > reported between the ticks - thus its accuracy is limited. > > > > > > > > The changes have been rather lightly (due to some limitations) tested on > > > > an FVP model. Note that some small discrepancies have been observed while > > > > testing (on the model) and this is currently being investigated, though it > > > > should not have any significant impact on the overall results. > > > What's the plan with this series? Are you still investigating those > > > discrepancies or is it good to go? > > > > > Overall it should be good to go with small caveat: > > as per discussion [1] we might need to provide new sysfs attribute exposing an > > average frequency instead of plugging new code under existing cpuinfo_cur_freq. > > This is to avoid messing up with other archs and make a clean distinction on > > which attribute provides what information. > > As such, the arch_freq_get_on_cpu implementation provided within this series > > [PATCH v6 3/4] will most probably be shifted to a new function. > > > > Hopefully will be able to send those changes soon. > > > > --- > > [1] https://lore.kernel.org/all/ZmrB_DqtmVpvG30l@arm.com/ > > --- > > BR > > Beata > > > > > -- > > > Catalin > > > > Hi Beata, Hi Jie, > > I've recently tested this patchset on a Kunpeng system. > It works as expected when reading scaling_cur_freq. > The frequency samples are much stabler than what cppc_cpufreq returns. Thank you for giving it a spin. (and apologies for late reply) > > A few minor things. > > 1. I observed larger errors on idle cpus than busy cpus, though it's just up > to 1%. > Not sure if this comes from the uncertain time interval between the last > tick and entering idle. > The shorter averaging interval, the larger error, I supposed. All right - will look into it. Just for my benefit: that diff is strictly between arch_freq_avg_get_on_cpu and cpufreq_driver->get(policy->cpu) ? > > 2. In the current implementation, the resolution of frequency would be: > max_freq_khz / SCHED_CAPACITY_SCALE > This looks a bit unnecessary to me. > > It's supposed to get a better resolution if we can do this in > arch_freq_get_on_cpu(): > > freq = delta_cycle_cnt * max_freq_khz / delta_const_cnt > > which may require caching both current and previous sets of counts in the > per-cpu struct amu_cntr_sample. > arch_freq_get_on_cpu relies on the frequency scale factor to derive the average frequency. The scale factor is being calculated based on the deltas you have mentioned and arch_max_freq_scale which uses SCHED_CAPACITY_SCALE*2 factor to accommodate for rather low reference frequencies. arch_freq_get_on_cpu just does somewhat reverse computation to that. --- BR Beata > Kind regards, > Jie >
On 26/08/2024 15:24, Beata Michalska wrote: > ... >> Hi Beata, > Hi Jie, >> I've recently tested this patchset on a Kunpeng system. >> It works as expected when reading scaling_cur_freq. >> The frequency samples are much stabler than what cppc_cpufreq returns. > Thank you for giving it a spin. > (and apologies for late reply) >> A few minor things. >> >> 1. I observed larger errors on idle cpus than busy cpus, though it's just up >> to 1%. >> Not sure if this comes from the uncertain time interval between the last >> tick and entering idle. >> The shorter averaging interval, the larger error, I supposed. > All right - will look into it. > Just for my benefit: that diff is strictly between arch_freq_avg_get_on_cpu > and cpufreq_driver->get(policy->cpu) ? I can't say whether it's "strictly" between them or not because driver->get() shows a fluctuating value. On my platform, cppc_cpufreq's driver->get() sometimes shows large errors on busy cpus (as reported by recent emails), but quite accurate on idle cpus (<1%). With this patch, the "error" on idle cpus as mentioned above is typically <1%, hence better in general. >> 2. In the current implementation, the resolution of frequency would be: >> max_freq_khz / SCHED_CAPACITY_SCALE >> This looks a bit unnecessary to me. >> >> It's supposed to get a better resolution if we can do this in >> arch_freq_get_on_cpu(): >> >> freq = delta_cycle_cnt * max_freq_khz / delta_const_cnt >> >> which may require caching both current and previous sets of counts in the >> per-cpu struct amu_cntr_sample. >> > arch_freq_get_on_cpu relies on the frequency scale factor to derive the average > frequency. The scale factor is being calculated based on the deltas you have > mentioned and arch_max_freq_scale which uses SCHED_CAPACITY_SCALE*2 factor to > accommodate for rather low reference frequencies. arch_freq_get_on_cpu just does > somewhat reverse computation to that. Yeah I understood this. arch_freq_get_on_cpu() now returns: freq = (arch_scale_freq_capacity() * arch_scale_freq_ref()) >> SCHED_CAPACITY_SHIFT The frequency resolution is (arch_scale_freq_ref() >> SCHED_CAPACITY_SHIFT), which is equivalent to max_freq_khz / SCHED_CAPACITY_SCALE. If we can directly do freq = delta_cycle_cnt * ref_freq_khz / delta_const_cnt in arch_freq_get_on_cpu(), it's supposed to have a 1KHz resolution. (sorry for the wrong multiplier in the last reply) Just similar to what's done in [1]. It could be more worthwhile to have a good resolution than utilising the arch_topology framework? [1] https://lore.kernel.org/all/20240229162520.970986-2-vanshikonda@os.amperecomputing.com/ > --- > BR > Beata >> Kind regards, >> Jie >>
© 2016 - 2025 Red Hat, Inc.