From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by smtp.lore.kernel.org (Postfix) with ESMTP id D3663C4167B for ; Tue, 28 Nov 2023 17:40:29 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1345272AbjK1RkU (ORCPT ); Tue, 28 Nov 2023 12:40:20 -0500 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:60174 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1345347AbjK1RkN (ORCPT ); Tue, 28 Nov 2023 12:40:13 -0500 Received: from out-172.mta0.migadu.com (out-172.mta0.migadu.com [91.218.175.172]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 8E518D64 for ; Tue, 28 Nov 2023 09:40:18 -0800 (PST) Date: Tue, 28 Nov 2023 12:40:12 -0500 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linux.dev; s=key1; t=1701193216; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: in-reply-to:in-reply-to:references:references; bh=INs2zZgjKSxCudQ4urP1DNFRoEn0+TxPVLEnF4N6OJI=; b=b302NC3gfpP27hEZZOIe+T5tsmwvJVErzX8kUCHeWMbrFdst8W4meCK43eU2fKX6vAsCw2 yAJLib84C2pCW7vxReaCpzLzdEjmigAzL80YQak+PVNIqTxVNEohLWyCvpliBiiToKYVMB dFN7lH9hwG/YQT7CCKR2rn17QKmKzDM= X-Report-Abuse: Please report any abuse attempt to abuse@migadu.com and include these headers. From: Kent Overstreet To: Petr Mladek Cc: linux-kernel@vger.kernel.org, Nicholas Piggin , Mike Christie , Zqiang , Andrew Morton Subject: Re: [PATCH] kthread: kthread_should_stop() checks if we're a kthread Message-ID: <20231128174012.bfno4f3matn3d7cw@moria.home.lan> References: <20231120221503.3378095-1-kent.overstreet@linux.dev> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: X-Migadu-Flow: FLOW_OUT Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Tue, Nov 28, 2023 at 10:30:49AM +0100, Petr Mladek wrote: > Adding Andrew into Cc. He usually takes changes in kernel/kthread.c. > > On Mon 2023-11-20 17:15:03, Kent Overstreet wrote: > > bcachefs has a fair amount of code that may or may not be running from a > > kthread (it may instead be called by a userspace ioctl); having > > kthread_should_stop() check if we're a kthread enables a fair bit of > > cleanup and makes it safer to use. > > > > Signed-off-by: Kent Overstreet > > --- > > kernel/kthread.c | 3 ++- > > 1 file changed, 2 insertions(+), 1 deletion(-) > > > > diff --git a/kernel/kthread.c b/kernel/kthread.c > > index 1eea53050bab..fe6090ddf414 100644 > > --- a/kernel/kthread.c > > +++ b/kernel/kthread.c > > @@ -155,7 +155,8 @@ void free_kthread_struct(struct task_struct *k) > > */ > > bool kthread_should_stop(void) > > { > > - return test_bit(KTHREAD_SHOULD_STOP, &to_kthread(current)->flags); > > + return (current->flags & PF_KTHREAD) && > > + test_bit(KTHREAD_SHOULD_STOP, &to_kthread(current)->flags); > > } > > EXPORT_SYMBOL(kthread_should_stop); > > I agree that it makes the API more safe because &to_kthread(current) > is NULL when the process is not a kthread. > > Well, I do not like the idea of quietly ignoring a misuse of > the kthread_*() API. I would personally prefer to do: It's only a misuse in the most pedantic sense, IMO. Is it ever possible that with this change calling kthread_should_stop() from a non-kthread could cause a problem? > > // define this in include/linux/kthread.h > static inline bool in_kthread(void) > { > return current->flags & PF_KTHREAD > } > > // add WARN() into kthread_should_stop() > bool kthread_should_stop(void) > { > if (WARN_ON_ONCE(!in_kthread)) > return false; > > return test_bit(KTHREAD_SHOULD_STOP, &to_kthread(current)->flags); > } > EXPORT_SYMBOL(kthread_should_stop); > > > And use the following in bcachefs() code: > > if (in_kthread() && kthread_should_stop()) > goto exit; That's what bcachefs code does today, and forgetting to check in_kthread() is a real footgun - particularly for other people starting to work on the code. So I could do a bch2_in_kthread() instead that does the in_kthread( check, but then I have to make sure that people know to use bch2_in_kthread() instead of in_kthread(). Not ideal.