Messages in this thread | | | Date | Tue, 28 Nov 2023 12:40:12 -0500 | From | Kent Overstreet <> | Subject | Re: [PATCH] kthread: kthread_should_stop() checks if we're a kthread |
| |
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 <kent.overstreet@linux.dev> > > --- > > 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.
| |