Re: re. Spurious wakeup on a newly created kthread

Michal Hocko
2022-06-27T12:06:52Z

On Sat 25-06-22 19:53:34, Linus Torvalds wrote:
> On Sat, Jun 25, 2022 at 6:58 PM Tejun Heo <tj@kernel.org> wrote:
[...]
> > * If there are no true spurious wakeups, where did the racing wakeup
> > come from? The task just got created w/ TASK_NEW and woken up once
> > with wake_up_new_task(). It hasn't been on any wait queue or
> > advertised itself to anything.
>
> I don't think it was ever a spurious wakeup at all.
>
> The create_worker() code does:
>
> worker->task = kthread_create_on_node(..
> ..
> worker_attach_to_pool(worker, pool);
> ..
> wake_up_process(worker->task);
>
> and thinks that the wake_up_process() happens after the worker_attach_to_pool().
>
> But I don't see that at all.
>
> The reality seems to be that the wake_up_process() is a complete
> no-op, because the task was already woken up by
> kthread_create_on_node().

Just for the record.
the newly created thread is not running our thread function at this
stage. It is rather subtle and took me some time to decypher but
kthread_create_on_node will create and wake up kernel thread running
kthread() function:
[...]
/*
* Thread is going to call schedule(), do not preempt it,
* or the creator may spend more time in wait_task_inactive().
*/
preempt_disable();
complete(done);
schedule_preempt_disabled();
preempt_enable();

ret = -EINTR;
if (!test_bit(KTHREAD_SHOULD_STOP, &self->flags)) {
cgroup_kthread_ready();
__kthread_parkme(self);
ret = threadfn(data);
}

so the newly created thread will go into sleep before calling the
threadfn (worker_thread here). Somebody must have woken it up other than
create_worker. I couldn't have found out who that was (see my other
email with some notes from the crash dump).

I do agree that a simple schedule without checking for a condition is
dubious, fragile and wrong. If anything wait_for_completion would be less
confusing and targetted waiting.

Petr has added that completion into worker_thread to address this
specific case and a better fix would be to address all callers because
who knows how many of those are similarly broken.

I also do agree that this whole scheme is rather convoluted and having
an init() callback to be executed before threadfn would be much more
easier to follow.
--
Michal Hocko
SUSE Labs