On Tue, May 22, 2018 at 12:02 PM, Rafael J. Wysocki <raf...@kernel.org> wrote: > On Mon, May 21, 2018 at 6:13 PM, Joel Fernandes <j...@joelfernandes.org> > wrote: >> On Mon, May 21, 2018 at 10:29:52AM +0200, Rafael J. Wysocki wrote: >>> On Mon, May 21, 2018 at 7:14 AM, Viresh Kumar <viresh.ku...@linaro.org> >>> wrote: >>> > On 18-05-18, 11:55, Joel Fernandes (Google.) wrote: >>> >> From: "Joel Fernandes (Google)" <j...@joelfernandes.org> >>> >> >>> >> Currently there is a chance of a schedutil cpufreq update request to be >>> >> dropped if there is a pending update request. This pending request can >>> >> be delayed if there is a scheduling delay of the irq_work and the wake >>> >> up of the schedutil governor kthread. >>> >> >>> >> A very bad scenario is when a schedutil request was already just made, >>> >> such as to reduce the CPU frequency, then a newer request to increase >>> >> CPU frequency (even sched deadline urgent frequency increase requests) >>> >> can be dropped, even though the rate limits suggest that its Ok to >>> >> process a request. This is because of the way the work_in_progress flag >>> >> is used. >>> >> >>> >> This patch improves the situation by allowing new requests to happen >>> >> even though the old one is still being processed. Note that in this >>> >> approach, if an irq_work was already issued, we just update next_freq >>> >> and don't bother to queue another request so there's no extra work being >>> >> done to make this happen. >>> > >>> > Now that this isn't an RFC anymore, you shouldn't have added below >>> > paragraph here. It could go to the comments section though. >>> > >>> >> I had brought up this issue at the OSPM conference and Claudio had a >>> >> discussion RFC with an alternate approach [1]. I prefer the approach as >>> >> done in the patch below since it doesn't need any new flags and doesn't >>> >> cause any other extra overhead. >>> >> >>> >> [1] https://patchwork.kernel.org/patch/10384261/ >>> >> >>> >> LGTMed-by: Viresh Kumar <viresh.ku...@linaro.org> >>> >> LGTMed-by: Juri Lelli <juri.le...@redhat.com> >>> > >>> > Looks like a Tag you just invented ? :) >>> >>> Yeah. >>> >>> The LGTM from Juri can be converned into an ACK silently IMO. That >>> said I have added Looks-good-to: tags to a couple of commits. :-) >> >> Cool, I'll covert them to Acks :-) > > So it looks like I should expect an update of this patch, right? > > Or do you prefer the current one to be applied and work on top of it? >
[cut] >> >> I just realized that on a single policy switch that uses the governor thread, >> there will be 1 thread per-CPU. The sugov_update_single will be called on the >> same CPU with interrupts disabled. > > sugov_update_single() doesn't have to run on the target CPU. Which sadly is a bug IMO. :-/ >> In sugov_work, we are doing a >> raw_spin_lock_irqsave which also disables interrupts. So I don't think >> there's any possibility of a race happening on the same CPU between the >> frequency update request and the sugov_work executing. In other words, I feel >> we can drop the above if (..) statement for single policies completely and >> only keep the changes for the shared policy. Viresh since you brought up the >> single policy issue initially which made me add this if statememnt, could you >> let me know if you agree with what I just said? > > Which is why you need the spinlock too. And you totally have a point. With the above bug fixed, disabling interrupts should be sufficient to prevent concurrent updates from occurring in the one-CPU policy case and the work_in_progress check in sugov_update_single() isn't necessary.
