Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Mon, 27 Jun 2022 14:04:02 +0200
From:   Michal Hocko <mhocko@suse.com>
To:     Linus Torvalds <torvalds@linux-foundation.org>
Cc:     Tejun Heo <tj@kernel.org>, Petr Mladek <pmladek@suse.com>,
        Lai Jiangshan <jiangshanlai@gmail.com>,
        Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
        Peter Zijlstra <peterz@infradead.org>,
        Thomas Gleixner <tglx@linutronix.de>,
        Ingo Molnar <mingo@redhat.com>,
        Andrew Morton <akpm@linux-foundation.org>,
        Oleg Nesterov <oleg@redhat.com>,
        "Eric W. Biederman" <ebiederm@xmission.com>
Subject: Re: re. Spurious wakeup on a newly created kthread
Message-ID: <YrmcsnHLjadryMSx@dhcp22.suse.cz>
References: <20220622140853.31383-1-pmladek@suse.com>
 <YraWWl+Go17uPOgR@mtj.duckdns.org>
 <CAHk-=wiC7rj1o7vTnYUPfD7YxAu09MZiZbahHqvLm9+Cgg1dFw@mail.gmail.com>
 <Yre9LO2nj+Hbr67V@mtj.duckdns.org>
 <CAHk-=wjmWUSdK7-LLjpUrH_TX78emb3ajxZ1ueT2HU0_FVJQfA@mail.gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <CAHk-=wjmWUSdK7-LLjpUrH_TX78emb3ajxZ1ueT2HU0_FVJQfA@mail.gmail.com>
Precedence: bulk

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