linux-rt-users.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [RFC PATCH] cpufreq: qcom-cpufreq-hw: allow work to be done on other CPU for PREEMPT_RT
@ 2023-03-15 16:49 Krzysztof Kozlowski
  2023-03-16 12:28 ` Krzysztof Kozlowski
                   ` (2 more replies)
  0 siblings, 3 replies; 10+ messages in thread
From: Krzysztof Kozlowski @ 2023-03-15 16:49 UTC (permalink / raw)
  To: Andy Gross, Bjorn Andersson, Konrad Dybcio, Rafael J. Wysocki,
	Viresh Kumar, linux-arm-msm, linux-pm, linux-kernel
  Cc: Adrien Thierry, Brian Masney, linux-rt-users, Krzysztof Kozlowski

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.

Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.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);
 
 	if (qcom_cpufreq.soc_data->reg_intr_clr)
 		writel_relaxed(GT_IRQ_STATUS,
-- 
2.34.1


^ permalink raw reply related	[flat|nested] 10+ messages in thread

* Re: [RFC PATCH] cpufreq: qcom-cpufreq-hw: allow work to be done on other CPU for PREEMPT_RT
  2023-03-15 16:49 [RFC PATCH] cpufreq: qcom-cpufreq-hw: allow work to be done on other CPU for PREEMPT_RT Krzysztof Kozlowski
@ 2023-03-16 12:28 ` Krzysztof Kozlowski
       [not found] ` <20230316235705.2235-1-hdanton@sina.com>
  2023-03-21 10:04 ` Sebastian Andrzej Siewior
  2 siblings, 0 replies; 10+ messages in thread
From: Krzysztof Kozlowski @ 2023-03-16 12:28 UTC (permalink / raw)
  To: Andy Gross, Bjorn Andersson, Konrad Dybcio, Rafael J. Wysocki,
	Viresh Kumar, linux-arm-msm, linux-pm, linux-kernel
  Cc: Adrien Thierry, Brian Masney, linux-rt-users


On 15/03/2023 17:49, 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.
> 
> Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
> ---
>  drivers/cpufreq/qcom-cpufreq-hw.c | 11 ++++++++++-

Let me also paste impact of this patch - rtla osnoise on entirely idle
system (cores 2-7 isolated for Realtime):

BEFORE:
       osnoise/7-2967    [007] d..h2..  3937.898311: irq_noise: dcvsh-irq-7:179 start 3937.898310871 duration 104 ns
 irq/179-dcvsh-i-343     [007] d..h2..  3937.898318: irq_noise: IPI:6 start 3937.898317537 duration 104 ns
 irq/179-dcvsh-i-343     [007] d...3..  3937.898321: thread_noise: irq/179-dcvsh-i:343 start 3937.898316287 duration 4740 ns
     kworker/7:0-85      [007] d..h2..  3937.898323: irq_noise: IPI:6 start 3937.898322381 duration 104 ns
     kworker/7:0-85      [007] d...3..  3937.898343: thread_noise: kworker/7:0:85 start 3937.898321339 duration 20990 ns
       osnoise/7-2967    [007] .......  3937.898343: sample_threshold: start 3937.898308475 duration 34531 ns interference 5

Noise duration: 34 us

AFTER:
       osnoise/7-2637    [007] d..h2..   530.563819: irq_noise: dcvsh-irq-7:178 start 530.563817139 duration 260 ns
       osnoise/7-2637    [007] d..h2..   530.563827: irq_noise: IPI:6 start 530.563826670 duration 156 ns
       osnoise/7-2637    [007] .......   530.563828: sample_threshold: start 530.563814587 duration 12864 ns interference 2

Noise duration: 13 us

Best regards,
Krzysztof


^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [RFC PATCH] cpufreq: qcom-cpufreq-hw: allow work to be done on other CPU for PREEMPT_RT
       [not found] ` <20230316235705.2235-1-hdanton@sina.com>
@ 2023-03-17  8:13   ` Krzysztof Kozlowski
  0 siblings, 0 replies; 10+ messages in thread
From: Krzysztof Kozlowski @ 2023-03-17  8:13 UTC (permalink / raw)
  To: Hillf Danton
  Cc: Andy Gross, Bjorn Andersson, Konrad Dybcio, Rafael J. Wysocki,
	Viresh Kumar, linux-arm-msm, linux-pm, linux-kernel,
	Adrien Thierry, Brian Masney, linux-rt-users

On 17/03/2023 00:57, Hillf Danton wrote:
> On 16 Mar 2023 13:28:18 +0100 Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
>> On 15/03/2023 17:49, 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.
>>>
>>> Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
>>> ---
>>>  drivers/cpufreq/qcom-cpufreq-hw.c | 11 ++++++++++-
>>
>> Let me also paste impact of this patch - rtla osnoise on entirely idle
>> system (cores 2-7 isolated for Realtime):
> 
> Are cores 2-7 the non-housekeeping CPUs[1]?

Yes, since their are isolated for Realtime this is synonymous.

> 
> [1] https://lore.kernel.org/lkml/20230223150624.GA29739@lst.de/



Best regards,
Krzysztof


^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [RFC PATCH] cpufreq: qcom-cpufreq-hw: allow work to be done on other CPU for PREEMPT_RT
  2023-03-15 16:49 [RFC PATCH] cpufreq: qcom-cpufreq-hw: allow work to be done on other CPU for PREEMPT_RT Krzysztof Kozlowski
  2023-03-16 12:28 ` Krzysztof Kozlowski
       [not found] ` <20230316235705.2235-1-hdanton@sina.com>
@ 2023-03-21 10:04 ` Sebastian Andrzej Siewior
  2023-03-21 10:24   ` Krzysztof Kozlowski
  2 siblings, 1 reply; 10+ messages in thread
From: Sebastian Andrzej Siewior @ 2023-03-21 10:04 UTC (permalink / raw)
  To: Krzysztof Kozlowski
  Cc: Andy Gross, Bjorn Andersson, Konrad Dybcio, Rafael J. Wysocki,
	Viresh Kumar, linux-arm-msm, linux-pm, linux-kernel,
	Adrien Thierry, Brian Masney, linux-rt-users

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.

> Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.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.

>  	if (qcom_cpufreq.soc_data->reg_intr_clr)
>  		writel_relaxed(GT_IRQ_STATUS,
> -- 
> 2.34.1
> 

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [RFC PATCH] cpufreq: qcom-cpufreq-hw: allow work to be done on other CPU for PREEMPT_RT
  2023-03-21 10:04 ` Sebastian Andrzej Siewior
@ 2023-03-21 10:24   ` Krzysztof Kozlowski
  2023-03-21 10:57     ` Sebastian Andrzej Siewior
  0 siblings, 1 reply; 10+ messages in thread
From: Krzysztof Kozlowski @ 2023-03-21 10:24 UTC (permalink / raw)
  To: Sebastian Andrzej Siewior
  Cc: Andy Gross, Bjorn Andersson, Konrad Dybcio, Rafael J. Wysocki,
	Viresh Kumar, linux-arm-msm, linux-pm, linux-kernel,
	Adrien Thierry, Brian Masney, linux-rt-users

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@linaro.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


^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [RFC PATCH] cpufreq: qcom-cpufreq-hw: allow work to be done on other CPU for PREEMPT_RT
  2023-03-21 10:24   ` Krzysztof Kozlowski
@ 2023-03-21 10:57     ` Sebastian Andrzej Siewior
  2023-03-21 11:27       ` Krzysztof Kozlowski
  0 siblings, 1 reply; 10+ messages in thread
From: Sebastian Andrzej Siewior @ 2023-03-21 10:57 UTC (permalink / raw)
  To: Krzysztof Kozlowski
  Cc: Andy Gross, Bjorn Andersson, Konrad Dybcio, Rafael J. Wysocki,
	Viresh Kumar, linux-arm-msm, linux-pm, linux-kernel,
	Adrien Thierry, Brian Masney, linux-rt-users

On 2023-03-21 11:24:46 [+0100], Krzysztof Kozlowski wrote:
> >> --- 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.

I still fail to understand why this is PREEMPT_RT specific and not a
problem in general when it comes not NO_HZ_FULL and/ or CPU isolation.
However the thermal notifications have nothing to do with cpufreq.

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

People often disable cpufreq because _usually_ the system boots at
maximum performance. There are however exceptions and even x86 system
are configured sometimes to a lower clock speed by the firmware/ BIOS.
In this case it is nice to have a cpufreq so it is possible to set the
system during boot to a higher clock speed. And then remain idle unless
the cpufreq governor changed.

> Best regards,
> Krzysztof

Sebastian

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [RFC PATCH] cpufreq: qcom-cpufreq-hw: allow work to be done on other CPU for PREEMPT_RT
  2023-03-21 10:57     ` Sebastian Andrzej Siewior
@ 2023-03-21 11:27       ` Krzysztof Kozlowski
  2023-03-21 13:39         ` Sebastian Andrzej Siewior
  0 siblings, 1 reply; 10+ messages in thread
From: Krzysztof Kozlowski @ 2023-03-21 11:27 UTC (permalink / raw)
  To: Sebastian Andrzej Siewior
  Cc: Andy Gross, Bjorn Andersson, Konrad Dybcio, Rafael J. Wysocki,
	Viresh Kumar, linux-arm-msm, linux-pm, linux-kernel,
	Adrien Thierry, Brian Masney, linux-rt-users

On 21/03/2023 11:57, Sebastian Andrzej Siewior wrote:
> On 2023-03-21 11:24:46 [+0100], Krzysztof Kozlowski wrote:
>>>> --- 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.
> 
> I still fail to understand why this is PREEMPT_RT specific and not a
> problem in general when it comes not NO_HZ_FULL and/ or CPU isolation.

Hm, good point, I actually don't know what is the workqueue
recommendation for NO_HZ_FULL CPUs - is still locality of the workqueue
preferred?

And how such code would look like?
if (tick_nohz_tick_stopped())?

> However the thermal notifications have nothing to do with cpufreq.

They have. The FW notifies that thermal mitigation is happening and
maximum allowed frequency is now XYZ. The cpufreq receives this and sets
maximum allowed scaling frequency for governor.

> 
>> The only other solution is to disable the cpufreq device, e.g. by not
>> compiling it.
> 
> People often disable cpufreq because _usually_ the system boots at
> maximum performance. There are however exceptions and even x86 system
> are configured sometimes to a lower clock speed by the firmware/ BIOS.
> In this case it is nice to have a cpufreq so it is possible to set the
> system during boot to a higher clock speed. And then remain idle unless
> the cpufreq governor changed.

Which we do not want here, thus disabling cpufreq is not the interesting
solution...

Best regards,
Krzysztof


^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [RFC PATCH] cpufreq: qcom-cpufreq-hw: allow work to be done on other CPU for PREEMPT_RT
  2023-03-21 11:27       ` Krzysztof Kozlowski
@ 2023-03-21 13:39         ` Sebastian Andrzej Siewior
  2023-03-23  8:16           ` Krzysztof Kozlowski
  0 siblings, 1 reply; 10+ messages in thread
From: Sebastian Andrzej Siewior @ 2023-03-21 13:39 UTC (permalink / raw)
  To: Krzysztof Kozlowski
  Cc: Andy Gross, Bjorn Andersson, Konrad Dybcio, Rafael J. Wysocki,
	Viresh Kumar, linux-arm-msm, linux-pm, linux-kernel,
	Adrien Thierry, Brian Masney, linux-rt-users,
	Frederic Weisbecker, Thomas Gleixner, Ingo Molnar

On 2023-03-21 12:27:42 [+0100], Krzysztof Kozlowski wrote:
> > I still fail to understand why this is PREEMPT_RT specific and not a
> > problem in general when it comes not NO_HZ_FULL and/ or CPU isolation.
> 
> Hm, good point, I actually don't know what is the workqueue
> recommendation for NO_HZ_FULL CPUs - is still locality of the workqueue
> preferred?

If you isolate a CPU you want the kernel to stay away from it. The idea
is that something is done on that CPU and the kernel should leave it
alone. That is why the HZ tick avoided. That is why timers migrate to
the "housekeeping" CPU and do not fire on the CPU that it was programmed
on (unless the timer has to fire on this CPU).

> And how such code would look like?
> if (tick_nohz_tick_stopped())?

Yeah closer :) The CPU-mask for workqueues can still be different on
non-NOHZ-full CPUs. Still you interrupt the CPU doing in-userland work
and this is not desired.

You have a threaded-IRQ which does nothing but schedules a worker. Why?
Why not sleep and remain in that threaded IRQ until the work is done?
You _can_ sleep in the threaded IRQ if you have to. Force-threaded is
different but this is one is explicit threaded so you could do it.
	
> > However the thermal notifications have nothing to do with cpufreq.
> 
> They have. The FW notifies that thermal mitigation is happening and
> maximum allowed frequency is now XYZ. The cpufreq receives this and sets
> maximum allowed scaling frequency for governor.

I see. So the driver is doing something in worst case. This interrupt,
you have per-CPU and you need to do this CPU? I mean could you change
the affinity of the interrupt to another CPU?

> Best regards,
> Krzysztof

Sebastian

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [RFC PATCH] cpufreq: qcom-cpufreq-hw: allow work to be done on other CPU for PREEMPT_RT
  2023-03-21 13:39         ` Sebastian Andrzej Siewior
@ 2023-03-23  8:16           ` Krzysztof Kozlowski
  2023-03-23 11:37             ` Sebastian Andrzej Siewior
  0 siblings, 1 reply; 10+ messages in thread
From: Krzysztof Kozlowski @ 2023-03-23  8:16 UTC (permalink / raw)
  To: Sebastian Andrzej Siewior
  Cc: Andy Gross, Bjorn Andersson, Konrad Dybcio, Rafael J. Wysocki,
	Viresh Kumar, linux-arm-msm, linux-pm, linux-kernel,
	Adrien Thierry, Brian Masney, linux-rt-users,
	Frederic Weisbecker, Thomas Gleixner, Ingo Molnar

On 21/03/2023 14:39, Sebastian Andrzej Siewior wrote:
> On 2023-03-21 12:27:42 [+0100], Krzysztof Kozlowski wrote:
>>> I still fail to understand why this is PREEMPT_RT specific and not a
>>> problem in general when it comes not NO_HZ_FULL and/ or CPU isolation.
>>
>> Hm, good point, I actually don't know what is the workqueue
>> recommendation for NO_HZ_FULL CPUs - is still locality of the workqueue
>> preferred?
> 
> If you isolate a CPU you want the kernel to stay away from it. The idea
> is that something is done on that CPU and the kernel should leave it
> alone. That is why the HZ tick avoided. That is why timers migrate to
> the "housekeeping" CPU and do not fire on the CPU that it was programmed
> on (unless the timer has to fire on this CPU).
> 
>> And how such code would look like?
>> if (tick_nohz_tick_stopped())?
> 
> Yeah closer :) The CPU-mask for workqueues can still be different on
> non-NOHZ-full CPUs. Still you interrupt the CPU doing in-userland work
> and this is not desired.

