Re: [PATCH 2/7] io_uring: handle signals for IO threads like a normal thread
On 3/27/21 11:40 AM, Eric W. Biederman wrote: > Jens Axboe writes: > >> On 3/26/21 4:38 PM, Jens Axboe wrote: >>> OK good point, and follows the same logic even if it won't make a >>> difference in my case. I'll make the change. >> >> Made the suggested edits and ran the quick tests and the KILL/STOP >> testing, and no ill effects observed. Kicked off the longer runs now. >> >> Not a huge amount of changes from the posted series, but please peruse >> here if you want to double check: >> >> https://git.kernel.dk/cgit/linux-block/log/?h=io_uring-5.12 >> >> And diff against v2 posted is below. Thanks! > > That looks good. Thanks. > > Acked-by: "Eric W. Biederman" Thanks Eric, amended to add that. -- Jens Axboe
Re: [PATCH 2/7] io_uring: handle signals for IO threads like a normal thread
Jens Axboe writes:
> On 3/26/21 4:38 PM, Jens Axboe wrote:
>> OK good point, and follows the same logic even if it won't make a
>> difference in my case. I'll make the change.
>
> Made the suggested edits and ran the quick tests and the KILL/STOP
> testing, and no ill effects observed. Kicked off the longer runs now.
>
> Not a huge amount of changes from the posted series, but please peruse
> here if you want to double check:
>
> https://git.kernel.dk/cgit/linux-block/log/?h=io_uring-5.12
>
> And diff against v2 posted is below. Thanks!
That looks good. Thanks.
Acked-by: "Eric W. Biederman"
>
> diff --git a/fs/io-wq.c b/fs/io-wq.c
> index 3e2f059a1737..7434eb40ca8c 100644
> --- a/fs/io-wq.c
> +++ b/fs/io-wq.c
> @@ -505,10 +505,9 @@ static int io_wqe_worker(void *data)
> if (signal_pending(current)) {
> struct ksignal ksig;
>
> - if (fatal_signal_pending(current))
> - break;
> - if (get_signal(&ksig))
> + if (!get_signal(&ksig))
> continue;
> + break;
> }
> if (ret)
> continue;
> @@ -722,10 +721,9 @@ static int io_wq_manager(void *data)
> if (signal_pending(current)) {
> struct ksignal ksig;
>
> - if (fatal_signal_pending(current))
> - set_bit(IO_WQ_BIT_EXIT, &wq->state);
> - else if (get_signal(&ksig))
> + if (!get_signal(&ksig))
> continue;
> + set_bit(IO_WQ_BIT_EXIT, &wq->state);
> }
> } while (!test_bit(IO_WQ_BIT_EXIT, &wq->state));
>
> diff --git a/fs/io_uring.c b/fs/io_uring.c
> index 66ae46874d85..880abd8b6d31 100644
> --- a/fs/io_uring.c
> +++ b/fs/io_uring.c
> @@ -6746,10 +6746,9 @@ static int io_sq_thread(void *data)
> if (signal_pending(current)) {
> struct ksignal ksig;
>
> - if (fatal_signal_pending(current))
> - break;
> - if (get_signal(&ksig))
> + if (!get_signal(&ksig))
> continue;
> + break;
> }
> sqt_spin = false;
> cap_entries = !list_is_singular(&sqd->ctx_list);
> diff --git a/kernel/signal.c b/kernel/signal.c
> index 5b75fbe3d2d6..f2718350bf4b 100644
> --- a/kernel/signal.c
> +++ b/kernel/signal.c
> @@ -2752,15 +2752,6 @@ bool get_signal(struct ksignal *ksig)
>*/
> current->flags |= PF_SIGNALED;
>
> - /*
> - * PF_IO_WORKER threads will catch and exit on fatal signals
> - * themselves. They have cleanup that must be performed, so
> - * we cannot call do_exit() on their behalf. coredumps also
> - * do not apply to them.
> - */
> - if (current->flags & PF_IO_WORKER)
> - return false;
> -
> if (sig_kernel_coredump(signr)) {
> if (print_fatal_signals)
> print_fatal_signal(ksig->info.si_signo);
> @@ -2776,6 +2767,14 @@ bool get_signal(struct ksignal *ksig)
> do_coredump(&ksig->info);
> }
>
> + /*
> + * PF_IO_WORKER threads will catch and exit on fatal signals
> + * themselves. They have cleanup that must be performed, so
> + * we cannot call do_exit() on their behalf.
> + */
> + if (current->flags & PF_IO_WORKER)
> + goto out;
> +
> /*
>* Death signals, no core dump.
>*/
> @@ -2783,7 +2782,7 @@ bool get_signal(struct ksignal *ksig)
> /* NOTREACHED */
> }
> spin_unlock_irq(&sighand->siglock);
> -
> +out:
> ksig->sig = signr;
>
> if (!(ksig->ka.sa.sa_flags & SA_EXPOSE_TAGBITS))
Re: [PATCH 2/7] io_uring: handle signals for IO threads like a normal thread
On 3/26/21 4:38 PM, Jens Axboe wrote:
> On 3/26/21 4:35 PM, Eric W. Biederman wrote:
>> Jens Axboe writes:
>>
>>> On 3/26/21 4:23 PM, Eric W. Biederman wrote:
Jens Axboe writes:
> On 3/26/21 2:29 PM, Eric W. Biederman wrote:
>> Jens Axboe writes:
>>
>>> We go through various hoops to disallow signals for the IO threads, but
>>> there's really no reason why we cannot just allow them. The IO threads
>>> never return to userspace like a normal thread, and hence don't go
>>> through
>>> normal signal processing. Instead, just check for a pending signal as
>>> part
>>> of the work loop, and call get_signal() to handle it for us if anything
>>> is pending.
>>>
>>> With that, we can support receiving signals, including special ones like
>>> SIGSTOP.
>>>
>>> Signed-off-by: Jens Axboe
>>> ---
>>> fs/io-wq.c| 24 +---
>>> fs/io_uring.c | 12
>>> 2 files changed, 25 insertions(+), 11 deletions(-)
>>>
>>> diff --git a/fs/io-wq.c b/fs/io-wq.c
>>> index b7c1fa932cb3..3e2f059a1737 100644
>>> --- a/fs/io-wq.c
>>> +++ b/fs/io-wq.c
>>> @@ -16,7 +16,6 @@
>>> #include
>>> #include
>>> #include
>>> -#include
>>>
>>> #include "../kernel/sched/sched.h"
>>> #include "io-wq.h"
>>> @@ -503,10 +502,16 @@ static int io_wqe_worker(void *data)
>>> if (io_flush_signals())
>>> continue;
>>> ret = schedule_timeout(WORKER_IDLE_TIMEOUT);
>>> - if (try_to_freeze() || ret)
>>> + if (signal_pending(current)) {
>>> + struct ksignal ksig;
>>> +
>>> + if (fatal_signal_pending(current))
>>> + break;
>>> + if (get_signal(&ksig))
>>> + continue;
>> ^^
>>
>> That is wrong. You are promising to deliver a signal to signal
>> handler and them simply discarding it. Perhaps:
>>
>> if (!get_signal(&ksig))
>> continue;
>> WARN_ON(!sig_kernel_stop(ksig->sig));
>> break;
>
> Thanks, updated.
Gah. Kill the WARN_ON.
I was thinking "WARN_ON(!sig_kernel_fatal(ksig->sig));"
The function sig_kernel_fatal does not exist.
Fatal is the state that is left when a signal is neither
ignored nor a stop signal, and does not have a handler.
The rest of the logic still works.
>>>
>>> I've just come to the same conclusion myself after testing it.
>>> Of the 3 cases, most of them can do the continue, but doesn't
>>> really matter with the way the loop is structured. Anyway, looks
>>> like this now:
>>
>> This idiom in the code:
>>> + if (signal_pending(current)) {
>>> + struct ksignal ksig;
>>> +
>>> + if (fatal_signal_pending(current))
>>> + break;
>>> + if (!get_signal(&ksig))
>>> + continue;
>>> }
>>
>> Needs to be:
>>> + if (signal_pending(current)) {
>>> + struct ksignal ksig;
>>> +
>>> + if (!get_signal(&ksig))
>>> + continue;
>>> + break;
>>> }
>>
>> Because any signal returned from get_signal is fatal in this case.
>> It might make sense to "WARN_ON(ksig->ka.sa.sa_handler != SIG_DFL)".
>> As the io workers don't handle that case.
>>
>> It won't happen because you have everything blocked.
>>
>> The extra fatal_signal_pending(current) logic is just confusing in this
>> case.
>
> OK good point, and follows the same logic even if it won't make a
> difference in my case. I'll make the change.
Made the suggested edits and ran the quick tests and the KILL/STOP
testing, and no ill effects observed. Kicked off the longer runs now.
Not a huge amount of changes from the posted series, but please peruse
here if you want to double check:
https://git.kernel.dk/cgit/linux-block/log/?h=io_uring-5.12
And diff against v2 posted is below. Thanks!
diff --git a/fs/io-wq.c b/fs/io-wq.c
index 3e2f059a1737..7434eb40ca8c 100644
--- a/fs/io-wq.c
+++ b/fs/io-wq.c
@@ -505,10 +505,9 @@ static int io_wqe_worker(void *data)
if (signal_pending(current)) {
struct ksignal ksig;
- if (fatal_signal_pending(current))
- break;
- if (get_signal(&ksig))
+ if (!get_signal(&ksig))
continue;
+ break;
}
if (ret)
continue;
@@ -722,10 +721,9 @@ static int io_wq_manager(void *data)
Re: [PATCH 2/7] io_uring: handle signals for IO threads like a normal thread
On 3/26/21 4:35 PM, Eric W. Biederman wrote:
> Jens Axboe writes:
>
>> On 3/26/21 4:23 PM, Eric W. Biederman wrote:
>>> Jens Axboe writes:
>>>
On 3/26/21 2:29 PM, Eric W. Biederman wrote:
> Jens Axboe writes:
>
>> We go through various hoops to disallow signals for the IO threads, but
>> there's really no reason why we cannot just allow them. The IO threads
>> never return to userspace like a normal thread, and hence don't go
>> through
>> normal signal processing. Instead, just check for a pending signal as
>> part
>> of the work loop, and call get_signal() to handle it for us if anything
>> is pending.
>>
>> With that, we can support receiving signals, including special ones like
>> SIGSTOP.
>>
>> Signed-off-by: Jens Axboe
>> ---
>> fs/io-wq.c| 24 +---
>> fs/io_uring.c | 12
>> 2 files changed, 25 insertions(+), 11 deletions(-)
>>
>> diff --git a/fs/io-wq.c b/fs/io-wq.c
>> index b7c1fa932cb3..3e2f059a1737 100644
>> --- a/fs/io-wq.c
>> +++ b/fs/io-wq.c
>> @@ -16,7 +16,6 @@
>> #include
>> #include
>> #include
>> -#include
>>
>> #include "../kernel/sched/sched.h"
>> #include "io-wq.h"
>> @@ -503,10 +502,16 @@ static int io_wqe_worker(void *data)
>> if (io_flush_signals())
>> continue;
>> ret = schedule_timeout(WORKER_IDLE_TIMEOUT);
>> -if (try_to_freeze() || ret)
>> +if (signal_pending(current)) {
>> +struct ksignal ksig;
>> +
>> +if (fatal_signal_pending(current))
>> +break;
>> +if (get_signal(&ksig))
>> +continue;
> ^^
>
> That is wrong. You are promising to deliver a signal to signal
> handler and them simply discarding it. Perhaps:
>
> if (!get_signal(&ksig))
> continue;
> WARN_ON(!sig_kernel_stop(ksig->sig));
> break;
Thanks, updated.
>>>
>>> Gah. Kill the WARN_ON.
>>>
>>> I was thinking "WARN_ON(!sig_kernel_fatal(ksig->sig));"
>>> The function sig_kernel_fatal does not exist.
>>>
>>> Fatal is the state that is left when a signal is neither
>>> ignored nor a stop signal, and does not have a handler.
>>>
>>> The rest of the logic still works.
>>
>> I've just come to the same conclusion myself after testing it.
>> Of the 3 cases, most of them can do the continue, but doesn't
>> really matter with the way the loop is structured. Anyway, looks
>> like this now:
>
> This idiom in the code:
>> +if (signal_pending(current)) {
>> +struct ksignal ksig;
>> +
>> +if (fatal_signal_pending(current))
>> +break;
>> +if (!get_signal(&ksig))
>> +continue;
>> }
>
> Needs to be:
>> +if (signal_pending(current)) {
>> +struct ksignal ksig;
>> +
>> +if (!get_signal(&ksig))
>> +continue;
>> +break;
>> }
>
> Because any signal returned from get_signal is fatal in this case.
> It might make sense to "WARN_ON(ksig->ka.sa.sa_handler != SIG_DFL)".
> As the io workers don't handle that case.
>
> It won't happen because you have everything blocked.
>
> The extra fatal_signal_pending(current) logic is just confusing in this
> case.
OK good point, and follows the same logic even if it won't make a
difference in my case. I'll make the change.
--
Jens Axboe
Re: [PATCH 2/7] io_uring: handle signals for IO threads like a normal thread
Jens Axboe writes:
> On 3/26/21 4:23 PM, Eric W. Biederman wrote:
>> Jens Axboe writes:
>>
>>> On 3/26/21 2:29 PM, Eric W. Biederman wrote:
Jens Axboe writes:
> We go through various hoops to disallow signals for the IO threads, but
> there's really no reason why we cannot just allow them. The IO threads
> never return to userspace like a normal thread, and hence don't go through
> normal signal processing. Instead, just check for a pending signal as part
> of the work loop, and call get_signal() to handle it for us if anything
> is pending.
>
> With that, we can support receiving signals, including special ones like
> SIGSTOP.
>
> Signed-off-by: Jens Axboe
> ---
> fs/io-wq.c| 24 +---
> fs/io_uring.c | 12
> 2 files changed, 25 insertions(+), 11 deletions(-)
>
> diff --git a/fs/io-wq.c b/fs/io-wq.c
> index b7c1fa932cb3..3e2f059a1737 100644
> --- a/fs/io-wq.c
> +++ b/fs/io-wq.c
> @@ -16,7 +16,6 @@
> #include
> #include
> #include
> -#include
>
> #include "../kernel/sched/sched.h"
> #include "io-wq.h"
> @@ -503,10 +502,16 @@ static int io_wqe_worker(void *data)
> if (io_flush_signals())
> continue;
> ret = schedule_timeout(WORKER_IDLE_TIMEOUT);
> - if (try_to_freeze() || ret)
> + if (signal_pending(current)) {
> + struct ksignal ksig;
> +
> + if (fatal_signal_pending(current))
> + break;
> + if (get_signal(&ksig))
> + continue;
^^
That is wrong. You are promising to deliver a signal to signal
handler and them simply discarding it. Perhaps:
if (!get_signal(&ksig))
continue;
WARN_ON(!sig_kernel_stop(ksig->sig));
break;
>>>
>>> Thanks, updated.
>>
>> Gah. Kill the WARN_ON.
>>
>> I was thinking "WARN_ON(!sig_kernel_fatal(ksig->sig));"
>> The function sig_kernel_fatal does not exist.
>>
>> Fatal is the state that is left when a signal is neither
>> ignored nor a stop signal, and does not have a handler.
>>
>> The rest of the logic still works.
>
> I've just come to the same conclusion myself after testing it.
> Of the 3 cases, most of them can do the continue, but doesn't
> really matter with the way the loop is structured. Anyway, looks
> like this now:
This idiom in the code:
> + if (signal_pending(current)) {
> + struct ksignal ksig;
> +
> + if (fatal_signal_pending(current))
> + break;
> + if (!get_signal(&ksig))
> + continue;
> }
Needs to be:
> + if (signal_pending(current)) {
> + struct ksignal ksig;
> +
> + if (!get_signal(&ksig))
> + continue;
> + break;
> }
Because any signal returned from get_signal is fatal in this case.
It might make sense to "WARN_ON(ksig->ka.sa.sa_handler != SIG_DFL)".
As the io workers don't handle that case.
It won't happen because you have everything blocked.
The extra fatal_signal_pending(current) logic is just confusing in this
case.
Eric
Re: [PATCH 2/7] io_uring: handle signals for IO threads like a normal thread
On 3/26/21 4:23 PM, Eric W. Biederman wrote:
> Jens Axboe writes:
>
>> On 3/26/21 2:29 PM, Eric W. Biederman wrote:
>>> Jens Axboe writes:
>>>
We go through various hoops to disallow signals for the IO threads, but
there's really no reason why we cannot just allow them. The IO threads
never return to userspace like a normal thread, and hence don't go through
normal signal processing. Instead, just check for a pending signal as part
of the work loop, and call get_signal() to handle it for us if anything
is pending.
With that, we can support receiving signals, including special ones like
SIGSTOP.
Signed-off-by: Jens Axboe
---
fs/io-wq.c| 24 +---
fs/io_uring.c | 12
2 files changed, 25 insertions(+), 11 deletions(-)
diff --git a/fs/io-wq.c b/fs/io-wq.c
index b7c1fa932cb3..3e2f059a1737 100644
--- a/fs/io-wq.c
+++ b/fs/io-wq.c
@@ -16,7 +16,6 @@
#include
#include
#include
-#include
#include "../kernel/sched/sched.h"
#include "io-wq.h"
@@ -503,10 +502,16 @@ static int io_wqe_worker(void *data)
if (io_flush_signals())
continue;
ret = schedule_timeout(WORKER_IDLE_TIMEOUT);
- if (try_to_freeze() || ret)
+ if (signal_pending(current)) {
+ struct ksignal ksig;
+
+ if (fatal_signal_pending(current))
+ break;
+ if (get_signal(&ksig))
+ continue;
>>> ^^
>>>
>>> That is wrong. You are promising to deliver a signal to signal
>>> handler and them simply discarding it. Perhaps:
>>>
>>> if (!get_signal(&ksig))
>>> continue;
>>> WARN_ON(!sig_kernel_stop(ksig->sig));
>>> break;
>>
>> Thanks, updated.
>
> Gah. Kill the WARN_ON.
>
> I was thinking "WARN_ON(!sig_kernel_fatal(ksig->sig));"
> The function sig_kernel_fatal does not exist.
>
> Fatal is the state that is left when a signal is neither
> ignored nor a stop signal, and does not have a handler.
>
> The rest of the logic still works.
I've just come to the same conclusion myself after testing it.
Of the 3 cases, most of them can do the continue, but doesn't
really matter with the way the loop is structured. Anyway, looks
like this now:
commit 769186e30cd437f5e1a000e7cf00286948779da4
Author: Jens Axboe
Date: Thu Mar 25 18:16:06 2021 -0600
io_uring: handle signals for IO threads like a normal thread
We go through various hoops to disallow signals for the IO threads, but
there's really no reason why we cannot just allow them. The IO threads
never return to userspace like a normal thread, and hence don't go through
normal signal processing. Instead, just check for a pending signal as part
of the work loop, and call get_signal() to handle it for us if anything
is pending.
With that, we can support receiving signals, including special ones like
SIGSTOP.
Signed-off-by: Jens Axboe
diff --git a/fs/io-wq.c b/fs/io-wq.c
index b7c1fa932cb3..7e5970c8b0be 100644
--- a/fs/io-wq.c
+++ b/fs/io-wq.c
@@ -16,7 +16,6 @@
#include
#include
#include
-#include
#include "../kernel/sched/sched.h"
#include "io-wq.h"
@@ -503,10 +502,16 @@ static int io_wqe_worker(void *data)
if (io_flush_signals())
continue;
ret = schedule_timeout(WORKER_IDLE_TIMEOUT);
- if (try_to_freeze() || ret)
+ if (signal_pending(current)) {
+ struct ksignal ksig;
+
+ if (fatal_signal_pending(current))
+ break;
+ if (!get_signal(&ksig))
+ continue;
+ }
+ if (ret)
continue;
- if (fatal_signal_pending(current))
- break;
/* timed out, exit unless we're the fixed worker */
if (test_bit(IO_WQ_BIT_EXIT, &wq->state) ||
!(worker->flags & IO_WORKER_F_FIXED))
@@ -714,9 +719,15 @@ static int io_wq_manager(void *data)
set_current_state(TASK_INTERRUPTIBLE);
io_wq_check_workers(wq);
schedule_timeout(HZ);
- try_to_freeze();
- if (fatal_signal_pending(current))
- set_bit(IO_WQ_BIT_EXIT, &wq->state);
+ if (signal_pending(current)) {
+ struct ksignal ksig;
+
+ if (fatal_signal_pending(current)) {
+ set_bit(IO_WQ_BIT_EXIT, &wq->state);
+ continue;
+ }
+
Re: [PATCH 2/7] io_uring: handle signals for IO threads like a normal thread
Jens Axboe writes:
> On 3/26/21 2:29 PM, Eric W. Biederman wrote:
>> Jens Axboe writes:
>>
>>> We go through various hoops to disallow signals for the IO threads, but
>>> there's really no reason why we cannot just allow them. The IO threads
>>> never return to userspace like a normal thread, and hence don't go through
>>> normal signal processing. Instead, just check for a pending signal as part
>>> of the work loop, and call get_signal() to handle it for us if anything
>>> is pending.
>>>
>>> With that, we can support receiving signals, including special ones like
>>> SIGSTOP.
>>>
>>> Signed-off-by: Jens Axboe
>>> ---
>>> fs/io-wq.c| 24 +---
>>> fs/io_uring.c | 12
>>> 2 files changed, 25 insertions(+), 11 deletions(-)
>>>
>>> diff --git a/fs/io-wq.c b/fs/io-wq.c
>>> index b7c1fa932cb3..3e2f059a1737 100644
>>> --- a/fs/io-wq.c
>>> +++ b/fs/io-wq.c
>>> @@ -16,7 +16,6 @@
>>> #include
>>> #include
>>> #include
>>> -#include
>>>
>>> #include "../kernel/sched/sched.h"
>>> #include "io-wq.h"
>>> @@ -503,10 +502,16 @@ static int io_wqe_worker(void *data)
>>> if (io_flush_signals())
>>> continue;
>>> ret = schedule_timeout(WORKER_IDLE_TIMEOUT);
>>> - if (try_to_freeze() || ret)
>>> + if (signal_pending(current)) {
>>> + struct ksignal ksig;
>>> +
>>> + if (fatal_signal_pending(current))
>>> + break;
>>> + if (get_signal(&ksig))
>>> + continue;
>> ^^
>>
>> That is wrong. You are promising to deliver a signal to signal
>> handler and them simply discarding it. Perhaps:
>>
>> if (!get_signal(&ksig))
>> continue;
>> WARN_ON(!sig_kernel_stop(ksig->sig));
>> break;
>
> Thanks, updated.
Gah. Kill the WARN_ON.
I was thinking "WARN_ON(!sig_kernel_fatal(ksig->sig));"
The function sig_kernel_fatal does not exist.
Fatal is the state that is left when a signal is neither
ignored nor a stop signal, and does not have a handler.
The rest of the logic still works.
Eric
Re: [PATCH 2/7] io_uring: handle signals for IO threads like a normal thread
On 3/26/21 2:29 PM, Eric W. Biederman wrote:
> Jens Axboe writes:
>
>> We go through various hoops to disallow signals for the IO threads, but
>> there's really no reason why we cannot just allow them. The IO threads
>> never return to userspace like a normal thread, and hence don't go through
>> normal signal processing. Instead, just check for a pending signal as part
>> of the work loop, and call get_signal() to handle it for us if anything
>> is pending.
>>
>> With that, we can support receiving signals, including special ones like
>> SIGSTOP.
>>
>> Signed-off-by: Jens Axboe
>> ---
>> fs/io-wq.c| 24 +---
>> fs/io_uring.c | 12
>> 2 files changed, 25 insertions(+), 11 deletions(-)
>>
>> diff --git a/fs/io-wq.c b/fs/io-wq.c
>> index b7c1fa932cb3..3e2f059a1737 100644
>> --- a/fs/io-wq.c
>> +++ b/fs/io-wq.c
>> @@ -16,7 +16,6 @@
>> #include
>> #include
>> #include
>> -#include
>>
>> #include "../kernel/sched/sched.h"
>> #include "io-wq.h"
>> @@ -503,10 +502,16 @@ static int io_wqe_worker(void *data)
>> if (io_flush_signals())
>> continue;
>> ret = schedule_timeout(WORKER_IDLE_TIMEOUT);
>> -if (try_to_freeze() || ret)
>> +if (signal_pending(current)) {
>> +struct ksignal ksig;
>> +
>> +if (fatal_signal_pending(current))
>> +break;
>> +if (get_signal(&ksig))
>> +continue;
> ^^
>
> That is wrong. You are promising to deliver a signal to signal
> handler and them simply discarding it. Perhaps:
>
> if (!get_signal(&ksig))
> continue;
> WARN_ON(!sig_kernel_stop(ksig->sig));
> break;
Thanks, updated.
--
Jens Axboe
Re: [PATCH 2/7] io_uring: handle signals for IO threads like a normal thread
Jens Axboe writes:
> We go through various hoops to disallow signals for the IO threads, but
> there's really no reason why we cannot just allow them. The IO threads
> never return to userspace like a normal thread, and hence don't go through
> normal signal processing. Instead, just check for a pending signal as part
> of the work loop, and call get_signal() to handle it for us if anything
> is pending.
>
> With that, we can support receiving signals, including special ones like
> SIGSTOP.
>
> Signed-off-by: Jens Axboe
> ---
> fs/io-wq.c| 24 +---
> fs/io_uring.c | 12
> 2 files changed, 25 insertions(+), 11 deletions(-)
>
> diff --git a/fs/io-wq.c b/fs/io-wq.c
> index b7c1fa932cb3..3e2f059a1737 100644
> --- a/fs/io-wq.c
> +++ b/fs/io-wq.c
> @@ -16,7 +16,6 @@
> #include
> #include
> #include
> -#include
>
> #include "../kernel/sched/sched.h"
> #include "io-wq.h"
> @@ -503,10 +502,16 @@ static int io_wqe_worker(void *data)
> if (io_flush_signals())
> continue;
> ret = schedule_timeout(WORKER_IDLE_TIMEOUT);
> - if (try_to_freeze() || ret)
> + if (signal_pending(current)) {
> + struct ksignal ksig;
> +
> + if (fatal_signal_pending(current))
> + break;
> + if (get_signal(&ksig))
> + continue;
^^
That is wrong. You are promising to deliver a signal to signal
handler and them simply discarding it. Perhaps:
if (!get_signal(&ksig))
continue;
WARN_ON(!sig_kernel_stop(ksig->sig));
break;
Eric
[PATCH 2/7] io_uring: handle signals for IO threads like a normal thread
We go through various hoops to disallow signals for the IO threads, but
there's really no reason why we cannot just allow them. The IO threads
never return to userspace like a normal thread, and hence don't go through
normal signal processing. Instead, just check for a pending signal as part
of the work loop, and call get_signal() to handle it for us if anything
is pending.
With that, we can support receiving signals, including special ones like
SIGSTOP.
Signed-off-by: Jens Axboe
---
fs/io-wq.c| 24 +---
fs/io_uring.c | 12
2 files changed, 25 insertions(+), 11 deletions(-)
diff --git a/fs/io-wq.c b/fs/io-wq.c
index b7c1fa932cb3..3e2f059a1737 100644
--- a/fs/io-wq.c
+++ b/fs/io-wq.c
@@ -16,7 +16,6 @@
#include
#include
#include
-#include
#include "../kernel/sched/sched.h"
#include "io-wq.h"
@@ -503,10 +502,16 @@ static int io_wqe_worker(void *data)
if (io_flush_signals())
continue;
ret = schedule_timeout(WORKER_IDLE_TIMEOUT);
- if (try_to_freeze() || ret)
+ if (signal_pending(current)) {
+ struct ksignal ksig;
+
+ if (fatal_signal_pending(current))
+ break;
+ if (get_signal(&ksig))
+ continue;
+ }
+ if (ret)
continue;
- if (fatal_signal_pending(current))
- break;
/* timed out, exit unless we're the fixed worker */
if (test_bit(IO_WQ_BIT_EXIT, &wq->state) ||
!(worker->flags & IO_WORKER_F_FIXED))
@@ -714,9 +719,14 @@ static int io_wq_manager(void *data)
set_current_state(TASK_INTERRUPTIBLE);
io_wq_check_workers(wq);
schedule_timeout(HZ);
- try_to_freeze();
- if (fatal_signal_pending(current))
- set_bit(IO_WQ_BIT_EXIT, &wq->state);
+ if (signal_pending(current)) {
+ struct ksignal ksig;
+
+ if (fatal_signal_pending(current))
+ set_bit(IO_WQ_BIT_EXIT, &wq->state);
+ else if (get_signal(&ksig))
+ continue;
+ }
} while (!test_bit(IO_WQ_BIT_EXIT, &wq->state));
io_wq_check_workers(wq);
diff --git a/fs/io_uring.c b/fs/io_uring.c
index 54ea561db4a5..350418a88db3 100644
--- a/fs/io_uring.c
+++ b/fs/io_uring.c
@@ -78,7 +78,6 @@
#include
#include
#include
-#include
#define CREATE_TRACE_POINTS
#include
@@ -6765,8 +6764,14 @@ static int io_sq_thread(void *data)
timeout = jiffies + sqd->sq_thread_idle;
continue;
}
- if (fatal_signal_pending(current))
- break;
+ if (signal_pending(current)) {
+ struct ksignal ksig;
+
+ if (fatal_signal_pending(current))
+ break;
+ if (get_signal(&ksig))
+ continue;
+ }
sqt_spin = false;
cap_entries = !list_is_singular(&sqd->ctx_list);
list_for_each_entry(ctx, &sqd->ctx_list, sqd_list) {
@@ -6809,7 +6814,6 @@ static int io_sq_thread(void *data)
mutex_unlock(&sqd->lock);
schedule();
- try_to_freeze();
mutex_lock(&sqd->lock);
list_for_each_entry(ctx, &sqd->ctx_list, sqd_list)
io_ring_clear_wakeup_flag(ctx);
--
2.31.0
