Re: [PATCH v2] schedutil: Allow cpufreq requests to be made even when kthread kicked

From: Rafael J. Wysocki
Date: Tue May 22 2018 - 06:49:38 EST


On Tue, May 22, 2018 at 1:38 PM, Viresh Kumar <viresh.kumar@xxxxxxxxxx> wrote:
> On 22-05-18, 13:31, Rafael J. Wysocki wrote:
>> So below is my (compiled-only) version of the $subject patch, obviously based
>> on the Joel's work.
>>
>> Roughly, what it does is to move the fast_switch_enabled path entirely to
>> sugov_update_single() and take the spinlock around sugov_update_commit()
>> in the one-CPU case too.
>>
>> ---
>> kernel/sched/cpufreq_schedutil.c | 57 ++++++++++++++++++++++++++-------------
>> 1 file changed, 38 insertions(+), 19 deletions(-)
>>
>> Index: linux-pm/kernel/sched/cpufreq_schedutil.c
>> ===================================================================
>> --- linux-pm.orig/kernel/sched/cpufreq_schedutil.c
>> +++ linux-pm/kernel/sched/cpufreq_schedutil.c
>> @@ -92,9 +92,6 @@ static bool sugov_should_update_freq(str
>> !cpufreq_can_do_remote_dvfs(sg_policy->policy))
>> return false;
>>
>> - if (sg_policy->work_in_progress)
>> - return false;
>> -
>> if (unlikely(sg_policy->need_freq_update))
>> return true;
>>
>> @@ -103,25 +100,25 @@ static bool sugov_should_update_freq(str
>> return delta_ns >= sg_policy->freq_update_delay_ns;
>> }
>>
>> -static void sugov_update_commit(struct sugov_policy *sg_policy, u64 time,
>> - unsigned int next_freq)
>> +static bool sugov_update_next_freq(struct sugov_policy *sg_policy, u64 time,
>> + unsigned int next_freq)
>> {
>> - struct cpufreq_policy *policy = sg_policy->policy;
>> -
>> if (sg_policy->next_freq == next_freq)
>> - return;
>> + return false;
>>
>> sg_policy->next_freq = next_freq;
>> sg_policy->last_freq_update_time = time;
>>
>> - if (policy->fast_switch_enabled) {
>> - next_freq = cpufreq_driver_fast_switch(policy, next_freq);
>> - if (!next_freq)
>> - return;
>> + return true;
>> +}
>>
>> - policy->cur = next_freq;
>> - trace_cpu_frequency(next_freq, smp_processor_id());
>> - } else {
>> +static void sugov_update_commit(struct sugov_policy *sg_policy, u64 time,
>> + unsigned int next_freq)
>> +{
>> + if (!sugov_update_next_freq(sg_policy, time, next_freq))
>> + return;
>> +
>> + if (!sg_policy->work_in_progress) {
>> sg_policy->work_in_progress = true;
>> irq_work_queue(&sg_policy->irq_work);
>> }
>> @@ -277,6 +274,7 @@ static void sugov_update_single(struct u
>> {
>> struct sugov_cpu *sg_cpu = container_of(hook, struct sugov_cpu, update_util);
>> struct sugov_policy *sg_policy = sg_cpu->sg_policy;
>> + struct cpufreq_policy *policy = sg_policy->policy;
>> unsigned long util, max;
>> unsigned int next_f;
>> bool busy;
>> @@ -307,7 +305,23 @@ static void sugov_update_single(struct u
>> sg_policy->cached_raw_freq = 0;
>> }
>>
>> - sugov_update_commit(sg_policy, time, next_f);
>> + if (policy->fast_switch_enabled) {
>
> Why do you assume that fast switch isn't possible in shared policy
> cases ? It infact is already enabled for few drivers.

OK, so the fast_switch thing needs to be left outside of the spinlock
in the single case only. Fair enough.