Probably this should be done by workqueue core code.  Individual drivers
should not need to investigate which CPUs are isolated.


> You have a threaded-IRQ which does nothing but schedules a worker. Why?
> Why not sleep and remain in that threaded IRQ until the work is done?
> You _can_ sleep in the threaded IRQ if you have to. Force-threaded is
> different but this is one is explicit threaded so you could do it.

If I get your point correctly, you want the IRQ handler thread to do the
actual work instead of scheduling work? The answer to this is probably here:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=e0e27c3d4e20dab861566f1c348ae44e4b498630

> 	
>>> However the thermal notifications have nothing to do with cpufreq.
>>
>> They have. The FW notifies that thermal mitigation is happening and
>> maximum allowed frequency is now XYZ. The cpufreq receives this and sets
>> maximum allowed scaling frequency for governor.
> 
> I see. So the driver is doing something in worst case. This interrupt,
> you have per-CPU and you need to do this CPU? I mean could you change
> the affinity of the interrupt to another CPU?

I don't know. The commit introducing it:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=3ed6dfbd3bb987b3d2de86304ae45972ebff5870
claimed it helps to reduce number of interrupts hitting CPU 10x-100x
times... I don't see it - neither in tests nor in the code, so I am just
thinking to revert that one.

Best regards,
Krzysztof


