Re: [PATCH RESEND v11] hwrng: core - let sleep be interrupted when unregistering hwrng

[Valentin Schneider <vschneid@redhat.com>]

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