On 26/07/22 12:17, Jason A. Donenfeld wrote:
>> if (rc <= 0) {
>> - pr_warn("hwrng: no data available\n");
>> - msleep_interruptible(10000);
>> + set_current_state(TASK_INTERRUPTIBLE);
>> + if (kthread_should_stop())
>> + __set_current_state(TASK_RUNNING);
>> + schedule_timeout(10 * HZ);
>> continue;
>> }
>
> Here you made a change whose utility I don't understand. My original
> hunk was:
>
> + if (kthread_should_stop())
> + break;
> + schedule_timeout_interruptible(HZ * 10);
>
> Which I think is a bit cleaner, as schedule_timeout_interruptible sets
> the state to INTERRUPTIBLE and such.
>
For any sort of wait loop, you need the state write to happen before the
loop-break condition is checked.
Consider:
READ kthread_should_stop() == false
kthread_stop()
set_bit(KTHREAD_SHOULD_STOP, &kthread->flags);
wake_up_process(k); // Reads TASK_RUNNING,
schedule_timeout_interruptible(); // does nothing
// We're now blocked, having missed a wakeup
That's why the canonical wait loop pattern is:
for (;;) {
set_current_state(TASK_UNINTERRUPTIBLE);
if (CONDITION)
break;
schedule();
}
__set_current_state(TASK_RUNNING);
(grep "wait loop" in kernel/sched/core.c)
> Regards,
> Jason
