Re: [PATCH v6 7/7][Resend] cpufreq: schedutil: New governor based on scheduler utilization data
On 03/31/2016 05:32 AM, Rafael J. Wysocki wrote: > On Thu, Mar 31, 2016 at 2:24 PM, Peter Zijlstra wrote: >> On Mon, Mar 28, 2016 at 11:17:44AM -0700, Steve Muckle wrote: >>> The scenario I'm contemplating is that while a CPU-intensive task is >>> running a thermal interrupt goes off. The driver for this thermal >>> interrupt responds by capping fmax. If this happens just after the tick, >>> it seems possible that we could wait a full tick before changing the >>> frequency. Given a 10ms tick it could be rather annoying for thermal >>> management algorithms on some platforms (I'm familiar with a few). >> >> So I'm blissfully unaware of all the thermal stuffs we have; but it >> looks like its somehow bolten onto cpufreq without feedback. >> >> The thing I worry about is thermal scaling the CPU back past where RT/DL >> tasks can still complete in time. It should not be able to do that, or >> rather, missing deadlines because thermal is about as useful as >> rebooting the device. I'd agree that impacting RT/DL activity because of throttling may be as bad as as a reset, but that seems worst case. There could be some graceful shutdown or notification/alarm that can be done. Or a platform can simply choose to reset. Shouldn't we try to give the system designer the option of doing something in software (by throttling the CPUs as low as necessary to continue operation) rather than giving up and relying on a hardware reset? > Right. If thermal throttling kicks in, the game is pretty much over. > > That's why ideas float about taking the thermal constraints into > account upfront, but that's a different discussion entirely. Current mainstream mobile platforms frequently throttle during normal operation. I think it's important to have a robust throttling mechanism at least until the more proactive thermal management scheme is fully developed and proves to be equally capable (if and when that happens). >> I guess I'm saying is, the whole cpufreq/thermal 'interface' needs work >> anyhow. > > Yes, it does. Agreed! thanks, Steve
Re: [PATCH v6 7/7][Resend] cpufreq: schedutil: New governor based on scheduler utilization data
On Thu, Mar 31, 2016 at 2:24 PM, Peter Zijlstra wrote: > On Mon, Mar 28, 2016 at 11:17:44AM -0700, Steve Muckle wrote: >> The scenario I'm contemplating is that while a CPU-intensive task is >> running a thermal interrupt goes off. The driver for this thermal >> interrupt responds by capping fmax. If this happens just after the tick, >> it seems possible that we could wait a full tick before changing the >> frequency. Given a 10ms tick it could be rather annoying for thermal >> management algorithms on some platforms (I'm familiar with a few). > > So I'm blissfully unaware of all the thermal stuffs we have; but it > looks like its somehow bolten onto cpufreq without feedback. > > The thing I worry about is thermal scaling the CPU back past where RT/DL > tasks can still complete in time. It should not be able to do that, or > rather, missing deadlines because thermal is about as useful as > rebooting the device. Right. If thermal throttling kicks in, the game is pretty much over. That's why ideas float about taking the thermal constraints into account upfront, but that's a different discussion entirely. > I guess I'm saying is, the whole cpufreq/thermal 'interface' needs work > anyhow. Yes, it does.
Re: [PATCH v6 7/7][Resend] cpufreq: schedutil: New governor based on scheduler utilization data
On Wed, Mar 30, 2016 at 03:12:40AM +0200, Rafael J. Wysocki wrote: > Except that for_each_cpu_and_not is not defined as of today. > > I guess I can play with cpumasks, but then I'm not sure that will end > up actually more efficient. It will not indeed.
Re: [PATCH v6 7/7][Resend] cpufreq: schedutil: New governor based on scheduler utilization data
On Mon, Mar 28, 2016 at 11:17:44AM -0700, Steve Muckle wrote: > The scenario I'm contemplating is that while a CPU-intensive task is > running a thermal interrupt goes off. The driver for this thermal > interrupt responds by capping fmax. If this happens just after the tick, > it seems possible that we could wait a full tick before changing the > frequency. Given a 10ms tick it could be rather annoying for thermal > management algorithms on some platforms (I'm familiar with a few). So I'm blissfully unaware of all the thermal stuffs we have; but it looks like its somehow bolten onto cpufreq without feedback. The thing I worry about is thermal scaling the CPU back past where RT/DL tasks can still complete in time. It should not be able to do that, or rather, missing deadlines because thermal is about as useful as rebooting the device. I guess I'm saying is, the whole cpufreq/thermal 'interface' needs work anyhow.
Re: [PATCH v6 7/7][Resend] cpufreq: schedutil: New governor based on scheduler utilization data
On 29-03-16, 14:58, Rafael J. Wysocki wrote: > On Monday, March 28, 2016 02:33:33 PM Viresh Kumar wrote: > > Its gonna be same for all policies sharing tunables .. > > The value will be the same, but the cacheline won't. Fair enough. So this information is replicated for each policy for performance benefits. Would it make sense to add a comment for that? Its not obvious today why we are keeping this per-policy. > > > +static ssize_t rate_limit_us_store(struct gov_attr_set *attr_set, const > > > char *buf, > > > +size_t count) > > > +{ > > > + struct sugov_tunables *tunables = to_sugov_tunables(attr_set); > > > + struct sugov_policy *sg_policy; > > > + unsigned int rate_limit_us; > > > + int ret; > > > + > > > + ret = sscanf(buf, "%u", &rate_limit_us); > > > + if (ret != 1) > > > + return -EINVAL; > > > + > > > + tunables->rate_limit_us = rate_limit_us; > > > + > > > + list_for_each_entry(sg_policy, &attr_set->policy_list, tunables_hook) > > > + sg_policy->freq_update_delay_ns = rate_limit_us * NSEC_PER_USEC; > > > + > > > + return count; > > > +} > > > + > > > +static struct governor_attr rate_limit_us = __ATTR_RW(rate_limit_us); > > > > Why not reuse gov_attr_rw() ? > > Would it work? Why wouldn't it? I had a look again at that and I couldn't find a reason for it to not work. Probably I missed something. > > > + ret = kobject_init_and_add(&tunables->attr_set.kobj, > > > &sugov_tunables_ktype, > > > +get_governor_parent_kobj(policy), "%s", > > > +schedutil_gov.name); > > > + if (!ret) > > > + goto out; > > > + > > > + /* Failure, so roll back. */ > > > + policy->governor_data = NULL; > > > + sugov_tunables_free(tunables); > > > + > > > + free_sg_policy: > > > + pr_err("cpufreq: schedutil governor initialization failed (error > > > %d)\n", ret); > > > + sugov_policy_free(sg_policy); > > > > I didn't like the way we have mixed success and failure path here, just to > > save > > a single line of code (unlock). > > I don't follow, sorry. Yes, I can do unlock/return instead of the "goto out", > but then the goto label is still needed. Sorry for not being clear earlier, but this what I was suggesting it to look like: --- static int sugov_init(struct cpufreq_policy *policy) { struct sugov_policy *sg_policy; struct sugov_tunables *tunables; unsigned int lat; int ret = 0; /* State should be equivalent to EXIT */ if (policy->governor_data) return -EBUSY; sg_policy = sugov_policy_alloc(policy); if (!sg_policy) return -ENOMEM; mutex_lock(&global_tunables_lock); if (global_tunables) { if (WARN_ON(have_governor_per_policy())) { ret = -EINVAL; goto free_sg_policy; } policy->governor_data = sg_policy; sg_policy->tunables = global_tunables; gov_attr_set_get(&global_tunables->attr_set, &sg_policy->tunables_hook); mutex_unlock(&global_tunables_lock); return 0; } tunables = sugov_tunables_alloc(sg_policy); if (!tunables) { ret = -ENOMEM; goto free_sg_policy; } tunables->rate_limit_us = LATENCY_MULTIPLIER; lat = policy->cpuinfo.transition_latency / NSEC_PER_USEC; if (lat) tunables->rate_limit_us *= lat; if (!have_governor_per_policy()) global_tunables = tunables; policy->governor_data = sg_policy; sg_policy->tunables = tunables; ret = kobject_init_and_add(&tunables->attr_set.kobj, &sugov_tunables_ktype, get_governor_parent_kobj(policy), "%s", schedutil_gov.name); if (!ret) { mutex_unlock(&global_tunables_lock); return 0; } /* Failure, so roll back. */ policy->governor_data = NULL; sugov_tunables_free(tunables); free_sg_policy: mutex_unlock(&global_tunables_lock); pr_err("cpufreq: schedutil governor initialization failed (error %d)\n", ret); sugov_policy_free(sg_policy); return ret; --- > > Over that it does things, that aren't symmetric anymore. For example, we > > have > > called sugov_policy_alloc() without locks > > Are you sure? Yes. > > and are freeing it from within locks. > > Both are under global_tunables_lock. No, sugov_policy_alloc() isn't called from within locks. > > > +static int __init sugov_module_init(void) > > > +{ > > > + return cpufreq_register_governor(&schedutil_gov); > > > +} > > > + > > > +static void __exit sugov_module_exit(void) > > > +{ > > > + cpufreq_unregister_governor(&schedutil_gov); > > > +} > > > + > > > +MODULE_AUTHOR("Rafael J. Wysocki "); > > > +MODULE_DESCRIPTION("Utilization-based CPU frequency selec
Re: [PATCH v6 7/7][Resend] cpufreq: schedutil: New governor based on scheduler utilization data
On Tue, Mar 29, 2016 at 2:58 PM, Rafael J. Wysocki wrote: > On Monday, March 28, 2016 02:33:33 PM Viresh Kumar wrote: >> On 22-03-16, 02:54, Rafael J. Wysocki wrote: [cut] >> > +static unsigned int sugov_next_freq_shared(struct sugov_policy *sg_policy, >> > + unsigned long util, unsigned long >> > max) >> > +{ >> > + struct cpufreq_policy *policy = sg_policy->policy; >> > + unsigned int max_f = policy->cpuinfo.max_freq; >> > + u64 last_freq_update_time = sg_policy->last_freq_update_time; >> > + unsigned int j; >> > + >> > + if (util == ULONG_MAX) >> > + return max_f; >> > + >> > + for_each_cpu(j, policy->cpus) { >> > + struct sugov_cpu *j_sg_cpu; >> > + unsigned long j_util, j_max; >> > + u64 delta_ns; >> > + >> > + if (j == smp_processor_id()) >> > + continue; >> >> Why skip local CPU completely ? > > Because the original util and max come from it. > >> And if we really want to do that, what about something like >> for_each_cpu_and_not >> to kill the unnecessary if {} statement ? > > That will work. Except that for_each_cpu_and_not is not defined as of today. I guess I can play with cpumasks, but then I'm not sure that will end up actually more efficient.
Re: [PATCH v6 7/7][Resend] cpufreq: schedutil: New governor based on scheduler utilization data
On Monday, March 28, 2016 02:33:33 PM Viresh Kumar wrote: > On 22-03-16, 02:54, Rafael J. Wysocki wrote: > > Index: linux-pm/kernel/sched/cpufreq_schedutil.c > > === > > --- /dev/null > > +++ linux-pm/kernel/sched/cpufreq_schedutil.c > > @@ -0,0 +1,528 @@ > > +/* > > + * CPUFreq governor based on scheduler-provided CPU utilization data. > > + * > > + * Copyright (C) 2016, Intel Corporation > > + * Author: Rafael J. Wysocki > > + * > > + * This program is free software; you can redistribute it and/or modify > > + * it under the terms of the GNU General Public License version 2 as > > + * published by the Free Software Foundation. > > + */ > > + > > +#include > > +#include > > +#include > > +#include > > + > > +#include "sched.h" > > + > > +struct sugov_tunables { > > + struct gov_attr_set attr_set; > > + unsigned int rate_limit_us; > > +}; > > + > > +struct sugov_policy { > > + struct cpufreq_policy *policy; > > + > > + struct sugov_tunables *tunables; > > + struct list_head tunables_hook; > > + > > + raw_spinlock_t update_lock; /* For shared policies */ > > + u64 last_freq_update_time; > > + s64 freq_update_delay_ns; > > And why isn't it part of sugov_tunables? Because it is not a tunable. > Its gonna be same for all policies sharing tunables .. The value will be the same, but the cacheline won't. > > > + unsigned int next_freq; > > + > > + /* The next fields are only needed if fast switch cannot be used. */ > > + struct irq_work irq_work; > > + struct work_struct work; > > + struct mutex work_lock; > > + bool work_in_progress; > > + > > + bool need_freq_update; > > +}; > > + > > +struct sugov_cpu { > > + struct update_util_data update_util; > > + struct sugov_policy *sg_policy; > > + > > + /* The fields below are only needed when sharing a policy. */ > > + unsigned long util; > > + unsigned long max; > > + u64 last_update; > > +}; > > + > > +static DEFINE_PER_CPU(struct sugov_cpu, sugov_cpu); > > + > > +/ Governor internals ***/ > > + > > +static bool sugov_should_update_freq(struct sugov_policy *sg_policy, u64 > > time) > > To make its purpose clear, maybe name it as: sugov_should_reevaluate_freq(), > because we aren't updating the freq just yet, but deciding if we need to > reevaluate again or not. Splitting hairs anyone? > As its going to be called from hotpath, maybe mark it as inline and let > compiler > decide ? The compiler will make it inline if it decides it's worth it anyway. > > +{ > > + u64 delta_ns; > > + > > + if (sg_policy->work_in_progress) > > + return false; > > + > > + if (unlikely(sg_policy->need_freq_update)) { > > + sg_policy->need_freq_update = false; > > + return true; > > + } > > + > > + delta_ns = time - sg_policy->last_freq_update_time; > > + return (s64)delta_ns >= sg_policy->freq_update_delay_ns; > > +} > > + > > +static void sugov_update_commit(struct sugov_policy *sg_policy, u64 time, > > Maybe sugov_update_freq() ? Can you please give up suggesting the names? What's wrong with the original one? Is it confusing in some way or something? > > + unsigned int next_freq) > > +{ > > + struct cpufreq_policy *policy = sg_policy->policy; > > + > > + sg_policy->last_freq_update_time = time; > > + > > + if (policy->fast_switch_enabled) { > > + if (next_freq > policy->max) > > + next_freq = policy->max; > > + else if (next_freq < policy->min) > > + next_freq = policy->min; > > + > > + if (sg_policy->next_freq == next_freq) { > > + trace_cpu_frequency(policy->cur, smp_processor_id()); > > + return; > > + } > > + sg_policy->next_freq = next_freq; > > Why not do all of above stuff as part of else block as well and move it before > the if {} block ? Because the trace_cpu_frequency() is needed only in the fast switch case. > > + next_freq = cpufreq_driver_fast_switch(policy, next_freq); > > + if (next_freq == CPUFREQ_ENTRY_INVALID) > > + return; > > + > > + policy->cur = next_freq; > > + trace_cpu_frequency(next_freq, smp_processor_id()); > > + } else if (sg_policy->next_freq != next_freq) { > > + sg_policy->next_freq = next_freq; > > + sg_policy->work_in_progress = true; > > + irq_work_queue(&sg_policy->irq_work); > > + } > > +} > > + > > +/** > > + * get_next_freq - Compute a new frequency for a given cpufreq policy. > > + * @policy: cpufreq policy object to compute the new frequency for. > > + * @util: Current CPU utilization. > > + * @max: CPU capacity. > > + * > > + * If the utilization is frequency-invariant, choose the new frequency to > > be > > + * proportional to it, that is > > + * > > + * next_freq = C * max_freq * util /
Re: [PATCH v6 7/7][Resend] cpufreq: schedutil: New governor based on scheduler utilization data
On Monday, March 28, 2016 11:17:44 AM Steve Muckle wrote: > On 03/26/2016 06:36 PM, Rafael J. Wysocki wrote: > +static int sugov_limits(struct cpufreq_policy *policy) > >>> +{ > >>> + struct sugov_policy *sg_policy = policy->governor_data; > >>> + > >>> + if (!policy->fast_switch_enabled) { > >>> + mutex_lock(&sg_policy->work_lock); > >>> + > >>> + if (policy->max < policy->cur) > >>> + __cpufreq_driver_target(policy, policy->max, > >>> + CPUFREQ_RELATION_H); > >>> + else if (policy->min > policy->cur) > >>> + __cpufreq_driver_target(policy, policy->min, > >>> + CPUFREQ_RELATION_L); > >>> + > >>> + mutex_unlock(&sg_policy->work_lock); > >>> + } > >>> >> > >>> >> Is the expectation that in the fast_switch_enabled case we should > >>> >> re-evaluate soon enough that an explicit fixup is not required here? > >> > > >> > Yes, it is. > >> > > >>> >> I'm worried as to whether that will always be true given the possible > >>> >> criticality of applying frequency limits (thermal for example). > >> > > >> > The part of the patch below that you cut actually takes care of that: > >> > > >> > sg_policy->need_freq_update = true; > >> > > >> > which causes the rate limit to be ignored essentially, so the > >> > frequency will be changed on the first update from the scheduler. > > The scenario I'm contemplating is that while a CPU-intensive task is > running a thermal interrupt goes off. The driver for this thermal > interrupt responds by capping fmax. If this happens just after the tick, > it seems possible that we could wait a full tick before changing the > frequency. Given a 10ms tick it could be rather annoying for thermal > management algorithms on some platforms (I'm familiar with a few). The thermal driver has to do something like cpufreq_update_policy() then which can only happen in process context. I'm not sure how it is possible to guarantee any latency better than that full tick here anyway. > >> > Which also is why the min/max check is before the sg_policy->next_freq > >> > == next_freq check in sugov_update_commit(). > >> > > >> > I wanted to avoid locking in the fast switch/one CPU per policy case > >> > which otherwise would be necessary just for the handling of this > >> > thing. I'd like to keep it the way it is unless it can be clearly > >> > demonstrated that it really would lead to problems in practice in a > >> > real system. > > > > Besides, even if frequency is updated directly from here in the "fast > > switch" case, that still doesn't guarantee that it will be updated > > immediately, because the task running this code may be preempted and > > only scheduled again in the next cycle. > > > > Not to mention the fact that it may not run on the CPU to be updated, > > so it would need to use something like smp_call_function_single() for > > the update and that would complicate things even more. > > > > Overall, I don't really think that doing the update directly from here > > in the "fast switch" case would improve things much latency-wise and > > it would increase complexity and introduce overhead into the fast > > path. So this really is a tradeoff and the current choice is the > > right one IMO. > > On the desire to avoid locking in the fast switch/one CPU per policy > case, I wondered about whether disabling interrupts in sugov_limits() > would suffice. That's a rarely called function and I was hoping that the > update hook would already have interrupts disabled due to its being > called in scheduler paths that may do raw_spin_lock_irqsave. But I'm not > sure offhand that will always be true. It will. That's why we can use RCU-sched in cpufreq_update_util() etc. > If it isn't though then I'm not > sure what's necessarily stopping say the sched tick calling the hook > while the hook is already in progress from some other path. > > Agreed there would need to be some additional complexity somewhere to > get things running on the correct CPU. > > Anyway I have nothing against deferring this for now. OK Thanks, Rafael
Re: [PATCH v6 7/7][Resend] cpufreq: schedutil: New governor based on scheduler utilization data
On 03/26/2016 06:36 PM, Rafael J. Wysocki wrote: +static int sugov_limits(struct cpufreq_policy *policy) >>> +{ >>> + struct sugov_policy *sg_policy = policy->governor_data; >>> + >>> + if (!policy->fast_switch_enabled) { >>> + mutex_lock(&sg_policy->work_lock); >>> + >>> + if (policy->max < policy->cur) >>> + __cpufreq_driver_target(policy, policy->max, >>> + CPUFREQ_RELATION_H); >>> + else if (policy->min > policy->cur) >>> + __cpufreq_driver_target(policy, policy->min, >>> + CPUFREQ_RELATION_L); >>> + >>> + mutex_unlock(&sg_policy->work_lock); >>> + } >>> >> >>> >> Is the expectation that in the fast_switch_enabled case we should >>> >> re-evaluate soon enough that an explicit fixup is not required here? >> > >> > Yes, it is. >> > >>> >> I'm worried as to whether that will always be true given the possible >>> >> criticality of applying frequency limits (thermal for example). >> > >> > The part of the patch below that you cut actually takes care of that: >> > >> > sg_policy->need_freq_update = true; >> > >> > which causes the rate limit to be ignored essentially, so the >> > frequency will be changed on the first update from the scheduler. The scenario I'm contemplating is that while a CPU-intensive task is running a thermal interrupt goes off. The driver for this thermal interrupt responds by capping fmax. If this happens just after the tick, it seems possible that we could wait a full tick before changing the frequency. Given a 10ms tick it could be rather annoying for thermal management algorithms on some platforms (I'm familiar with a few). >> > Which also is why the min/max check is before the sg_policy->next_freq >> > == next_freq check in sugov_update_commit(). >> > >> > I wanted to avoid locking in the fast switch/one CPU per policy case >> > which otherwise would be necessary just for the handling of this >> > thing. I'd like to keep it the way it is unless it can be clearly >> > demonstrated that it really would lead to problems in practice in a >> > real system. > > Besides, even if frequency is updated directly from here in the "fast > switch" case, that still doesn't guarantee that it will be updated > immediately, because the task running this code may be preempted and > only scheduled again in the next cycle. > > Not to mention the fact that it may not run on the CPU to be updated, > so it would need to use something like smp_call_function_single() for > the update and that would complicate things even more. > > Overall, I don't really think that doing the update directly from here > in the "fast switch" case would improve things much latency-wise and > it would increase complexity and introduce overhead into the fast > path. So this really is a tradeoff and the current choice is the > right one IMO. On the desire to avoid locking in the fast switch/one CPU per policy case, I wondered about whether disabling interrupts in sugov_limits() would suffice. That's a rarely called function and I was hoping that the update hook would already have interrupts disabled due to its being called in scheduler paths that may do raw_spin_lock_irqsave. But I'm not sure offhand that will always be true. If it isn't though then I'm not sure what's necessarily stopping say the sched tick calling the hook while the hook is already in progress from some other path. Agreed there would need to be some additional complexity somewhere to get things running on the correct CPU. Anyway I have nothing against deferring this for now. thanks, Steve
Re: [PATCH v6 7/7][Resend] cpufreq: schedutil: New governor based on scheduler utilization data
On 22-03-16, 02:54, Rafael J. Wysocki wrote: > Index: linux-pm/kernel/sched/cpufreq_schedutil.c > === > --- /dev/null > +++ linux-pm/kernel/sched/cpufreq_schedutil.c > @@ -0,0 +1,528 @@ > +/* > + * CPUFreq governor based on scheduler-provided CPU utilization data. > + * > + * Copyright (C) 2016, Intel Corporation > + * Author: Rafael J. Wysocki > + * > + * This program is free software; you can redistribute it and/or modify > + * it under the terms of the GNU General Public License version 2 as > + * published by the Free Software Foundation. > + */ > + > +#include > +#include > +#include > +#include > + > +#include "sched.h" > + > +struct sugov_tunables { > + struct gov_attr_set attr_set; > + unsigned int rate_limit_us; > +}; > + > +struct sugov_policy { > + struct cpufreq_policy *policy; > + > + struct sugov_tunables *tunables; > + struct list_head tunables_hook; > + > + raw_spinlock_t update_lock; /* For shared policies */ > + u64 last_freq_update_time; > + s64 freq_update_delay_ns; And why isn't it part of sugov_tunables? Its gonna be same for all policies sharing tunables .. > + unsigned int next_freq; > + > + /* The next fields are only needed if fast switch cannot be used. */ > + struct irq_work irq_work; > + struct work_struct work; > + struct mutex work_lock; > + bool work_in_progress; > + > + bool need_freq_update; > +}; > + > +struct sugov_cpu { > + struct update_util_data update_util; > + struct sugov_policy *sg_policy; > + > + /* The fields below are only needed when sharing a policy. */ > + unsigned long util; > + unsigned long max; > + u64 last_update; > +}; > + > +static DEFINE_PER_CPU(struct sugov_cpu, sugov_cpu); > + > +/ Governor internals ***/ > + > +static bool sugov_should_update_freq(struct sugov_policy *sg_policy, u64 > time) To make its purpose clear, maybe name it as: sugov_should_reevaluate_freq(), because we aren't updating the freq just yet, but deciding if we need to reevaluate again or not. As its going to be called from hotpath, maybe mark it as inline and let compiler decide ? > +{ > + u64 delta_ns; > + > + if (sg_policy->work_in_progress) > + return false; > + > + if (unlikely(sg_policy->need_freq_update)) { > + sg_policy->need_freq_update = false; > + return true; > + } > + > + delta_ns = time - sg_policy->last_freq_update_time; > + return (s64)delta_ns >= sg_policy->freq_update_delay_ns; > +} > + > +static void sugov_update_commit(struct sugov_policy *sg_policy, u64 time, Maybe sugov_update_freq() ? > + unsigned int next_freq) > +{ > + struct cpufreq_policy *policy = sg_policy->policy; > + > + sg_policy->last_freq_update_time = time; > + > + if (policy->fast_switch_enabled) { > + if (next_freq > policy->max) > + next_freq = policy->max; > + else if (next_freq < policy->min) > + next_freq = policy->min; > + > + if (sg_policy->next_freq == next_freq) { > + trace_cpu_frequency(policy->cur, smp_processor_id()); > + return; > + } > + sg_policy->next_freq = next_freq; Why not do all of above stuff as part of else block as well and move it before the if {} block ? > + next_freq = cpufreq_driver_fast_switch(policy, next_freq); > + if (next_freq == CPUFREQ_ENTRY_INVALID) > + return; > + > + policy->cur = next_freq; > + trace_cpu_frequency(next_freq, smp_processor_id()); > + } else if (sg_policy->next_freq != next_freq) { > + sg_policy->next_freq = next_freq; > + sg_policy->work_in_progress = true; > + irq_work_queue(&sg_policy->irq_work); > + } > +} > + > +/** > + * get_next_freq - Compute a new frequency for a given cpufreq policy. > + * @policy: cpufreq policy object to compute the new frequency for. > + * @util: Current CPU utilization. > + * @max: CPU capacity. > + * > + * If the utilization is frequency-invariant, choose the new frequency to be > + * proportional to it, that is > + * > + * next_freq = C * max_freq * util / max > + * > + * Otherwise, approximate the would-be frequency-invariant utilization by > + * util_raw * (curr_freq / max_freq) which leads to > + * > + * next_freq = C * curr_freq * util_raw / max > + * > + * Take C = 1.25 for the frequency tipping point at (util / max) = 0.8. > + */ > +static unsigned int get_next_freq(struct cpufreq_policy *policy, > + unsigned long util, unsigned long max) > +{ > + unsigned int freq = arch_scale_freq_invariant() ? > + policy->cpuinfo.max_freq : policy->cur; > + > + return (freq + (freq >> 2)) * util / max;
Re: [PATCH v6 7/7][Resend] cpufreq: schedutil: New governor based on scheduler utilization data
On Sat, Mar 26, 2016 at 3:05 AM, Rafael J. Wysocki wrote: > On Sat, Mar 26, 2016 at 2:12 AM, Steve Muckle wrote: >> Hi Rafael, >> >> On 03/21/2016 06:54 PM, Rafael J. Wysocki wrote: >> ... >>> +config CPU_FREQ_GOV_SCHEDUTIL >>> + tristate "'schedutil' cpufreq policy governor" >>> + depends on CPU_FREQ >>> + select CPU_FREQ_GOV_ATTR_SET >>> + select IRQ_WORK >>> + help >>> + The frequency selection formula used by this governor is analogous >>> + to the one used by 'ondemand', but instead of computing CPU load >>> + as the "non-idle CPU time" to "total CPU time" ratio, it uses CPU >>> + utilization data provided by the scheduler as input. >> >> The formula's changed a bit from ondemand - can the formula description >> in the commit text be repackaged a bit and used here? > > Right, I forgot to update this help text. > > I'll figure out what to do here. > >> ... >>> + >>> +static void sugov_update_commit(struct sugov_policy *sg_policy, u64 time, >>> + unsigned int next_freq) >>> +{ >>> + struct cpufreq_policy *policy = sg_policy->policy; >>> + >>> + sg_policy->last_freq_update_time = time; >>> + >>> + if (policy->fast_switch_enabled) { >>> + if (next_freq > policy->max) >>> + next_freq = policy->max; >>> + else if (next_freq < policy->min) >>> + next_freq = policy->min; >> >> The __cpufreq_driver_target() interface has this capping in it. For >> uniformity should this be pushed into cpufreq_driver_fast_switch()? > > It could, but see below. It should be doable regardless unless I'm overlooking something. Will try. [cut] >> ... >>> +static int sugov_limits(struct cpufreq_policy *policy) >>> +{ >>> + struct sugov_policy *sg_policy = policy->governor_data; >>> + >>> + if (!policy->fast_switch_enabled) { >>> + mutex_lock(&sg_policy->work_lock); >>> + >>> + if (policy->max < policy->cur) >>> + __cpufreq_driver_target(policy, policy->max, >>> + CPUFREQ_RELATION_H); >>> + else if (policy->min > policy->cur) >>> + __cpufreq_driver_target(policy, policy->min, >>> + CPUFREQ_RELATION_L); >>> + >>> + mutex_unlock(&sg_policy->work_lock); >>> + } >> >> Is the expectation that in the fast_switch_enabled case we should >> re-evaluate soon enough that an explicit fixup is not required here? > > Yes, it is. > >> I'm worried as to whether that will always be true given the possible >> criticality of applying frequency limits (thermal for example). > > The part of the patch below that you cut actually takes care of that: > > sg_policy->need_freq_update = true; > > which causes the rate limit to be ignored essentially, so the > frequency will be changed on the first update from the scheduler. > Which also is why the min/max check is before the sg_policy->next_freq > == next_freq check in sugov_update_commit(). > > I wanted to avoid locking in the fast switch/one CPU per policy case > which otherwise would be necessary just for the handling of this > thing. I'd like to keep it the way it is unless it can be clearly > demonstrated that it really would lead to problems in practice in a > real system. Besides, even if frequency is updated directly from here in the "fast switch" case, that still doesn't guarantee that it will be updated immediately, because the task running this code may be preempted and only scheduled again in the next cycle. Not to mention the fact that it may not run on the CPU to be updated, so it would need to use something like smp_call_function_single() for the update and that would complicate things even more. Overall, I don't really think that doing the update directly from here in the "fast switch" case would improve things much latency-wise and it would increase complexity and introduce overhead into the fast path. So this really is a tradeoff and the current choice is the right one IMO. Thanks, Rafael
Re: [PATCH v6 7/7][Resend] cpufreq: schedutil: New governor based on scheduler utilization data
On Sat, Mar 26, 2016 at 2:12 AM, Steve Muckle wrote: > Hi Rafael, > > On 03/21/2016 06:54 PM, Rafael J. Wysocki wrote: > ... >> +config CPU_FREQ_GOV_SCHEDUTIL >> + tristate "'schedutil' cpufreq policy governor" >> + depends on CPU_FREQ >> + select CPU_FREQ_GOV_ATTR_SET >> + select IRQ_WORK >> + help >> + The frequency selection formula used by this governor is analogous >> + to the one used by 'ondemand', but instead of computing CPU load >> + as the "non-idle CPU time" to "total CPU time" ratio, it uses CPU >> + utilization data provided by the scheduler as input. > > The formula's changed a bit from ondemand - can the formula description > in the commit text be repackaged a bit and used here? Right, I forgot to update this help text. I'll figure out what to do here. > ... >> + >> +static void sugov_update_commit(struct sugov_policy *sg_policy, u64 time, >> + unsigned int next_freq) >> +{ >> + struct cpufreq_policy *policy = sg_policy->policy; >> + >> + sg_policy->last_freq_update_time = time; >> + >> + if (policy->fast_switch_enabled) { >> + if (next_freq > policy->max) >> + next_freq = policy->max; >> + else if (next_freq < policy->min) >> + next_freq = policy->min; > > The __cpufreq_driver_target() interface has this capping in it. For > uniformity should this be pushed into cpufreq_driver_fast_switch()? It could, but see below. >> + >> + if (sg_policy->next_freq == next_freq) { >> + trace_cpu_frequency(policy->cur, smp_processor_id()); >> + return; >> + } > > I fear this may bloat traces unnecessarily as there may be long > stretches when a frequency domain is at the same frequency (especially > fmin or fmax). I put it here, because without it powertop reports that the CPU is idle in situations like these. > ... >> +static unsigned int sugov_next_freq_shared(struct sugov_policy *sg_policy, >> +unsigned long util, unsigned long >> max) >> +{ >> + struct cpufreq_policy *policy = sg_policy->policy; >> + unsigned int max_f = policy->cpuinfo.max_freq; >> + u64 last_freq_update_time = sg_policy->last_freq_update_time; >> + unsigned int j; >> + >> + if (util == ULONG_MAX) >> + return max_f; >> + >> + for_each_cpu(j, policy->cpus) { >> + struct sugov_cpu *j_sg_cpu; >> + unsigned long j_util, j_max; >> + u64 delta_ns; >> + >> + if (j == smp_processor_id()) >> + continue; >> + >> + j_sg_cpu = &per_cpu(sugov_cpu, j); >> + /* >> + * If the CPU utilization was last updated before the previous >> + * frequency update and the time elapsed between the last >> update >> + * of the CPU utilization and the last frequency update is long >> + * enough, don't take the CPU into account as it probably is >> + * idle now. >> + */ >> + delta_ns = last_freq_update_time - j_sg_cpu->last_update; >> + if ((s64)delta_ns > TICK_NSEC) > > Why not declare delta_ns as an s64 (also in suguv_should_update_freq) > and avoid the cast? I took this from __update_load_avg(), but it shouldn't matter here. > ... >> +static int sugov_limits(struct cpufreq_policy *policy) >> +{ >> + struct sugov_policy *sg_policy = policy->governor_data; >> + >> + if (!policy->fast_switch_enabled) { >> + mutex_lock(&sg_policy->work_lock); >> + >> + if (policy->max < policy->cur) >> + __cpufreq_driver_target(policy, policy->max, >> + CPUFREQ_RELATION_H); >> + else if (policy->min > policy->cur) >> + __cpufreq_driver_target(policy, policy->min, >> + CPUFREQ_RELATION_L); >> + >> + mutex_unlock(&sg_policy->work_lock); >> + } > > Is the expectation that in the fast_switch_enabled case we should > re-evaluate soon enough that an explicit fixup is not required here? Yes, it is. > I'm worried as to whether that will always be true given the possible > criticality of applying frequency limits (thermal for example). The part of the patch below that you cut actually takes care of that: sg_policy->need_freq_update = true; which causes the rate limit to be ignored essentially, so the frequency will be changed on the first update from the scheduler. Which also is why the min/max check is before the sg_policy->next_freq == next_freq check in sugov_update_commit(). I wanted to avoid locking in the fast switch/one CPU per policy case which otherwise would be necessary just for the handling of this thing. I'd like to keep it the way it is unless it can be clearly demonstrated that it really would lead
Re: [PATCH v6 7/7][Resend] cpufreq: schedutil: New governor based on scheduler utilization data
Hi Rafael, On 03/21/2016 06:54 PM, Rafael J. Wysocki wrote: ... > +config CPU_FREQ_GOV_SCHEDUTIL > + tristate "'schedutil' cpufreq policy governor" > + depends on CPU_FREQ > + select CPU_FREQ_GOV_ATTR_SET > + select IRQ_WORK > + help > + The frequency selection formula used by this governor is analogous > + to the one used by 'ondemand', but instead of computing CPU load > + as the "non-idle CPU time" to "total CPU time" ratio, it uses CPU > + utilization data provided by the scheduler as input. The formula's changed a bit from ondemand - can the formula description in the commit text be repackaged a bit and used here? ... > + > +static void sugov_update_commit(struct sugov_policy *sg_policy, u64 time, > + unsigned int next_freq) > +{ > + struct cpufreq_policy *policy = sg_policy->policy; > + > + sg_policy->last_freq_update_time = time; > + > + if (policy->fast_switch_enabled) { > + if (next_freq > policy->max) > + next_freq = policy->max; > + else if (next_freq < policy->min) > + next_freq = policy->min; The __cpufreq_driver_target() interface has this capping in it. For uniformity should this be pushed into cpufreq_driver_fast_switch()? > + > + if (sg_policy->next_freq == next_freq) { > + trace_cpu_frequency(policy->cur, smp_processor_id()); > + return; > + } I fear this may bloat traces unnecessarily as there may be long stretches when a frequency domain is at the same frequency (especially fmin or fmax). ... > +static unsigned int sugov_next_freq_shared(struct sugov_policy *sg_policy, > +unsigned long util, unsigned long > max) > +{ > + struct cpufreq_policy *policy = sg_policy->policy; > + unsigned int max_f = policy->cpuinfo.max_freq; > + u64 last_freq_update_time = sg_policy->last_freq_update_time; > + unsigned int j; > + > + if (util == ULONG_MAX) > + return max_f; > + > + for_each_cpu(j, policy->cpus) { > + struct sugov_cpu *j_sg_cpu; > + unsigned long j_util, j_max; > + u64 delta_ns; > + > + if (j == smp_processor_id()) > + continue; > + > + j_sg_cpu = &per_cpu(sugov_cpu, j); > + /* > + * If the CPU utilization was last updated before the previous > + * frequency update and the time elapsed between the last update > + * of the CPU utilization and the last frequency update is long > + * enough, don't take the CPU into account as it probably is > + * idle now. > + */ > + delta_ns = last_freq_update_time - j_sg_cpu->last_update; > + if ((s64)delta_ns > TICK_NSEC) Why not declare delta_ns as an s64 (also in suguv_should_update_freq) and avoid the cast? ... > +static int sugov_limits(struct cpufreq_policy *policy) > +{ > + struct sugov_policy *sg_policy = policy->governor_data; > + > + if (!policy->fast_switch_enabled) { > + mutex_lock(&sg_policy->work_lock); > + > + if (policy->max < policy->cur) > + __cpufreq_driver_target(policy, policy->max, > + CPUFREQ_RELATION_H); > + else if (policy->min > policy->cur) > + __cpufreq_driver_target(policy, policy->min, > + CPUFREQ_RELATION_L); > + > + mutex_unlock(&sg_policy->work_lock); > + } Is the expectation that in the fast_switch_enabled case we should re-evaluate soon enough that an explicit fixup is not required here? I'm worried as to whether that will always be true given the possible criticality of applying frequency limits (thermal for example). thanks, Steve
[PATCH v6 7/7][Resend] cpufreq: schedutil: New governor based on scheduler utilization data
From: Rafael J. Wysocki Add a new cpufreq scaling governor, called "schedutil", that uses scheduler-provided CPU utilization information as input for making its decisions. Doing that is possible after commit 34e2c555f3e1 (cpufreq: Add mechanism for registering utilization update callbacks) that introduced cpufreq_update_util() called by the scheduler on utilization changes (from CFS) and RT/DL task status updates. In particular, CPU frequency scaling decisions may be based on the the utilization data passed to cpufreq_update_util() by CFS. The new governor is relatively simple. The frequency selection formula used by it depends on whether or not the utilization is frequency-invariant. In the frequency-invariant case the new CPU frequency is given by next_freq = 1.25 * max_freq * util / max where util and max are the last two arguments of cpufreq_update_util(). In turn, if util is not frequency-invariant, the maximum frequency in the above formula is replaced with the current frequency of the CPU: next_freq = 1.25 * curr_freq * util / max The coefficient 1.25 corresponds to the frequency tipping point at (util / max) = 0.8. All of the computations are carried out in the utilization update handlers provided by the new governor. One of those handlers is used for cpufreq policies shared between multiple CPUs and the other one is for policies with one CPU only (and therefore it doesn't need to use any extra synchronization means). The governor supports fast frequency switching if that is supported by the cpufreq driver in use and possible for the given policy. In the fast switching case, all operations of the governor take place in its utilization update handlers. If fast switching cannot be used, the frequency switch operations are carried out with the help of a work item which only calls __cpufreq_driver_target() (under a mutex) to trigger a frequency update (to a value already computed beforehand in one of the utilization update handlers). Currently, the governor treats all of the RT and DL tasks as "unknown utilization" and sets the frequency to the allowed maximum when updated from the RT or DL sched classes. That heavy-handed approach should be replaced with something more subtle and specifically targeted at RT and DL tasks. The governor shares some tunables management code with the "ondemand" and "conservative" governors and uses some common definitions from cpufreq_governor.h, but apart from that it is stand-alone. Signed-off-by: Rafael J. Wysocki --- Changes from v5: - Fixed sugov_update_commit() to set sg_policy->next_freq properly in the "work item" branch. - Used smp_processor_id() in sugov_irq_work() and restored work_in_progress. Changes from v4: - Use TICK_NSEC in sugov_next_freq_shared(). - Use schedule_work_on() to schedule work items and replace work_in_progress with work_cpu (which is used both for scheduling work items and as a "work in progress" marker). - Rearrange sugov_update_commit() to only check policy->min/max if fast switching is enabled. - Replace util > max checks with util == ULONG_MAX checks to make it clear that they are about a special case (RT/DL). Changes from v3: - The "next frequency" formula based on http://marc.info/?l=linux-acpi&m=145756618321500&w=4 and http://marc.info/?l=linux-kernel&m=145760739700716&w=4 - The governor goes into kernel/sched/ (again). Changes from v2: - The governor goes into drivers/cpufreq/. - The "next frequency" formula has an additional 1.1 factor to allow more util/max values to map onto the top-most frequency in case the distance between that and the previous one is unproportionally small. - sugov_update_commit() traces CPU frequency even if the new one is the same as the previous one (otherwise, if the system is 100% loaded for long enough, powertop starts to report that all CPUs are 100% idle). --- drivers/cpufreq/Kconfig | 26 + kernel/sched/Makefile|1 kernel/sched/cpufreq_schedutil.c | 528 +++ kernel/sched/sched.h |8 4 files changed, 563 insertions(+) Index: linux-pm/drivers/cpufreq/Kconfig === --- linux-pm.orig/drivers/cpufreq/Kconfig +++ linux-pm/drivers/cpufreq/Kconfig @@ -107,6 +107,16 @@ config CPU_FREQ_DEFAULT_GOV_CONSERVATIVE Be aware that not all cpufreq drivers support the conservative governor. If unsure have a look at the help section of the driver. Fallback governor will be the performance governor. + +config CPU_FREQ_DEFAULT_GOV_SCHEDUTIL + bool "schedutil" + select CPU_FREQ_GOV_SCHEDUTIL + select CPU_FREQ_GOV_PERFORMANCE + help + Use the 'schedutil' CPUFreq governor by default. If unsure, + have a look at the help section of that governor. The fallback + governor will be 'performance'. + endchoice config CPU_FREQ_GOV_PERFORMANCE @@ -18