linux-kernel - Re: [RFC PATCH] cpufreq: qcom-cpufreq-hw: allow work to be done on other CPU for PREEMPT

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Tue, 21 Mar 2023 11:24:46 +0100
From:   Krzysztof Kozlowski <krzysztof.kozlowski@...aro.org>
To:     Sebastian Andrzej Siewior <bigeasy@...utronix.de>
Cc:     Andy Gross <agross@...nel.org>,
        Bjorn Andersson <andersson@...nel.org>,
        Konrad Dybcio <konrad.dybcio@...aro.org>,
        "Rafael J. Wysocki" <rafael@...nel.org>,
        Viresh Kumar <viresh.kumar@...aro.org>,
        linux-arm-msm@...r.kernel.org, linux-pm@...r.kernel.org,
        linux-kernel@...r.kernel.org, Adrien Thierry <athierry@...hat.com>,
        Brian Masney <bmasney@...hat.com>,
        linux-rt-users@...r.kernel.org
Subject: Re: [RFC PATCH] cpufreq: qcom-cpufreq-hw: allow work to be done on
 other CPU for PREEMPT_RT

On 21/03/2023 11:04, Sebastian Andrzej Siewior wrote:
> On 2023-03-15 17:49:10 [+0100], Krzysztof Kozlowski wrote:
>> Qualcomm cpufreq driver configures interrupts with affinity to each
>> cluster, e.g.  dcvsh-irq-0, dcvsh-irq-4 and dcvsh-irq-7 on SM8250.
>> Triggered interrupt will schedule delayed work, but, since workqueue
>> prefers local CPUs, it might get executed on a CPU dedicated to realtime
>> tasks causing unexpected latencies in realtime workload.
>>
>> Use unbound workqueue for such case.  This might come with performance
>> or energy penalty, e.g. because of cache miss or when other CPU is
>> sleeping.
> 
> I miss the point where it explains that only PREEMPT_RT is affected by
> this.

I assume "realtime tasks" implies this, but I can make it clearer.

> 
>> Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@...aro.org>
>> ---
>>  drivers/cpufreq/qcom-cpufreq-hw.c | 11 ++++++++++-
>>  1 file changed, 10 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/cpufreq/qcom-cpufreq-hw.c b/drivers/cpufreq/qcom-cpufreq-hw.c
>> index 2f581d2d617d..c5ff8d25fabb 100644
>> --- a/drivers/cpufreq/qcom-cpufreq-hw.c
>> +++ b/drivers/cpufreq/qcom-cpufreq-hw.c
>> @@ -390,7 +390,16 @@ static irqreturn_t qcom_lmh_dcvs_handle_irq(int irq, void *data)
>>  
>>  	/* Disable interrupt and enable polling */
>>  	disable_irq_nosync(c_data->throttle_irq);
>> -	schedule_delayed_work(&c_data->throttle_work, 0);
>> +
>> +	/*
>> +	 * Workqueue prefers local CPUs and since interrupts have set affinity,
>> +	 * the work might execute on a CPU dedicated to realtime tasks.
>> +	 */
>> +	if (IS_ENABLED(CONFIG_PREEMPT_RT))
>> +		queue_delayed_work_on(WORK_CPU_UNBOUND, system_unbound_wq,
>> +				      &c_data->throttle_work, 0);
>> +	else
>> +		schedule_delayed_work(&c_data->throttle_work, 0);
> 
> You isolated CPUs and use this on PREEMPT_RT. And this special use-case
> is your reasoning to make this change and let it depend on PREEMPT_RT?
> 
> If you do PREEMPT_RT and you care about latency I would argue that you
> either disable cpufreq and set it to PERFORMANCE so that the highest
> available frequency is set once and not changed afterwards.

The cpufreq is set to performance. It will be changed anyway because
underlying FW notifies through such interrupts about thermal mitigation
happening.

The only other solution is to disable the cpufreq device, e.g. by not
compiling it.

Best regards,
Krzysztof