^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [RFC PATCH] cpufreq: qcom-cpufreq-hw: allow work to be done on other CPU for PREEMPT_RT
  2023-03-23  8:16           ` Krzysztof Kozlowski
@ 2023-03-23 11:37             ` Sebastian Andrzej Siewior
  0 siblings, 0 replies; 10+ messages in thread
From: Sebastian Andrzej Siewior @ 2023-03-23 11:37 UTC (permalink / raw)
  To: Krzysztof Kozlowski
  Cc: Andy Gross, Bjorn Andersson, Konrad Dybcio, Rafael J. Wysocki,
	Viresh Kumar, linux-arm-msm, linux-pm, linux-kernel,
	Adrien Thierry, Brian Masney, linux-rt-users,
	Frederic Weisbecker, Thomas Gleixner, Ingo Molnar

On 2023-03-23 09:16:27 [+0100], Krzysztof Kozlowski wrote:
> > Yeah closer :) The CPU-mask for workqueues can still be different on
> > non-NOHZ-full CPUs. Still you interrupt the CPU doing in-userland work
> > and this is not desired.
> 
> Probably this should be done by workqueue core code.  Individual drivers
> should not need to investigate which CPUs are isolated.

_Either_ this is part of the interrupt service routine or it is not.
Sometimes work can be offloaded.
However this interrupt is short and offloads work to a workqueue.
Can the interrupt be moved to another CPU without breaking something? 
The use can only change the CPUs on which the workqueue can run but also
the affinity of the IRQ itself. If the user wishes to isolate CPU X and
move workqueues and interrupts away from the CPU the question is why is
this a problem for you.

