* [PATCH v2 0/2] fix task_work interation with freezing
@ 2024-07-09 14:27 Pavel Begunkov
2024-07-09 14:27 ` [PATCH v2 1/2] io_uring/io-wq: limit retrying worker initialisation Pavel Begunkov
2024-07-09 14:27 ` [PATCH v2 2/2] kernel: rerun task_work while freezing in get_signal() Pavel Begunkov
0 siblings, 2 replies; 6+ messages in thread
From: Pavel Begunkov @ 2024-07-09 14:27 UTC (permalink / raw)
To: io-uring
Cc: Jens Axboe, asml.silence, Oleg Nesterov, Andrew Morton,
Christian Brauner, Tycho Andersen, Thomas Gleixner, linux-kernel,
Julian Orth, Peter Zijlstra, Tejun Heo
It's reported [1] that a task_work queued at a wrong time can prevent
freezing and make the tasks to spin in get_signal() taking 100%
of CPU. Patch 1 is a preparation. Patch 2 addresses the issue.
[1] https://github.com/systemd/systemd/issues/33626
v2: move task_work_run() into do_freezer_trap()
change Fixes tag
Pavel Begunkov (2):
io_uring/io-wq: limit retrying worker initialisation
kernel: rerun task_work while freezing in get_signal()
io_uring/io-wq.c | 10 +++++++---
kernel/signal.c | 8 ++++++++
2 files changed, 15 insertions(+), 3 deletions(-)
--
2.44.0
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH v2 1/2] io_uring/io-wq: limit retrying worker initialisation
2024-07-09 14:27 [PATCH v2 0/2] fix task_work interation with freezing Pavel Begunkov
@ 2024-07-09 14:27 ` Pavel Begunkov
2024-07-09 14:27 ` [PATCH v2 2/2] kernel: rerun task_work while freezing in get_signal() Pavel Begunkov
1 sibling, 0 replies; 6+ messages in thread
From: Pavel Begunkov @ 2024-07-09 14:27 UTC (permalink / raw)
To: io-uring
Cc: Jens Axboe, asml.silence, Oleg Nesterov, Andrew Morton,
Christian Brauner, Tycho Andersen, Thomas Gleixner, linux-kernel,
Julian Orth, Peter Zijlstra, Tejun Heo
If io-wq worker creation fails, we retry it by queueing up a task_work.
tasK_work is needed because it should be done from the user process
context. The problem is that retries are not limited, and if queueing a
task_work is the reason for the failure, we might get into an infinite
loop.
It doesn't seem to happen now but it will with the following patch
executing task_work in the freezing loop. For now, arbitrarily limit the
number of attempts to create a worker.
Cc: [email protected]
Fixes: 3146cba99aa28 ("io-wq: make worker creation resilient against signals")
Reported-by: Julian Orth <[email protected]>
Signed-off-by: Pavel Begunkov <[email protected]>
---
io_uring/io-wq.c | 10 +++++++---
1 file changed, 7 insertions(+), 3 deletions(-)
diff --git a/io_uring/io-wq.c b/io_uring/io-wq.c
index 913c92249522..f1e7c670add8 100644
--- a/io_uring/io-wq.c
+++ b/io_uring/io-wq.c
@@ -23,6 +23,7 @@
#include "io_uring.h"
#define WORKER_IDLE_TIMEOUT (5 * HZ)
+#define WORKER_INIT_LIMIT 3
enum {
IO_WORKER_F_UP = 0, /* up and active */
@@ -58,6 +59,7 @@ struct io_worker {
unsigned long create_state;
struct callback_head create_work;
+ int init_retries;
union {
struct rcu_head rcu;
@@ -745,7 +747,7 @@ static bool io_wq_work_match_all(struct io_wq_work *work, void *data)
return true;
}
-static inline bool io_should_retry_thread(long err)
+static inline bool io_should_retry_thread(struct io_worker *worker, long err)
{
/*
* Prevent perpetual task_work retry, if the task (or its group) is
@@ -753,6 +755,8 @@ static inline bool io_should_retry_thread(long err)
*/
if (fatal_signal_pending(current))
return false;
+ if (worker->init_retries++ >= WORKER_INIT_LIMIT)
+ return false;
switch (err) {
case -EAGAIN:
@@ -779,7 +783,7 @@ static void create_worker_cont(struct callback_head *cb)
io_init_new_worker(wq, worker, tsk);
io_worker_release(worker);
return;
- } else if (!io_should_retry_thread(PTR_ERR(tsk))) {
+ } else if (!io_should_retry_thread(worker, PTR_ERR(tsk))) {
struct io_wq_acct *acct = io_wq_get_acct(worker);
atomic_dec(&acct->nr_running);
@@ -846,7 +850,7 @@ static bool create_io_worker(struct io_wq *wq, int index)
tsk = create_io_thread(io_wq_worker, worker, NUMA_NO_NODE);
if (!IS_ERR(tsk)) {
io_init_new_worker(wq, worker, tsk);
- } else if (!io_should_retry_thread(PTR_ERR(tsk))) {
+ } else if (!io_should_retry_thread(worker, PTR_ERR(tsk))) {
kfree(worker);
goto fail;
} else {
--
2.44.0
^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH v2 2/2] kernel: rerun task_work while freezing in get_signal()
2024-07-09 14:27 [PATCH v2 0/2] fix task_work interation with freezing Pavel Begunkov
2024-07-09 14:27 ` [PATCH v2 1/2] io_uring/io-wq: limit retrying worker initialisation Pavel Begunkov
@ 2024-07-09 14:27 ` Pavel Begunkov
2024-07-09 14:42 ` Oleg Nesterov
2024-07-10 0:57 ` Tejun Heo
1 sibling, 2 replies; 6+ messages in thread
From: Pavel Begunkov @ 2024-07-09 14:27 UTC (permalink / raw)
To: io-uring
Cc: Jens Axboe, asml.silence, Oleg Nesterov, Andrew Morton,
Christian Brauner, Tycho Andersen, Thomas Gleixner, linux-kernel,
Julian Orth, Peter Zijlstra, Tejun Heo
io_uring can asynchronously add a task_work while the task is getting
freezed. TIF_NOTIFY_SIGNAL will prevent the task from sleeping in
do_freezer_trap(), and since the get_signal()'s relock loop doesn't
retry task_work, the task will spin there not being able to sleep
until the freezing is cancelled / the task is killed / etc.
Cc: [email protected]
Link: https://github.com/systemd/systemd/issues/33626
Fixes: 12db8b690010c ("entry: Add support for TIF_NOTIFY_SIGNAL")
Reported-by: Julian Orth <[email protected]>
Signed-off-by: Pavel Begunkov <[email protected]>
---
kernel/signal.c | 8 ++++++++
1 file changed, 8 insertions(+)
diff --git a/kernel/signal.c b/kernel/signal.c
index 1f9dd41c04be..60c737e423a1 100644
--- a/kernel/signal.c
+++ b/kernel/signal.c
@@ -2600,6 +2600,14 @@ static void do_freezer_trap(void)
spin_unlock_irq(¤t->sighand->siglock);
cgroup_enter_frozen();
schedule();
+
+ /*
+ * We could've been woken by task_work, run it to clear
+ * TIF_NOTIFY_SIGNAL. The caller will retry if necessary.
+ */
+ clear_notify_signal();
+ if (unlikely(task_work_pending(current)))
+ task_work_run();
}
static int ptrace_signal(int signr, kernel_siginfo_t *info, enum pid_type type)
--
2.44.0
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH v2 2/2] kernel: rerun task_work while freezing in get_signal()
2024-07-09 14:27 ` [PATCH v2 2/2] kernel: rerun task_work while freezing in get_signal() Pavel Begunkov
@ 2024-07-09 14:42 ` Oleg Nesterov
2024-07-10 0:57 ` Tejun Heo
1 sibling, 0 replies; 6+ messages in thread
From: Oleg Nesterov @ 2024-07-09 14:42 UTC (permalink / raw)
To: Pavel Begunkov
Cc: io-uring, Jens Axboe, Andrew Morton, Christian Brauner,
Tycho Andersen, Thomas Gleixner, linux-kernel, Julian Orth,
Peter Zijlstra, Tejun Heo
On 07/09, Pavel Begunkov wrote:
>
> --- a/kernel/signal.c
> +++ b/kernel/signal.c
> @@ -2600,6 +2600,14 @@ static void do_freezer_trap(void)
> spin_unlock_irq(¤t->sighand->siglock);
> cgroup_enter_frozen();
> schedule();
> +
> + /*
> + * We could've been woken by task_work, run it to clear
> + * TIF_NOTIFY_SIGNAL. The caller will retry if necessary.
> + */
> + clear_notify_signal();
> + if (unlikely(task_work_pending(current)))
> + task_work_run();
> }
Acked-by: Oleg Nesterov <[email protected]>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2 2/2] kernel: rerun task_work while freezing in get_signal()
2024-07-09 14:27 ` [PATCH v2 2/2] kernel: rerun task_work while freezing in get_signal() Pavel Begunkov
2024-07-09 14:42 ` Oleg Nesterov
@ 2024-07-10 0:57 ` Tejun Heo
2024-07-10 17:55 ` Pavel Begunkov
1 sibling, 1 reply; 6+ messages in thread
From: Tejun Heo @ 2024-07-10 0:57 UTC (permalink / raw)
To: Pavel Begunkov
Cc: io-uring, Jens Axboe, Oleg Nesterov, Andrew Morton,
Christian Brauner, Tycho Andersen, Thomas Gleixner, linux-kernel,
Julian Orth, Peter Zijlstra
On Tue, Jul 09, 2024 at 03:27:19PM +0100, Pavel Begunkov wrote:
> io_uring can asynchronously add a task_work while the task is getting
> freezed. TIF_NOTIFY_SIGNAL will prevent the task from sleeping in
> do_freezer_trap(), and since the get_signal()'s relock loop doesn't
> retry task_work, the task will spin there not being able to sleep
> until the freezing is cancelled / the task is killed / etc.
>
> Cc: [email protected]
> Link: https://github.com/systemd/systemd/issues/33626
> Fixes: 12db8b690010c ("entry: Add support for TIF_NOTIFY_SIGNAL")
> Reported-by: Julian Orth <[email protected]>
> Signed-off-by: Pavel Begunkov <[email protected]>
I haven't looked at the signal code for too long to be all that useful but
the problem described and the patch does make sense to me. FWIW,
Acked-by: Tejun Heo <[email protected]>
Maybe note that this is structured specifically to ease backport and we need
further cleanups? It's not great that this is special cased in
do_freezer_trap() instead of being integrated into the outer loop.
Thanks.
--
tejun
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2 2/2] kernel: rerun task_work while freezing in get_signal()
2024-07-10 0:57 ` Tejun Heo
@ 2024-07-10 17:55 ` Pavel Begunkov
0 siblings, 0 replies; 6+ messages in thread
From: Pavel Begunkov @ 2024-07-10 17:55 UTC (permalink / raw)
To: Tejun Heo
Cc: io-uring, Jens Axboe, Oleg Nesterov, Andrew Morton,
Christian Brauner, Tycho Andersen, Thomas Gleixner, linux-kernel,
Julian Orth, Peter Zijlstra
On 7/10/24 01:57, Tejun Heo wrote:
> On Tue, Jul 09, 2024 at 03:27:19PM +0100, Pavel Begunkov wrote:
>> io_uring can asynchronously add a task_work while the task is getting
>> freezed. TIF_NOTIFY_SIGNAL will prevent the task from sleeping in
>> do_freezer_trap(), and since the get_signal()'s relock loop doesn't
>> retry task_work, the task will spin there not being able to sleep
>> until the freezing is cancelled / the task is killed / etc.
>>
>> Cc: [email protected]
>> Link: https://github.com/systemd/systemd/issues/33626
>> Fixes: 12db8b690010c ("entry: Add support for TIF_NOTIFY_SIGNAL")
>> Reported-by: Julian Orth <[email protected]>
>> Signed-off-by: Pavel Begunkov <[email protected]>
>
> I haven't looked at the signal code for too long to be all that useful but
> the problem described and the patch does make sense to me. FWIW,
>
> Acked-by: Tejun Heo <[email protected]>
>
> Maybe note that this is structured specifically to ease backport and we need
> further cleanups? It's not great that this is special cased in
I'll add a couple of words
> do_freezer_trap() instead of being integrated into the outer loop.
v1 had it in the common loop, but might actually be good it's
limited to freezing, need more digging.
--
Pavel Begunkov
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2024-07-10 17:55 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-07-09 14:27 [PATCH v2 0/2] fix task_work interation with freezing Pavel Begunkov
2024-07-09 14:27 ` [PATCH v2 1/2] io_uring/io-wq: limit retrying worker initialisation Pavel Begunkov
2024-07-09 14:27 ` [PATCH v2 2/2] kernel: rerun task_work while freezing in get_signal() Pavel Begunkov
2024-07-09 14:42 ` Oleg Nesterov
2024-07-10 0:57 ` Tejun Heo
2024-07-10 17:55 ` Pavel Begunkov
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox