Hi all,
This is the 3rd version of patch set which tries to address issues which are
due to missing proper information about CPU performance in time.
The issue description:
1. "Cpufreq statistics cover the time when CPUs are in idle states, so they
are not suitable for certain purposes, like thermal control." Rafael [2]
2. Thermal governor Intelligent Power Allocation (IPA) has to estimate power,
for the last period, e.g. 100ms, for each CPU in the Cluster, to grant new
power and set max possible frequency. Currently in some cases it gets big
error, when the frequency of CPU changed in the middle. It is due to the
fact that IPA reads the current frequency for the CPU, not aware of all
other frequencies which were actively (not in idle) used in the last 100ms.
This code focuses on tracking the events of idle entry/exit for each CPU
and combine them with the frequency tracked statistics inside internal
statistics arrays (per-CPU). In the old cpufreq stats we have one shared
statistics array for the policy (all CPUs) and not take into account
periods when each CPU was in idle.
Sometimes the IPA error between old estimation signal and reality is quite
big (>50%).
changelog:
v3:
- moved the core implementation into the cpufreq and not
creating a new framework (as sugested by Rafael)
- updated all function names and APIs
v2 [1]
Regards,
Lukasz Luba
[1] https://lore.kernel.org/all/[email protected]/
[2] https://lore.kernel.org/all/CAJZ5v0gzpfT__EyrVuZSr32ms7-YJZw7qEok0WZECv1iDRRvWA@mail.gmail.com/
Lukasz Luba (5):
cpufreq: stats: Introduce Cpufreq Active Stats
cpuidle: Add Cpufreq Active Stats calls tracking idle entry/exit
thermal: Add interface to cooling devices to handle governor change
thermal: power allocator: Prepare power actors and calm down when not
used
thermal: cpufreq_cooling: Improve power estimation using Cpufreq
Active Stats
MAINTAINERS | 2 +-
drivers/cpufreq/cpufreq_stats.c | 872 ++++++++++++++++++++++++++
drivers/cpuidle/cpuidle.c | 5 +
drivers/thermal/cpufreq_cooling.c | 131 ++++
drivers/thermal/gov_power_allocator.c | 71 +++
include/linux/cpufreq_stats.h | 131 ++++
include/linux/thermal.h | 1 +
7 files changed, 1212 insertions(+), 1 deletion(-)
create mode 100644 include/linux/cpufreq_stats.h
--
2.17.1
The cpufreq_cooling has dedicated APIs for thermal governor called
Intelligent Power Allocation (IPA). IPA needs the CPUs power used by the
devices in the past. Based on this, IPA tries to estimate the power
budget, allocate new budget and split it across cooling devices for the
next period (keeping the system in the thermal envelope). When the input
power estimated value has big error, the whole mechanism does not work
properly. The old power estimation assumes constant CPU frequency during
the whole IPA period (e.g. 100ms). This can cause big error in the power
estimation, especially when SchedUtil governor and Uclamp is used and
frequency is often adjusted to the current need. This can be visible in
Thus, introduce a new mechanism which solves the CPU frequency sampling
problem and missing proper residency. Use Cpufreq Active Stats calculate
the CPU power used for a given IPA period.
Signed-off-by: Lukasz Luba <[email protected]>
---
drivers/thermal/cpufreq_cooling.c | 131 ++++++++++++++++++++++++++++++
1 file changed, 131 insertions(+)
diff --git a/drivers/thermal/cpufreq_cooling.c b/drivers/thermal/cpufreq_cooling.c
index 0bfb8eebd126..a609bd55ed80 100644
--- a/drivers/thermal/cpufreq_cooling.c
+++ b/drivers/thermal/cpufreq_cooling.c
@@ -12,6 +12,7 @@
*/
#include <linux/cpu.h>
#include <linux/cpufreq.h>
+#include <linux/cpufreq_stats.h>
#include <linux/cpu_cooling.h>
#include <linux/device.h>
#include <linux/energy_model.h>
@@ -61,6 +62,7 @@ struct time_in_idle {
* @policy: cpufreq policy.
* @idle_time: idle time stats
* @qos_req: PM QoS contraint to apply
+ * @ast_mon: Cpufreq Active Stats Monitor array of pointers
*
* This structure is required for keeping information of each registered
* cpufreq_cooling_device.
@@ -75,6 +77,9 @@ struct cpufreq_cooling_device {
struct time_in_idle *idle_time;
#endif
struct freq_qos_request qos_req;
+#ifdef CONFIG_CPU_FREQ_STAT
+ struct cpufreq_active_stats_monitor **ast_mon;
+#endif
};
#ifdef CONFIG_THERMAL_GOV_POWER_ALLOCATOR
@@ -124,6 +129,106 @@ static u32 cpu_power_to_freq(struct cpufreq_cooling_device *cpufreq_cdev,
return cpufreq_cdev->em->table[i].frequency;
}
+#ifdef CONFIG_CPU_FREQ_STAT
+static u32 account_cpu_power(struct cpufreq_active_stats_monitor *ast_mon,
+ struct em_perf_domain *em)
+{
+ u64 single_power, residency, total_time;
+ struct cpufreq_active_stats_state *result;
+ u32 power = 0;
+ int i;
+
+ mutex_lock(&ast_mon->lock);
+ result = ast_mon->snapshot.result;
+ total_time = ast_mon->local_period;
+
+ for (i = 0; i < ast_mon->states_count; i++) {
+ residency = result->residency[i];
+ single_power = em->table[i].power * residency;
+ single_power = div64_u64(single_power, total_time);
+ power += (u32)single_power;
+ }
+
+ mutex_unlock(&ast_mon->lock);
+
+ return power;
+}
+
+static u32 get_power_est(struct cpufreq_cooling_device *cdev)
+{
+ int num_cpus, ret, i;
+ u32 total_power = 0;
+
+ num_cpus = cpumask_weight(cdev->policy->related_cpus);
+
+ for (i = 0; i < num_cpus; i++) {
+ ret = cpufreq_active_stats_cpu_update_monitor(cdev->ast_mon[i]);
+ if (ret)
+ return 0;
+
+ total_power += account_cpu_power(cdev->ast_mon[i], cdev->em);
+ }
+
+ return total_power;
+}
+
+static int cpufreq_get_requested_power(struct thermal_cooling_device *cdev,
+ u32 *power)
+{
+ struct cpufreq_cooling_device *cpufreq_cdev = cdev->devdata;
+ struct cpufreq_policy *policy = cpufreq_cdev->policy;
+
+ *power = get_power_est(cpufreq_cdev);
+
+ trace_thermal_power_cpu_get_power(policy->related_cpus, 0, 0, 0,
+ *power);
+
+ return 0;
+}
+
+static void clean_cpu_monitoring(struct cpufreq_cooling_device *cdev)
+{
+ int num_cpus, i;
+
+ if (!cdev->ast_mon)
+ return;
+
+ num_cpus = cpumask_weight(cdev->policy->related_cpus);
+
+ for (i = 0; i < num_cpus; i++)
+ cpufreq_active_stats_cpu_free_monitor(cdev->ast_mon[i++]);
+
+ kfree(cdev->ast_mon);
+ cdev->ast_mon = NULL;
+}
+
+static int setup_cpu_monitoring(struct cpufreq_cooling_device *cdev)
+{
+ int cpu, cpus, i = 0;
+
+ if (cdev->ast_mon)
+ return 0;
+
+ cpus = cpumask_weight(cdev->policy->related_cpus);
+
+ cdev->ast_mon = kcalloc(cpus, sizeof(cdev->ast_mon), GFP_KERNEL);
+ if (!cdev->ast_mon)
+ return -ENOMEM;
+
+ for_each_cpu(cpu, cdev->policy->related_cpus) {
+ cdev->ast_mon[i] = cpufreq_active_stats_setup(cpu);
+ if (IS_ERR_OR_NULL(cdev->ast_mon[i++]))
+ goto cleanup;
+ }
+
+ return 0;
+
+cleanup:
+ clean_cpu_monitoring(cdev);
+ return -EINVAL;
+}
+#else /* !CONFIG_CPU_FREQ_STATS */
+
/**
* get_load() - get load for a cpu
* @cpufreq_cdev: struct cpufreq_cooling_device for the cpu
@@ -184,6 +289,15 @@ static u32 get_dynamic_power(struct cpufreq_cooling_device *cpufreq_cdev,
return (raw_cpu_power * cpufreq_cdev->last_load) / 100;
}
+static void clean_cpu_monitoring(struct cpufreq_cooling_device *cpufreq_cdev)
+{
+}
+
+static int setup_cpu_monitoring(struct cpufreq_cooling_device *cpufreq_cdev)
+{
+ return 0;
+}
+
/**
* cpufreq_get_requested_power() - get the current power
* @cdev: &thermal_cooling_device pointer
@@ -252,6 +366,7 @@ static int cpufreq_get_requested_power(struct thermal_cooling_device *cdev,
return 0;
}
+#endif
/**
* cpufreq_state2power() - convert a cpu cdev state to power consumed
@@ -323,6 +438,20 @@ static int cpufreq_power2state(struct thermal_cooling_device *cdev,
return 0;
}
+static int cpufreq_change_governor(struct thermal_cooling_device *cdev,
+ bool governor_up)
+{
+ struct cpufreq_cooling_device *cpufreq_cdev = cdev->devdata;
+ int ret = 0;
+
+ if (governor_up)
+ ret = setup_cpu_monitoring(cpufreq_cdev);
+ else
+ clean_cpu_monitoring(cpufreq_cdev);
+
+ return ret;
+}
+
static inline bool em_is_sane(struct cpufreq_cooling_device *cpufreq_cdev,
struct em_perf_domain *em) {
struct cpufreq_policy *policy;
@@ -562,6 +691,7 @@ __cpufreq_cooling_register(struct device_node *np,
cooling_ops->get_requested_power = cpufreq_get_requested_power;
cooling_ops->state2power = cpufreq_state2power;
cooling_ops->power2state = cpufreq_power2state;
+ cooling_ops->change_governor = cpufreq_change_governor;
} else
#endif
if (policy->freq_table_sorted == CPUFREQ_TABLE_UNSORTED) {
@@ -686,6 +816,7 @@ void cpufreq_cooling_unregister(struct thermal_cooling_device *cdev)
thermal_cooling_device_unregister(cdev);
freq_qos_remove_request(&cpufreq_cdev->qos_req);
+ clean_cpu_monitoring(cpufreq_cdev);
free_idle_time(cpufreq_cdev);
kfree(cpufreq_cdev);
}
--
2.17.1
The Cpufreq Active Stats tracks and accounts the activity of the CPU
for each performance level. It accounts the real residency, when the CPU
was not idle, at a given performance level. This patch adds needed calls
which provide the CPU idle entry/exit events to the Cpufreq Active Stats.
Signed-off-by: Lukasz Luba <[email protected]>
---
drivers/cpuidle/cpuidle.c | 5 +++++
1 file changed, 5 insertions(+)
diff --git a/drivers/cpuidle/cpuidle.c b/drivers/cpuidle/cpuidle.c
index ef2ea1b12cd8..f19711b95afb 100644
--- a/drivers/cpuidle/cpuidle.c
+++ b/drivers/cpuidle/cpuidle.c
@@ -16,6 +16,7 @@
#include <linux/notifier.h>
#include <linux/pm_qos.h>
#include <linux/cpu.h>
+#include <linux/cpufreq_stats.h>
#include <linux/cpuidle.h>
#include <linux/ktime.h>
#include <linux/hrtimer.h>
@@ -231,6 +232,8 @@ int cpuidle_enter_state(struct cpuidle_device *dev, struct cpuidle_driver *drv,
trace_cpu_idle(index, dev->cpu);
time_start = ns_to_ktime(local_clock());
+ cpufreq_active_stats_cpu_idle_enter(time_start);
+
stop_critical_timings();
if (!(target_state->flags & CPUIDLE_FLAG_RCU_IDLE))
rcu_idle_enter();
@@ -243,6 +246,8 @@ int cpuidle_enter_state(struct cpuidle_device *dev, struct cpuidle_driver *drv,
time_end = ns_to_ktime(local_clock());
trace_cpu_idle(PWR_EVENT_EXIT, dev->cpu);
+ cpufreq_active_stats_cpu_idle_exit(time_end);
+
/* The cpu is no longer idle or about to enter idle. */
sched_idle_set_state(NULL);
--
2.17.1
The cooling devices in thermal zone can support an interface for
preparation to work with thermal governor. They should be properly setup
before the first throttling happens, which means the internal power
tracking mechanism should be ready. When the IPA is not used or thermal
zone is disabled the power tracking can be stopped. Thus, add the code
which handles cooling device proper setup for the operation with IPA.
Signed-off-by: Lukasz Luba <[email protected]>
---
drivers/thermal/gov_power_allocator.c | 71 +++++++++++++++++++++++++++
1 file changed, 71 insertions(+)
diff --git a/drivers/thermal/gov_power_allocator.c b/drivers/thermal/gov_power_allocator.c
index 13e375751d22..678fb544c8af 100644
--- a/drivers/thermal/gov_power_allocator.c
+++ b/drivers/thermal/gov_power_allocator.c
@@ -53,6 +53,8 @@ static inline s64 div_frac(s64 x, s64 y)
* struct power_allocator_params - parameters for the power allocator governor
* @allocated_tzp: whether we have allocated tzp for this thermal zone and
* it needs to be freed on unbind
+ * @actors_ready: set to 1 when power actors are properly setup or set to
+ * -EINVAL when there were errors during preparation
* @err_integral: accumulated error in the PID controller.
* @prev_err: error in the previous iteration of the PID controller.
* Used to calculate the derivative term.
@@ -68,6 +70,7 @@ static inline s64 div_frac(s64 x, s64 y)
*/
struct power_allocator_params {
bool allocated_tzp;
+ int actors_ready;
s64 err_integral;
s32 prev_err;
int trip_switch_on;
@@ -693,9 +696,20 @@ static int power_allocator_bind(struct thermal_zone_device *tz)
static void power_allocator_unbind(struct thermal_zone_device *tz)
{
struct power_allocator_params *params = tz->governor_data;
+ struct thermal_instance *instance;
dev_dbg(&tz->device, "Unbinding from thermal zone %d\n", tz->id);
+ /* Calm down cooling devices and stop monitoring mechanims */
+ list_for_each_entry(instance, &tz->thermal_instances, tz_node) {
+ struct thermal_cooling_device *cdev = instance->cdev;
+
+ if (!cdev_is_power_actor(cdev))
+ continue;
+ if (cdev->ops->change_governor)
+ cdev->ops->change_governor(cdev, false);
+ }
+
if (params->allocated_tzp) {
kfree(tz->tzp);
tz->tzp = NULL;
@@ -705,6 +719,51 @@ static void power_allocator_unbind(struct thermal_zone_device *tz)
tz->governor_data = NULL;
}
+static int prepare_power_actors(struct thermal_zone_device *tz)
+{
+ struct power_allocator_params *params = tz->governor_data;
+ struct thermal_instance *instance;
+ int ret;
+
+ if (params->actors_ready > 0)
+ return 0;
+
+ if (params->actors_ready < 0)
+ return -EINVAL;
+
+ mutex_lock(&tz->lock);
+
+ list_for_each_entry(instance, &tz->thermal_instances, tz_node) {
+ struct thermal_cooling_device *cdev = instance->cdev;
+
+ if (!cdev_is_power_actor(cdev))
+ continue;
+ if (cdev->ops->change_governor) {
+ ret = cdev->ops->change_governor(cdev, true);
+ if (ret)
+ goto clean_up;
+ }
+ }
+
+ mutex_unlock(&tz->lock);
+ params->actors_ready = 1;
+ return 0;
+
+clean_up:
+ list_for_each_entry(instance, &tz->thermal_instances, tz_node) {
+ struct thermal_cooling_device *cdev = instance->cdev;
+
+ if (!cdev_is_power_actor(cdev))
+ continue;
+ if (cdev->ops->change_governor)
+ cdev->ops->change_governor(cdev, false);
+ }
+
+ mutex_unlock(&tz->lock);
+ params->actors_ready = -EINVAL;
+ return -EINVAL;
+}
+
static int power_allocator_throttle(struct thermal_zone_device *tz, int trip)
{
int ret;
@@ -719,6 +778,18 @@ static int power_allocator_throttle(struct thermal_zone_device *tz, int trip)
if (trip != params->trip_max_desired_temperature)
return 0;
+ /*
+ * If we are called for the first time (e.g. after enabling thermal
+ * zone), setup properly power actors
+ */
+ ret = prepare_power_actors(tz);
+ if (ret) {
+ dev_warn_once(&tz->device,
+ "Failed to setup IPA power actors: %d\n",
+ ret);
+ return ret;
+ }
+
ret = tz->ops->get_trip_temp(tz, params->trip_switch_on,
&switch_on_temp);
if (!ret && (tz->temperature < switch_on_temp)) {
--
2.17.1
On 4/26/22 04:11, Viresh Kumar wrote:
> On 06-04-22, 23:08, Lukasz Luba wrote:
>> Hi all,
>>
>> This is the 3rd version of patch set which tries to address issues which are
>> due to missing proper information about CPU performance in time.
>>
>> The issue description:
>> 1. "Cpufreq statistics cover the time when CPUs are in idle states, so they
>> are not suitable for certain purposes, like thermal control." Rafael [2]
>> 2. Thermal governor Intelligent Power Allocation (IPA) has to estimate power,
>> for the last period, e.g. 100ms, for each CPU in the Cluster, to grant new
>> power and set max possible frequency. Currently in some cases it gets big
>> error, when the frequency of CPU changed in the middle. It is due to the
>> fact that IPA reads the current frequency for the CPU, not aware of all
>> other frequencies which were actively (not in idle) used in the last 100ms.
>>
>> This code focuses on tracking the events of idle entry/exit for each CPU
>> and combine them with the frequency tracked statistics inside internal
>> statistics arrays (per-CPU). In the old cpufreq stats we have one shared
>> statistics array for the policy (all CPUs) and not take into account
>> periods when each CPU was in idle.
>>
>> Sometimes the IPA error between old estimation signal and reality is quite
>> big (>50%).
>
> It would have been useful to show how the stats hierarchy looks in userspace
> now.
>
I haven't modify your current cpufreq stats, they are still counting
total time (idle + running) for the given frequency. I think this is
still useful for some userspace tools. These new proposed stats don't
have such sysfs interface to read them. I don't know if userspace would
be interested in this information (the running only time). IIRC Android
uses bpf mechanisms to get this information to the userspace.
On 26-04-22, 08:59, Lukasz Luba wrote:
> :) but I didn't dare to make it sysfs. I don't know if anything in
> user-space would be interested (apart from my test scripts).
Sure, I was talking about hierarchy in debugfs only. Will be useful if
you can show how it looks and what all data is exposed.
--
viresh
On 06-04-22, 23:08, Lukasz Luba wrote:
> Hi all,
>
> This is the 3rd version of patch set which tries to address issues which are
> due to missing proper information about CPU performance in time.
>
> The issue description:
> 1. "Cpufreq statistics cover the time when CPUs are in idle states, so they
> are not suitable for certain purposes, like thermal control." Rafael [2]
> 2. Thermal governor Intelligent Power Allocation (IPA) has to estimate power,
> for the last period, e.g. 100ms, for each CPU in the Cluster, to grant new
> power and set max possible frequency. Currently in some cases it gets big
> error, when the frequency of CPU changed in the middle. It is due to the
> fact that IPA reads the current frequency for the CPU, not aware of all
> other frequencies which were actively (not in idle) used in the last 100ms.
>
> This code focuses on tracking the events of idle entry/exit for each CPU
> and combine them with the frequency tracked statistics inside internal
> statistics arrays (per-CPU). In the old cpufreq stats we have one shared
> statistics array for the policy (all CPUs) and not take into account
> periods when each CPU was in idle.
>
> Sometimes the IPA error between old estimation signal and reality is quite
> big (>50%).
It would have been useful to show how the stats hierarchy looks in userspace
now.
--
viresh
On 4/26/22 09:02, Viresh Kumar wrote:
> On 26-04-22, 08:59, Lukasz Luba wrote:
>> :) but I didn't dare to make it sysfs. I don't know if anything in
>> user-space would be interested (apart from my test scripts).
>
> Sure, I was talking about hierarchy in debugfs only. Will be useful if
> you can show how it looks and what all data is exposed.
>
I've created a new way for sharing such thing. Please check the rendered
notebook at [1]. You can find raw output of that debugfs at cell 9 or
in cell 11 as a dictionary. The residency is in ns. You can also find a
diff from two snapshots for all cpus at cell 16. We randomly use Little
cpus: 0,3,4,5.
At the bottom you can find plots for all cpus, their active residency at
frequencies. Cpu1 and cpu2 are big, cpu2 has been hotplug out so there
is an empty plot (which is good).
BTW, if you are interested in comparison of different input power
estimation mechanism, you can find them here [2]. There are 4 different
power signals. One is real from Juno power/energy meters the rest
is SW estimations of avg power for the 100ms period. As you can see
there in cell 25 plot, the new proposal in this patch set is better
that two previous one used in mainline. The last plot shows real
power signal and the new avg signal. The plot is interactive and
supports 'Box Zoom' on the right (scroll to right to see that toolbox).
Regards,
Lukasz
[1]
https://nbviewer.org/github/lukaszluba-arm/lisa/blob/public_tests/ipa_input_power-debugfs.ipynb
[2]
https://nbviewer.org/github/lukaszluba-arm/lisa/blob/public_tests/ipa_input_power.ipynb
On Tue, 2022-04-26 at 16:01 +0100, Lukasz Luba wrote:
> > I am worried about adding more stuff here.
> >
> > Please, consider getting the stats after interrupts are re-enabled. You may
> > lose
> > some "precision" because of that, but it is probably overall better that
> > adding
> > to idle interrupt latency.
>
> Definitely. I don't need such precision, so later when interrupts are
> re-enabled is OK for me.
Thanks. That is preferable in general: we do not do things with interrupts
disabled unless there is a very good reason to.
>
> This new call might be empty for your x86 kernels, since probably
> you set the CONFIG_CPU_FREQ_STAT.I can add additional config
> so platforms might still have CONFIG_CPU_FREQ_STAT but avoid this
> new feature and additional overhead in idle exit when e.g.
> CONFIG_CPU_FREQ_ACTIVE_STAT is not set.
>
> The x86 platforms won't use IPA governor, so it's reasonable to
> do this way.
>
> Does this sounds good?
I did not thoroughly read your patches so can't comment on the details.
Just pointing that in general idle path is to be considered the critical path,
especially the part before interrupts are re-enabled. Not only on x86,
but on all platforms using cpuidle. This does not mean we can't read more
statistics there, but it does mean that we should be very careful about added
overhead, keep it under control, etc.
Thank you!
On 4/26/22 08:54, Viresh Kumar wrote:
> On 26-04-22, 08:46, Lukasz Luba wrote:
>> I haven't modify your current cpufreq stats, they are still counting
>> total time (idle + running) for the given frequency. I think this is
>> still useful for some userspace tools. These new proposed stats don't
>> have such sysfs interface to read them. I don't know if userspace would
>> be interested in this information (the running only time). IIRC Android
>> uses bpf mechanisms to get this information to the userspace.
>
> I saw some debugfs bits there, aren't you exposing any data via it ? I
> am just asking about, not suggesting :)
>
:) but I didn't dare to make it sysfs. I don't know if anything in
user-space would be interested (apart from my test scripts).
Hi Lukasz,
On Wed, 2022-04-06 at 23:08 +0100, Lukasz Luba wrote:
> @@ -231,6 +232,8 @@ int cpuidle_enter_state(struct cpuidle_device *dev, struct
> cpuidle_driver *drv,
> trace_cpu_idle(index, dev->cpu);
> time_start = ns_to_ktime(local_clock());
>
> + cpufreq_active_stats_cpu_idle_enter(time_start);
> +
> stop_critical_timings();
> if (!(target_state->flags & CPUIDLE_FLAG_RCU_IDLE))
> rcu_idle_enter();
> @@ -243,6 +246,8 @@ int cpuidle_enter_state(struct cpuidle_device *dev, struct
> cpuidle_driver *drv,
> time_end = ns_to_ktime(local_clock());
> trace_cpu_idle(PWR_EVENT_EXIT, dev->cpu);
>
> + cpufreq_active_stats_cpu_idle_exit(time_end);
> +
At this point the interrupts are still disabled, and they get enabled later. So
the more code you add here and the longer it executes, the longer you delay the
interrupts. Therefore, you are effectively increasing IRQ latency from idle by
adding more code here.
How much? I do not know, depends on how much code you need to execute. But the
amount of code in functions like this tends to increase over time.
So the risk is that we'll keep making 'cpufreq_active_stats_cpu_idle_exit()',
and (may be unintentionally) increase idle interrupt latency.
This is not ideal.
We use the 'wult' tool (https://github.com/intel/wult) to measure C-states
latency and interrupt latency on Intel platforms, and for fast C-states like
Intel C1, we can see that even the current code between C-state exit and
interrupt re-enabled adds measurable overhead.
I am worried about adding more stuff here.
Please, consider getting the stats after interrupts are re-enabled. You may lose
some "precision" because of that, but it is probably overall better that adding
to idle interrupt latency.
> /* The cpu is no longer idle or about to enter idle. */
> sched_idle_set_state(NULL);
On 26-04-22, 08:46, Lukasz Luba wrote:
> I haven't modify your current cpufreq stats, they are still counting
> total time (idle + running) for the given frequency. I think this is
> still useful for some userspace tools. These new proposed stats don't
> have such sysfs interface to read them. I don't know if userspace would
> be interested in this information (the running only time). IIRC Android
> uses bpf mechanisms to get this information to the userspace.
I saw some debugfs bits there, aren't you exposing any data via it ? I
am just asking about, not suggesting :)
--
viresh
Hi Artem,
Thanks for comments!
On 4/26/22 13:05, Artem Bityutskiy wrote:
> Hi Lukasz,
>
> On Wed, 2022-04-06 at 23:08 +0100, Lukasz Luba wrote:
>> @@ -231,6 +232,8 @@ int cpuidle_enter_state(struct cpuidle_device *dev, struct
>> cpuidle_driver *drv,
>> trace_cpu_idle(index, dev->cpu);
>> time_start = ns_to_ktime(local_clock());
>>
>> + cpufreq_active_stats_cpu_idle_enter(time_start);
>> +
>> stop_critical_timings();
>> if (!(target_state->flags & CPUIDLE_FLAG_RCU_IDLE))
>> rcu_idle_enter();
>> @@ -243,6 +246,8 @@ int cpuidle_enter_state(struct cpuidle_device *dev, struct
>> cpuidle_driver *drv,
>> time_end = ns_to_ktime(local_clock());
>> trace_cpu_idle(PWR_EVENT_EXIT, dev->cpu);
>>
>> + cpufreq_active_stats_cpu_idle_exit(time_end);
>> +
>
> At this point the interrupts are still disabled, and they get enabled later. So
> the more code you add here and the longer it executes, the longer you delay the
> interrupts. Therefore, you are effectively increasing IRQ latency from idle by
> adding more code here.
Good point, I've added it just below the trace_cpu_idle().
>
> How much? I do not know, depends on how much code you need to execute. But the
> amount of code in functions like this tends to increase over time.
>
> So the risk is that we'll keep making 'cpufreq_active_stats_cpu_idle_exit()',
> and (may be unintentionally) increase idle interrupt latency.
>
> This is not ideal.
I agree, I will try to find a better place to put this call.
>
> We use the 'wult' tool (https://github.com/intel/wult) to measure C-states
> latency and interrupt latency on Intel platforms, and for fast C-states like
> Intel C1, we can see that even the current code between C-state exit and
> interrupt re-enabled adds measurable overhead.
Thanks for the hint and the link. I'll check that tool. I don't know if
that would work with my platforms. I might create some tests for this
latency measurements.
>
> I am worried about adding more stuff here.
>
> Please, consider getting the stats after interrupts are re-enabled. You may lose
> some "precision" because of that, but it is probably overall better that adding
> to idle interrupt latency.
Definitely. I don't need such precision, so later when interrupts are
re-enabled is OK for me.
>
>> /* The cpu is no longer idle or about to enter idle. */
>> sched_idle_set_state(NULL);
>
>
This new call might be empty for your x86 kernels, since probably
you set the CONFIG_CPU_FREQ_STAT.I can add additional config
so platforms might still have CONFIG_CPU_FREQ_STAT but avoid this
new feature and additional overhead in idle exit when e.g.
CONFIG_CPU_FREQ_ACTIVE_STAT is not set.
The x86 platforms won't use IPA governor, so it's reasonable to
do this way.
Does this sounds good?
Regards,
Lukasz
On 4/26/22 17:29, Artem Bityutskiy wrote:
> On Tue, 2022-04-26 at 16:01 +0100, Lukasz Luba wrote:
>>> I am worried about adding more stuff here.
>>>
>>> Please, consider getting the stats after interrupts are re-enabled. You may
>>> lose
>>> some "precision" because of that, but it is probably overall better that
>>> adding
>>> to idle interrupt latency.
>>
>> Definitely. I don't need such precision, so later when interrupts are
>> re-enabled is OK for me.
>
> Thanks. That is preferable in general: we do not do things with interrupts
> disabled unless there is a very good reason to.
>
>>
>> This new call might be empty for your x86 kernels, since probably
>> you set the CONFIG_CPU_FREQ_STAT.I can add additional config
>> so platforms might still have CONFIG_CPU_FREQ_STAT but avoid this
>> new feature and additional overhead in idle exit when e.g.
>> CONFIG_CPU_FREQ_ACTIVE_STAT is not set.
>>
>> The x86 platforms won't use IPA governor, so it's reasonable to
>> do this way.
>>
>> Does this sounds good?
>
> I did not thoroughly read your patches so can't comment on the details.
>
> Just pointing that in general idle path is to be considered the critical path,
> especially the part before interrupts are re-enabled. Not only on x86,
> but on all platforms using cpuidle. This does not mean we can't read more
> statistics there, but it does mean that we should be very careful about added
> overhead, keep it under control, etc.
I totally agree with you. I didn't know that the interrupts were not
enabled at that moment. I'll address it.
Regards,
Lukasz