> > You have a threaded-IRQ which does nothing but schedules a worker. Why?
> > Why not sleep and remain in that threaded IRQ until the work is done?
> > You _can_ sleep in the threaded IRQ if you have to. Force-threaded is
> > different but this is one is explicit threaded so you could do it.
> 
> If I get your point correctly, you want the IRQ handler thread to do the
> actual work instead of scheduling work? The answer to this is probably here:
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=e0e27c3d4e20dab861566f1c348ae44e4b498630

Let me look.

| Re-enabling an interrupt from its own interrupt handler may cause
| an interrupt storm, if there is a pending interrupt and because its
| handling is disabled due to already done entrance into the handler
| above in the stack.

I have hard time parsing this.
disable_irq_nosync() and enable enable_irq() shouldn't be invoked from
within the interrupt handler itself. This interrupt is already requested
as a threaded handler with IRQF_ONESHOT. This means the IRQ-chip already
disables the interrupt while the threaded handler is invoked. No need
for that.

I don't know what the purpose of reg_intr_clr here is. Acking the
interrupt before reading the status and doing any work does not look
right.

| Also, apparently it is improper to lock a mutex in an interrupt contex.

Again, this is an interrupt handler requested as a threaded handler.
This handler is invoked with enabled interrupts and preemption. The code
in this handler can invoke ssleep() or mutex_lock(). A might_sleep()
does not produce a plat here. It okay to acquire a mutex. This is why we
have threaded interrupts.

