Re: re. Spurious wakeup on a newly created kthread

[Michal Hocko <mhocko@suse.com>]

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