On Sat, Jan 08, 2022 at 12:35:40PM -0600, Eric W. Biederman wrote:
> There are kernel threads started by modules that do:
> complete(...);
> return 0;
>
> That should be at a minimum calling complete_and_exit. Possibly should
> be restructured to use kthread_stop().
>
> Some of those users of the now removed thread_exit() in staging are
> among the offenders.
>
> However thread_exit() was implemented as:
> #define thread_exit() complete_and_exit(NULL, 0)
>
> Which does nothing with a completion, it was just a really funny way to
> spell "do_exit(0)".
Yes. And there's a plenty of cargo-culting in that area.
> While I agree digging through all of the kernel threads and finding the
> ones that should be calling complete_and_exit is a fine idea. It is
> a concern independent of these patches.
BTW, could somebody explain how could this
/*
* Prevent the kthread exits directly, and make sure when kthread_stop()
* is called to stop a kthread, it is still alive. If a kthread might be
* stopped by CACHE_SET_IO_DISABLE bit set, wait_for_kthread_stop() is
* necessary before the kthread returns.
*/
static inline void wait_for_kthread_stop(void)
{
while (!kthread_should_stop()) {
set_current_state(TASK_INTERRUPTIBLE);
schedule();
}
}
in drivers/md/bcache/bcache.h possibly avoid losing wakeups?
AFAICS, it can be called while in TASK_RUNNING. Suppose kthread_stop()
gets called just after the check for kthread_should_stop(). Our thread
is still in TASK_RUNNING; kthread_stop() sets the flag for the next
kthread_should_stop() to observe and does wake_up_process() to our
thread. Which does nothing. Now our thread goes into TASK_INTERRUPTIBLE
and calls schedule(). Sure, as soon as it gets woken up it'll call
kthread_should_stop(), get true from it and that's it. What's going
to wake it up, though?
The same goes for e.g. fs/btrfs/disk-io.c:cleaner_kthread():
if (kthread_should_stop())
return 0;
if (!again) {
set_current_state(TASK_INTERRUPTIBLE);
schedule();
__set_current_state(TASK_RUNNING);
}
can't be right. Similar fun exists in e.g. fs/jfs, etc.
Am I missing something?
