On 12/15/2015 02:31 AM, Juri Lelli wrote: >>>> + do { >>>> > >> + set_current_state(TASK_INTERRUPTIBLE); >>>> > >> + new_request = gd->requested_freq; >>>> > >> + if (new_request == last_request) { >>>> > >> + schedule(); >>>> > >> + } else { >>> > > >>> > > Shouldn't we have to do the following here? >>> > > >>> > > >>> > > @@ -125,9 +125,9 @@ static int cpufreq_sched_thread(void *data) >>> > > } >>> > > >>> > > do { >>> > > - set_current_state(TASK_INTERRUPTIBLE); >>> > > new_request = gd->requested_freq; >>> > > if (new_request == last_request) { >>> > > + set_current_state(TASK_INTERRUPTIBLE); >>> > > schedule(); >>> > > } else { >>> > > /* >>> > > >>> > > Otherwise we set task to INTERRUPTIBLE state right after it has been >>> > > woken up. >> > >> > The state must be set to TASK_INTERRUPTIBLE before the data used to >> > decide whether to sleep or not is read (gd->requested_freq in this case). >> > >> > If it is set after, then once gd->requested_freq is read but before the >> > state is set to TASK_INTERRUPTIBLE, the other side may update >> > gd->requested_freq and issue a wakeup on the freq thread. The wakeup >> > will have no effect since the freq thread would still be TASK_RUNNING at >> > that time. The freq thread would proceed to go to sleep and the update >> > would be lost. >> > > Mmm, I suggested that because I was hitting this while testing: > > [ 34.816158] ------------[ cut here ]------------ > [ 34.816177] WARNING: CPU: 2 PID: 1712 at kernel/kernel/sched/core.c:7617 > __might_sleep+0x90/0xa8() > [ 34.816188] do not call blocking ops when !TASK_RUNNING; state=1 set at > [<c007c1f8>] cpufreq_sched_thread+0x80/0x2b0 > [ 34.816198] Modules linked in: > [ 34.816207] CPU: 2 PID: 1712 Comm: kschedfreq:1 Not tainted 4.4.0-rc2+ #401 > [ 34.816212] Hardware name: ARM-Versatile Express > [ 34.816229] [<c0018874>] (unwind_backtrace) from [<c0013f60>] > (show_stack+0x20/0x24) > [ 34.816243] [<c0013f60>] (show_stack) from [<c0448c98>] > (dump_stack+0x80/0xb4) > [ 34.816257] [<c0448c98>] (dump_stack) from [<c0029930>] > (warn_slowpath_common+0x88/0xc0) > [ 34.816267] [<c0029930>] (warn_slowpath_common) from [<c0029a24>] > (warn_slowpath_fmt+0x40/0x48) > [ 34.816278] [<c0029a24>] (warn_slowpath_fmt) from [<c0054764>] > (__might_sleep+0x90/0xa8) > [ 34.816291] [<c0054764>] (__might_sleep) from [<c0578400>] > (cpufreq_freq_transition_begin+0x6c/0x13c) > [ 34.816303] [<c0578400>] (cpufreq_freq_transition_begin) from [<c0578714>] > (__cpufreq_driver_target+0x180/0x2c0) > [ 34.816314] [<c0578714>] (__cpufreq_driver_target) from [<c007c14c>] > (cpufreq_sched_try_driver_target+0x48/0x74) > [ 34.816324] [<c007c14c>] (cpufreq_sched_try_driver_target) from > [<c007c1e8>] (cpufreq_sched_thread+0x70/0x2b0) > [ 34.816336] [<c007c1e8>] (cpufreq_sched_thread) from [<c004ce30>] > (kthread+0xf4/0x114) > [ 34.816347] [<c004ce30>] (kthread) from [<c000fdd0>] > (ret_from_fork+0x14/0x24) > [ 34.816355] ---[ end trace 30e92db342678467 ]--- > > Maybe we could cope with what you are saying with an atomic flag > indicating that the kthread is currently servicing a request? Like > extending the finish_last_request thing to cover this case as well.
Ah. I should be able to just set_current_state(TASK_RUNNING) at the top of the else clause. Will include this change next time. thanks, Steve -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/