| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <6c643297-fe13-42c9-820c-2f95db5a63b9@oppo.com>
Date: Mon, 22 Jul 2024 16:26:56 +0800
From: Gaowei Pu <pugaowei@...o.com>
To: Tim Chen <tim.c.chen@...ux.intel.com>, rafael@...nel.org,
viresh.kumar@...aro.org
Cc: linux-pm@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] cpufreq: queue policy->update work to rt thread to reduce
its schedule latency
On 2024/7/22 10:30, Gaowei Pu wrote:
> Hi Tim,
>
> On 2024/7/19 6:03, Tim Chen wrote:
>> On Wed, 2024-07-17 at 14:33 +0800, Gaowei Pu wrote:
>>> Currently we encountered a problem that the cpufreq boost latency
>>> is about 10 milliseconds or worse when we boost through cpufreq QOS request
>>> under high workload scenarios, while the boost latency mainly consumed by
>>> schedule latency of policy->update work.
>>
>> What is the tail latency now after your change?
sorry missed this.
the tail latency now is about within 50 microseconds after my change.
>>
>>>
>>> We should ensure the low schedule latency of cpu frequency limits work
>>> to meet performance and power demands. so queue the policy->update work
>>> to rt thread to reduce its schedule latency.
>>
>> If my understanding is correct, kthread has a default nice
>> value of 0 and is not a rt thread.
>>
>> I think the gain you see is
>> your patch created a dedicated kthread work queue on CPU 0.
>> The work from policy change no longer have to compete time with other
>> requests coming from schedule_work().
> It's not just other requests coming from schedule_work(), also some normal
> cfs tasks running on the same cpu.
>
> In order to not competing time with the above threads, i change the thread
> policy to rt and prio set to 98 to reduce the schedule latency.
>>
>> If the policy change really needs to get ahead
>> of other tasks, I think you need a dedicated
>> workqueue with alloc_workqueue() using WQ_HIGHPRI flag.
> I think the cpufreq boost or limit action should be trigger in time to meet
> performance and power demands. An dedicated workqueue with highpri will be
> better but maybe not good enough because cfs pick or preempt policy is not
> purely based on thread nice value. So i think the final solution is rt thread
> and the policy change work deserves it :)
> thanks.
>>
>> Tim
>>
>>>
>>> Signed-off-by: Gaowei Pu <pugaowei@...o.com>
>>> ---
>>> drivers/cpufreq/cpufreq.c | 24 ++++++++++++++++++------
>>> include/linux/cpufreq.h | 4 +++-
>>> 2 files changed, 21 insertions(+), 7 deletions(-)
>>>
>>> diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
>>> index a45aac17c20f..e6e42a3ba9ab 100644
>>> --- a/drivers/cpufreq/cpufreq.c
>>> +++ b/drivers/cpufreq/cpufreq.c
>>> @@ -1193,7 +1193,7 @@ void refresh_frequency_limits(struct cpufreq_policy *policy)
>>> }
>>> EXPORT_SYMBOL(refresh_frequency_limits);
>>>
>>> -static void handle_update(struct work_struct *work)
>>> +static void handle_update(struct kthread_work *work)
>>> {
>>> struct cpufreq_policy *policy =
>>> container_of(work, struct cpufreq_policy, update);
>>> @@ -1209,7 +1209,7 @@ static int cpufreq_notifier_min(struct notifier_block *nb, unsigned long freq,
>>> {
>>> struct cpufreq_policy *policy = container_of(nb, struct cpufreq_policy, nb_min);
>>>
>>> - schedule_work(&policy->update);
>>> + kthread_queue_work(policy->worker, &policy->update);
>>> return 0;
>>> }
>>>
>>> @@ -1218,7 +1218,7 @@ static int cpufreq_notifier_max(struct notifier_block *nb, unsigned long freq,
>>> {
>>> struct cpufreq_policy *policy = container_of(nb, struct cpufreq_policy, nb_max);
>>>
>>> - schedule_work(&policy->update);
>>> + kthread_queue_work(policy->worker, &policy->update);
>>> return 0;
>>> }
>>>
>>> @@ -1301,15 +1301,25 @@ static struct cpufreq_policy *cpufreq_policy_alloc(unsigned int cpu)
>>> goto err_min_qos_notifier;
>>> }
>>>
>>> + policy->worker = kthread_create_worker_on_cpu(cpu, 0, "policy_worker%d", cpu);
>>> + if (IS_ERR(policy->worker)) {
>>> + dev_err(dev, "Failed to create policy_worker%d\n", cpu);
>>> + goto err_max_qos_notifier;
>>> + }
>>> +
>>> + sched_set_fifo_low(policy->worker->task);
>>> INIT_LIST_HEAD(&policy->policy_list);
>>> init_rwsem(&policy->rwsem);
>>> spin_lock_init(&policy->transition_lock);
>>> init_waitqueue_head(&policy->transition_wait);
>>> - INIT_WORK(&policy->update, handle_update);
>>> + kthread_init_work(&policy->update, handle_update);
>>>
>>> policy->cpu = cpu;
>>> return policy;
>>>
>>> +err_max_qos_notifier:
>>> + freq_qos_remove_notifier(&policy->constraints, FREQ_QOS_MAX,
>>> + &policy->nb_max);
>>> err_min_qos_notifier:
>>> freq_qos_remove_notifier(&policy->constraints, FREQ_QOS_MIN,
>>> &policy->nb_min);
>>> @@ -1353,7 +1363,9 @@ static void cpufreq_policy_free(struct cpufreq_policy *policy)
>>> &policy->nb_min);
>>>
>>> /* Cancel any pending policy->update work before freeing the policy. */
>>> - cancel_work_sync(&policy->update);
>>> + kthread_cancel_work_sync(&policy->update);
>>> + if (policy->worker)
>>> + kthread_destroy_worker(policy->worker);
>>>
>>> if (policy->max_freq_req) {
>>> /*
>>> @@ -1802,7 +1814,7 @@ static unsigned int cpufreq_verify_current_freq(struct cpufreq_policy *policy, b
>>>
>>> cpufreq_out_of_sync(policy, new_freq);
>>> if (update)
>>> - schedule_work(&policy->update);
>>> + kthread_queue_work(policy->worker, &policy->update);
>>> }
>>>
>>> return new_freq;
>>> diff --git a/include/linux/cpufreq.h b/include/linux/cpufreq.h
>>> index 20f7e98ee8af..73029daddfc5 100644
>>> --- a/include/linux/cpufreq.h
>>> +++ b/include/linux/cpufreq.h
>>> @@ -20,6 +20,7 @@
>>> #include <linux/spinlock.h>
>>> #include <linux/sysfs.h>
>>> #include <linux/minmax.h>
>>> +#include <linux/kthread.h>
>>>
>>> /*********************************************************************
>>> * CPUFREQ INTERFACE *
>>> @@ -77,8 +78,9 @@ struct cpufreq_policy {
>>> void *governor_data;
>>> char last_governor[CPUFREQ_NAME_LEN]; /* last governor used */
>>>
>>> - struct work_struct update; /* if update_policy() needs to be
>>> + struct kthread_work update; /* if update_policy() needs to be
>>> * called, but you're in IRQ context */
>>> + struct kthread_worker *worker;
>>>
>>> struct freq_constraints constraints;
>>> struct freq_qos_request *min_freq_req;
>>
>
Powered by blists - more mailing lists