You can't acquire a mutex in a forced-threaded handler. This is not the
case here.

> > 	
> >>> However the thermal notifications have nothing to do with cpufreq.
> >>
> >> They have. The FW notifies that thermal mitigation is happening and
> >> maximum allowed frequency is now XYZ. The cpufreq receives this and sets
> >> maximum allowed scaling frequency for governor.
> > 
> > I see. So the driver is doing something in worst case. This interrupt,
> > you have per-CPU and you need to do this CPU? I mean could you change
> > the affinity of the interrupt to another CPU?
> 
> I don't know. The commit introducing it:
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=3ed6dfbd3bb987b3d2de86304ae45972ebff5870
> claimed it helps to reduce number of interrupts hitting CPU 10x-100x
> times... I don't see it - neither in tests nor in the code, so I am just
> thinking to revert that one.

So it may run on another CPU but doing it on the right cluster reduces
the received interrupt 10-100 times. Do we have per-cluster register or
is the interrupt ACK wrong and this what is observed?

The questions are:
- What is the interrupt signaling.
- What must be done to acknowledge the interrupt.

This needs to be figured out and verified that it actually works as
intended. The hardware might keep sending interrupt because the source
is either not acknowledge properly or the source of the interrupt (the
reason why it was generated in first place) is still pending/ not
handled. The changes you reference look like "if we do this then it
seems better.". No explanation about the issue/ root cause and the
targeted solution.

> Best regards,
> Krzysztof

Sebastian

^ permalink raw reply	[flat|nested] 10+ messages in thread

end of thread, other threads:[~2023-03-23 11:37 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-03-15 16:49 [RFC PATCH] cpufreq: qcom-cpufreq-hw: allow work to be done on other CPU for PREEMPT_RT Krzysztof Kozlowski
2023-03-16 12:28 ` Krzysztof Kozlowski
     [not found] ` <20230316235705.2235-1-hdanton@sina.com>
2023-03-17  8:13   ` Krzysztof Kozlowski
2023-03-21 10:04 ` Sebastian Andrzej Siewior
2023-03-21 10:24   ` Krzysztof Kozlowski
2023-03-21 10:57     ` Sebastian Andrzej Siewior
2023-03-21 11:27       ` Krzysztof Kozlowski
2023-03-21 13:39         ` Sebastian Andrzej Siewior
2023-03-23  8:16           ` Krzysztof Kozlowski
2023-03-23 11:37             ` Sebastian Andrzej Siewior

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).