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.
Cc: Nicholas Piggin <[email protected]>
Cc: Mike Christie <[email protected]>
Cc: Zqiang <[email protected]>
Cc: Petr Mladek <[email protected]>
Signed-off-by: Kent Overstreet <[email protected]>
---
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);
--
2.42.0
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 <[email protected]>
> ---
> 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:
// 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;
Is see several advantages:
+ It will warn when the API is misused.
+ It will be more clear that the bcachefs code might be
used in both kthread and userspace code.
+ in_kthread() might be used around other code which is
needed only when the process is a kthread.
+ Similar check and WARN() might be used also in the other
kthread() API.
Best Regards,
Petr
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 <[email protected]>
> > ---
> > 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.
On Tue 2023-11-28 12:40:12, Kent Overstreet wrote:
> 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 <[email protected]>
> > > ---
> > > 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?
Of course, calling the updated kthread_should_stop() won't
cause problems in non-kthread context.
But it would make it more complicated to check for misuse
in the future.
Q: Why is a check for misuse important?
A: It partly depends on the personal opinion. But in general, it
prevents other problems.
Q: Is it worth it in this case?
A: I do not have strong opinion.
I have basically two concerns:
1. Consistent behavior:
For example, kthread_use_mm() and kthread_unuse_mm() already
warn about a misuse.
2. Understanding the code
Unconditional use of kthread_should_stop() makes the feeling
that the code is intended for kthread context. People, not
familiar with the code might miss that it might run also
in userspace process.
And indeed, I have looked how kthread_should_stop() is
used in bcachefs code. For example, bch2_move_ratelimit()
seems to handle only the kthread context:
int bch2_move_ratelimit(struct moving_context *ctxt)
{
[...]
if (ctxt->wait_on_copygc && !c->copygc_running) {
bch2_trans_unlock_long(ctxt->trans);
wait_event_killable(c->copygc_running_wq,
!c->copygc_running ||
kthread_should_stop());
}
Yes, wait_event_killable() is primary for userspace
process. AFAIK, signals are not delivered to kthreads
by default, see sig_task_ignored().
But the code does not check return value so that
it does not detect when usespace process calling
this function was killed.
do {
delay = ctxt->rate ? bch2_ratelimit_delay(ctxt->rate) : 0;
[...]
if (delay) {
[...]
set_current_state(TASK_INTERRUPTIBLE);
}
[...]
if ((current->flags & PF_KTHREAD) && kthread_should_stop()) {
__set_current_state(TASK_RUNNING);
return 1;
}
if (delay)
schedule_timeout(delay);
[...]
} while (delay);
Here, the do-while cycle returns early when the kthread gets
stopped. But it does not check whether there is a pending
signal when called from userspace process.
IMHO, it would be better to make it clear that the function might
be called from both kthread and userspace processes. Otherwise,
someone could later replace wait_event_killable() to wait_event()
because kthreads do not react to signals by default.
Also it would be worth to make it more clear what code is for
the kthread context and what is for the userspace context.
And the following would make it more obvious:
if (in_kthread) {
// handle kthread_should_stop
} else {
// handle signal_pending()
}
IMPORTANT: If the function could return early because of signal
then it might make sense to make the kthread call path
more robust. The kthread should not return before another
process called kthread_stop().
I vaguely remember that it might cause some problems.
I am not sure how big problems. Either the kthread would stay in
a limbo state because nobody would read the exit
code. Or the eventual kthread_stop() caller could
blow up or something like this.
BTW: kthread_should_stop() is called in wait_event_killable()
without PF_KTHREAD check so that it could probably blow up.
My experience says that it is far from trivial to handle kthread_stop(),
freezer, and signals correctly. IMHO, doing some implicit
NOPs do not help to get the right picture.
BTW: try_to_freeze() might cause a deadlock when a kthread_stop()
caller waits for the kthread() return but the kthread()
is blocked in __refrigerator().
At least this is my understanding of the commit 8a32c441c1609f80e
("freezer: implement and use kthread_freezable_should_stop()").
Maybe, __refrigerator() should check kthread_should_stop()
for all kthreads. But maybe, it might break something else.
> > // 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);
I like this solution because the warning is printed even when
KTHREAD_SHOULD_STOP is not set. It means that misuse might
be caught easily.
Anyway, I am going to let Andrew to do a final decision.
Best Regards,
Petr