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;
Sender: Tejun Heo <htejun@gmail.com>
Date:   Mon, 27 Jun 2022 05:23:00 +0900
From:   Tejun Heo <tj@kernel.org>
To:     Linus Torvalds <torvalds@linux-foundation.org>
Cc:     "Eric W. Biederman" <ebiederm@xmission.com>,
        Christian Brauner <brauner@kernel.org>,
        Petr Mladek <pmladek@suse.com>,
        Lai Jiangshan <jiangshanlai@gmail.com>,
        Michal Hocko <mhocko@suse.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>
Subject: Re: [PATCH 3/3] kthread: Stop abusing TASK_UNINTERRUPTIBLE
 (INCOMPLETE)
Message-ID: <YrjAJN7dDJ9R7Ocu@mtj.duckdns.org>
References: <CAHk-=wiC7rj1o7vTnYUPfD7YxAu09MZiZbahHqvLm9+Cgg1dFw@mail.gmail.com>
 <874k0863x8.fsf@email.froward.int.ebiederm.org>
 <CAHk-=wgTG2K3erROP19320zBN6BHVf0hRfXGdawkGR4gzrJN6w@mail.gmail.com>
 <CAHk-=whLsaRKaFKS0UffeCYYCVyP0bbiB4BTYTaXtScgu6R9yA@mail.gmail.com>
 <87pmiw1fy6.fsf@email.froward.int.ebiederm.org>
 <CAHk-=wiutNT47oNhyk_WvMj2qp4pehYFptXCUzW=u_2STLQiww@mail.gmail.com>
 <CAHk-=whX_=BNZ4kVEAu2NV3CMnhwsuYTyE65FQXUMx8VPNOAOA@mail.gmail.com>
 <87ilonuti2.fsf_-_@email.froward.int.ebiederm.org>
 <871qvbutex.fsf_-_@email.froward.int.ebiederm.org>
 <CAHk-=wg9eqtrpYrjJ=yobkwkTimWFtiDd_JOfADttG0fyAJrqg@mail.gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <CAHk-=wg9eqtrpYrjJ=yobkwkTimWFtiDd_JOfADttG0fyAJrqg@mail.gmail.com>
Precedence: bulk

Hello,

On Sun, Jun 26, 2022 at 12:59:09PM -0700, Linus Torvalds wrote:
> And I think *that* should be the change - add a "setup()" function
> pointer to the whole kthread infrastructure. Allow it to return an
> error, which will then just kill the new thread again without ever
> even starting it up.
> 
> I'd really prefer to avoid having random drivers and subsystems know
> about the *very* magical "wake_up_new_task()" thing.  Yes, it's a real
> thing, but it's a thing that normal code should not ever use.
> 
> The whole "wake_up_process()" model for kthread creation was wrong.
> But moving existing users of a bad interface to using the even more
> special "wake_up_new_task()" thing is not the solution, I feel.

This is a bit of bike-shedding but there are inherent downsides to
callback based interface in terms of write/readability. Now you have
to move the init code out of line, and if the context that needs to be
passing doesn't fit in a single pointer, you gotta define a struct to
carry it adding to the boilerplate.

Having separate create and run steps makes sense as a pattern and
wake_up_new_task() is less error prone in one sense - if the caller
doesn't call it, the new kthread actually won't start running making
the bug obvious. The fact that the function blindly trusts the caller
to be handing in an actual new task is scary and we likely don't wanna
proliferate its use across the code base.

Would it be a reasonable tradeoff to have a kthread wrapper -
kthread_start() or whatever - which ensures that it is actually called
once on a new task? That way, we can keep the init code inline and
bugs on both sides (not starting and starting incorrectly) are
obvious.

Thanks.

-- 
tejun