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

From: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
To: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
Cc: Andy Gross <agross@kernel.org>,
	Bjorn Andersson <andersson@kernel.org>,
	Konrad Dybcio <konrad.dybcio@linaro.org>,
	"Rafael J. Wysocki" <rafael@kernel.org>,
	Viresh Kumar <viresh.kumar@linaro.org>,
	linux-arm-msm@vger.kernel.org, linux-pm@vger.kernel.org,
	linux-kernel@vger.kernel.org,
	Adrien Thierry <athierry@redhat.com>,
	Brian Masney <bmasney@redhat.com>,
	linux-rt-users@vger.kernel.org,
	Frederic Weisbecker <fweisbec@gmail.com>,
	Thomas Gleixner <tglx@linutronix.de>,
	Ingo Molnar <mingo@kernel.org>
Subject: Re: [RFC PATCH] cpufreq: qcom-cpufreq-hw: allow work to be done on other CPU for PREEMPT_RT
Date: Thu, 23 Mar 2023 09:16:27 +0100	[thread overview]
Message-ID: <386e5c1b-cf64-8047-97e5-2cbbe3dc415b@linaro.org> (raw)
In-Reply-To: <20230321133922.ontdC41h@linutronix.de>

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