* [PATCH] livepatch: klp_send_signal should treat PF_IO_WORKER like PF_KTHREAD @ 2021-03-25 1:48 Dong Kai 2021-03-25 2:51 ` Joe Lawrence ` (2 more replies) 0 siblings, 3 replies; 9+ messages in thread From: Dong Kai @ 2021-03-25 1:48 UTC (permalink / raw) To: jpoimboe, jikos, mbenes, pmladek, joe.lawrence Cc: axboe, live-patching, linux-kernel commit 15b2219facad ("kernel: freezer should treat PF_IO_WORKER like PF_KTHREAD for freezing") is to fix the freezeing issue of IO threads by making the freezer not send them fake signals. Here live patching consistency model call klp_send_signals to wake up all tasks by send fake signal to all non-kthread which only check the PF_KTHREAD flag, so it still send signal to io threads which may lead to freezeing issue of io threads. Here we take the same fix action by treating PF_IO_WORKERS as PF_KTHREAD within klp_send_signal function. Signed-off-by: Dong Kai <dongkai11@huawei.com> --- note: the io threads freeze issue links: [1] https://lore.kernel.org/io-uring/YEgnIp43%2F6kFn8GL@kevinlocke.name/ [2] https://lore.kernel.org/io-uring/d7350ce7-17dc-75d7-611b-27ebf2cb539b@kernel.dk/ kernel/livepatch/transition.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/kernel/livepatch/transition.c b/kernel/livepatch/transition.c index f6310f848f34..0e1c35c8f4b4 100644 --- a/kernel/livepatch/transition.c +++ b/kernel/livepatch/transition.c @@ -358,7 +358,7 @@ static void klp_send_signals(void) * Meanwhile the task could migrate itself and the action * would be meaningless. It is not serious though. */ - if (task->flags & PF_KTHREAD) { + if (task->flags & (PF_KTHREAD | PF_IO_WORKER)) { /* * Wake up a kthread which sleeps interruptedly and * still has not been migrated. -- 2.17.1 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH] livepatch: klp_send_signal should treat PF_IO_WORKER like PF_KTHREAD 2021-03-25 1:48 [PATCH] livepatch: klp_send_signal should treat PF_IO_WORKER like PF_KTHREAD Dong Kai @ 2021-03-25 2:51 ` Joe Lawrence 2021-03-25 9:30 ` Miroslav Benes 2021-03-25 9:26 ` Miroslav Benes 2021-03-25 16:29 ` Jens Axboe 2 siblings, 1 reply; 9+ messages in thread From: Joe Lawrence @ 2021-03-25 2:51 UTC (permalink / raw) To: Dong Kai, jpoimboe, jikos, mbenes, pmladek Cc: axboe, live-patching, linux-kernel On 3/24/21 9:48 PM, Dong Kai wrote: > commit 15b2219facad ("kernel: freezer should treat PF_IO_WORKER like > PF_KTHREAD for freezing") is to fix the freezeing issue of IO threads nit: s/freezeing/freezing > by making the freezer not send them fake signals. > > Here live patching consistency model call klp_send_signals to wake up > all tasks by send fake signal to all non-kthread which only check the > PF_KTHREAD flag, so it still send signal to io threads which may lead to > freezeing issue of io threads. > > Here we take the same fix action by treating PF_IO_WORKERS as PF_KTHREAD > within klp_send_signal function. > > Signed-off-by: Dong Kai <dongkai11@huawei.com> > --- > note: > the io threads freeze issue links: > [1] https://lore.kernel.org/io-uring/YEgnIp43%2F6kFn8GL@kevinlocke.name/ > [2] https://lore.kernel.org/io-uring/d7350ce7-17dc-75d7-611b-27ebf2cb539b@kernel.dk/ > > kernel/livepatch/transition.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/kernel/livepatch/transition.c b/kernel/livepatch/transition.c > index f6310f848f34..0e1c35c8f4b4 100644 > --- a/kernel/livepatch/transition.c > +++ b/kernel/livepatch/transition.c > @@ -358,7 +358,7 @@ static void klp_send_signals(void) > * Meanwhile the task could migrate itself and the action > * would be meaningless. It is not serious though. > */ > - if (task->flags & PF_KTHREAD) { > + if (task->flags & (PF_KTHREAD | PF_IO_WORKER)) { > /* > * Wake up a kthread which sleeps interruptedly and > * still has not been migrated. > (PF_KTHREAD | PF_IO_WORKER) is open coded in soo many places maybe this is a silly question, but... If the livepatch code could use fake_signal_wake_up(), we could consolidate the pattern in klp_send_signals() with the one in freeze_task(). Then there would only one place for wake up / fake signal logic. I don't fully understand the differences in the freeze_task() version, so I only pose this as a question and not v2 request. As it is, this change seems logical to me, so: Acked-by: Joe Lawrence <joe.lawrence@redhat.com> Thanks, -- Joe ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] livepatch: klp_send_signal should treat PF_IO_WORKER like PF_KTHREAD 2021-03-25 2:51 ` Joe Lawrence @ 2021-03-25 9:30 ` Miroslav Benes 2021-03-25 16:30 ` Jens Axboe 0 siblings, 1 reply; 9+ messages in thread From: Miroslav Benes @ 2021-03-25 9:30 UTC (permalink / raw) To: Joe Lawrence Cc: Dong Kai, jpoimboe, jikos, pmladek, axboe, live-patching, linux-kernel > (PF_KTHREAD | PF_IO_WORKER) is open coded in soo many places maybe this is a > silly question, but... > > If the livepatch code could use fake_signal_wake_up(), we could consolidate > the pattern in klp_send_signals() with the one in freeze_task(). Then there > would only one place for wake up / fake signal logic. > > I don't fully understand the differences in the freeze_task() version, so I > only pose this as a question and not v2 request. The plan was to remove our live patching fake signal completely and use the new infrastructure Jens proposed in the past. Something like diff --git a/kernel/livepatch/transition.c b/kernel/livepatch/transition.c index f6310f848f34..3a4beb9395c4 100644 --- a/kernel/livepatch/transition.c +++ b/kernel/livepatch/transition.c @@ -9,6 +9,7 @@ #include <linux/cpu.h> #include <linux/stacktrace.h> +#include <linux/tracehook.h> #include "core.h" #include "patch.h" #include "transition.h" @@ -369,9 +370,7 @@ static void klp_send_signals(void) * Send fake signal to all non-kthread tasks which are * still not migrated. */ - spin_lock_irq(&task->sighand->siglock); - signal_wake_up(task, 0); - spin_unlock_irq(&task->sighand->siglock); + set_notify_signal(task); } } read_unlock(&tasklist_lock); diff --git a/kernel/signal.c b/kernel/signal.c index a15c584a0455..b7cf4eda8611 100644 --- a/kernel/signal.c +++ b/kernel/signal.c @@ -181,8 +181,7 @@ void recalc_sigpending_and_wake(struct task_struct *t) void recalc_sigpending(void) { - if (!recalc_sigpending_tsk(current) && !freezing(current) && - !klp_patch_pending(current)) + if (!recalc_sigpending_tsk(current) && !freezing(current)) clear_thread_flag(TIF_SIGPENDING); } Let me verify it still works and there are all the needed pieces merged for all the architectures we support (x86_64, ppc64le and s390x). I'll send a proper patch then. Miroslav ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH] livepatch: klp_send_signal should treat PF_IO_WORKER like PF_KTHREAD 2021-03-25 9:30 ` Miroslav Benes @ 2021-03-25 16:30 ` Jens Axboe 2021-03-26 8:39 ` Miroslav Benes 0 siblings, 1 reply; 9+ messages in thread From: Jens Axboe @ 2021-03-25 16:30 UTC (permalink / raw) To: Miroslav Benes, Joe Lawrence Cc: Dong Kai, jpoimboe, jikos, pmladek, live-patching, linux-kernel On 3/25/21 3:30 AM, Miroslav Benes wrote: >> (PF_KTHREAD | PF_IO_WORKER) is open coded in soo many places maybe this is a >> silly question, but... >> >> If the livepatch code could use fake_signal_wake_up(), we could consolidate >> the pattern in klp_send_signals() with the one in freeze_task(). Then there >> would only one place for wake up / fake signal logic. >> >> I don't fully understand the differences in the freeze_task() version, so I >> only pose this as a question and not v2 request. > > The plan was to remove our live patching fake signal completely and use > the new infrastructure Jens proposed in the past. That would be great, I've actually been waiting for that to show up! I would greatly prefer this approach if you deem it suitable for 5.12, if not we'll still need the temporary work-around for live patching. -- Jens Axboe ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] livepatch: klp_send_signal should treat PF_IO_WORKER like PF_KTHREAD 2021-03-25 16:30 ` Jens Axboe @ 2021-03-26 8:39 ` Miroslav Benes 0 siblings, 0 replies; 9+ messages in thread From: Miroslav Benes @ 2021-03-26 8:39 UTC (permalink / raw) To: Jens Axboe Cc: Joe Lawrence, Dong Kai, jpoimboe, jikos, pmladek, live-patching, linux-kernel On Thu, 25 Mar 2021, Jens Axboe wrote: > On 3/25/21 3:30 AM, Miroslav Benes wrote: > >> (PF_KTHREAD | PF_IO_WORKER) is open coded in soo many places maybe this is a > >> silly question, but... > >> > >> If the livepatch code could use fake_signal_wake_up(), we could consolidate > >> the pattern in klp_send_signals() with the one in freeze_task(). Then there > >> would only one place for wake up / fake signal logic. > >> > >> I don't fully understand the differences in the freeze_task() version, so I > >> only pose this as a question and not v2 request. > > > > The plan was to remove our live patching fake signal completely and use > > the new infrastructure Jens proposed in the past. > > That would be great, I've actually been waiting for that to show up! Sorry about that. I failed to notice that the infrastructure was merged already. I'll send it soonish. > I would greatly prefer this approach if you deem it suitable for 5.12, > if not we'll still need the temporary work-around for live patching. I noticed there is 20210326003928.978750-1-axboe@kernel.dk now, so I suppose we should wait for that to land in mainline and simply do nothing about PF_IO_WORKER for live patching. Miroslav ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] livepatch: klp_send_signal should treat PF_IO_WORKER like PF_KTHREAD 2021-03-25 1:48 [PATCH] livepatch: klp_send_signal should treat PF_IO_WORKER like PF_KTHREAD Dong Kai 2021-03-25 2:51 ` Joe Lawrence @ 2021-03-25 9:26 ` Miroslav Benes 2021-03-25 16:43 ` Joe Lawrence 2021-03-26 6:31 ` dongkai (H) 2021-03-25 16:29 ` Jens Axboe 2 siblings, 2 replies; 9+ messages in thread From: Miroslav Benes @ 2021-03-25 9:26 UTC (permalink / raw) To: Dong Kai Cc: jpoimboe, jikos, pmladek, joe.lawrence, axboe, live-patching, linux-kernel On Thu, 25 Mar 2021, Dong Kai wrote: > commit 15b2219facad ("kernel: freezer should treat PF_IO_WORKER like > PF_KTHREAD for freezing") is to fix the freezeing issue of IO threads > by making the freezer not send them fake signals. > > Here live patching consistency model call klp_send_signals to wake up > all tasks by send fake signal to all non-kthread which only check the > PF_KTHREAD flag, so it still send signal to io threads which may lead to > freezeing issue of io threads. I suppose this could happen, but it will also affect the live patching transition if the io threads do not react to signals. Are you able to reproduce it easily? I mean, is there a testcase I could use to take a closer look? > Here we take the same fix action by treating PF_IO_WORKERS as PF_KTHREAD > within klp_send_signal function. Yes, this sounds reasonable. Miroslav > Signed-off-by: Dong Kai <dongkai11@huawei.com> > --- > note: > the io threads freeze issue links: > [1] https://lore.kernel.org/io-uring/YEgnIp43%2F6kFn8GL@kevinlocke.name/ > [2] https://lore.kernel.org/io-uring/d7350ce7-17dc-75d7-611b-27ebf2cb539b@kernel.dk/ > > kernel/livepatch/transition.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/kernel/livepatch/transition.c b/kernel/livepatch/transition.c > index f6310f848f34..0e1c35c8f4b4 100644 > --- a/kernel/livepatch/transition.c > +++ b/kernel/livepatch/transition.c > @@ -358,7 +358,7 @@ static void klp_send_signals(void) > * Meanwhile the task could migrate itself and the action > * would be meaningless. It is not serious though. > */ > - if (task->flags & PF_KTHREAD) { > + if (task->flags & (PF_KTHREAD | PF_IO_WORKER)) { > /* > * Wake up a kthread which sleeps interruptedly and > * still has not been migrated. ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] livepatch: klp_send_signal should treat PF_IO_WORKER like PF_KTHREAD 2021-03-25 9:26 ` Miroslav Benes @ 2021-03-25 16:43 ` Joe Lawrence 2021-03-26 6:31 ` dongkai (H) 1 sibling, 0 replies; 9+ messages in thread From: Joe Lawrence @ 2021-03-25 16:43 UTC (permalink / raw) To: Miroslav Benes, Dong Kai Cc: jpoimboe, jikos, pmladek, axboe, live-patching, linux-kernel On 3/25/21 5:26 AM, Miroslav Benes wrote: > On Thu, 25 Mar 2021, Dong Kai wrote: > >> commit 15b2219facad ("kernel: freezer should treat PF_IO_WORKER like >> PF_KTHREAD for freezing") is to fix the freezeing issue of IO threads >> by making the freezer not send them fake signals. >> >> Here live patching consistency model call klp_send_signals to wake up >> all tasks by send fake signal to all non-kthread which only check the >> PF_KTHREAD flag, so it still send signal to io threads which may lead to >> freezeing issue of io threads. > > I suppose this could happen, but it will also affect the live patching > transition if the io threads do not react to signals. > > Are you able to reproduce it easily? I mean, is there a testcase I could > use to take a closer look? > If repro is only hypothetical at this point, perhaps we can artificially create it in selftests? And useful to verify the future change you mentioned in your other reply? -- Joe ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] livepatch: klp_send_signal should treat PF_IO_WORKER like PF_KTHREAD 2021-03-25 9:26 ` Miroslav Benes 2021-03-25 16:43 ` Joe Lawrence @ 2021-03-26 6:31 ` dongkai (H) 1 sibling, 0 replies; 9+ messages in thread From: dongkai (H) @ 2021-03-26 6:31 UTC (permalink / raw) To: Miroslav Benes Cc: jpoimboe, jikos, pmladek, joe.lawrence, axboe, live-patching, linux-kernel On 2021/3/25 17:26, Miroslav Benes wrote: > On Thu, 25 Mar 2021, Dong Kai wrote: > >> commit 15b2219facad ("kernel: freezer should treat PF_IO_WORKER like >> PF_KTHREAD for freezing") is to fix the freezeing issue of IO threads >> by making the freezer not send them fake signals. >> >> Here live patching consistency model call klp_send_signals to wake up >> all tasks by send fake signal to all non-kthread which only check the >> PF_KTHREAD flag, so it still send signal to io threads which may lead to >> freezeing issue of io threads. > > I suppose this could happen, but it will also affect the live patching > transition if the io threads do not react to signals. > > Are you able to reproduce it easily? I mean, is there a testcase I could > use to take a closer look? > Um... I tried but failed to reproduce this on real environment as i'm not familiar with the io uring usage. So i use a tricky way to verify this possibility by the following patch which create a fake io thread in module and patch the func which is always within thread running stack. Then the stack check will failed when transition and trigger the klp_send_signal flow. This example may not suitable, but you can get my point Kai Note: this patch export some symbols just for test via module because if i create io thread via sysinit, it will receive SIGKILL signal[set by zap_other_threads] when run init process and exit the loop, weird... diff --git a/fs/exec.c b/fs/exec.c index 18594f11c31f..a64af6cac43b 100644 --- a/fs/exec.c +++ b/fs/exec.c @@ -1229,6 +1229,7 @@ void __set_task_comm(struct task_struct *tsk, const char *buf, bool exec) task_unlock(tsk); perf_event_comm(tsk, exec); } +EXPORT_SYMBOL_GPL(__set_task_comm); /* * Calling this is the point of no return. None of the failures will be diff --git a/kernel/fork.c b/kernel/fork.c index 54cc905e5fe0..03064fef7bb1 100644 --- a/kernel/fork.c +++ b/kernel/fork.c @@ -2447,6 +2447,7 @@ struct task_struct *create_io_thread(int (*fn)(void *), void *arg, int node) } return tsk; } +EXPORT_SYMBOL(create_io_thread); /* * Ok, this is the main fork-routine. index 98191218d891..8151d17149a0 100644 --- a/kernel/sched/core.c +++ b/kernel/sched/core.c @@ -3856,6 +3856,7 @@ void wake_up_new_task(struct task_struct *p) #endif task_rq_unlock(rq, p, &rf); } +EXPORT_SYMBOL_GPL(wake_up_new_task); #ifdef CONFIG_PREEMPT_NOTIFIERS diff --git a/samples/test/Makefile b/samples/test/Makefile new file mode 100644 index 000000000000..efbf01c6477e --- /dev/null +++ b/samples/test/Makefile @@ -0,0 +1 @@ +obj-m += io_thread.o livepatch-sample.o diff --git a/samples/test/io_thread.c b/samples/test/io_thread.c new file mode 100644 index 000000000000..e7bdc51a4582 --- /dev/null +++ b/samples/test/io_thread.c @@ -0,0 +1,49 @@ +#include <linux/module.h> +#include <linux/sched.h> +#include <linux/sched/task.h> +#include <linux/sched/signal.h> + +static __used noinline void func(void) +{ + printk("func\n"); + schedule_timeout(HZ * 5); +} + +static int io_worker(void *data) +{ + set_task_comm(current, "io_worker"); + while (1) { + set_current_state(TASK_INTERRUPTIBLE); + func(); + + if (fatal_signal_pending(current)) + break; + } + + return 0; +} + +static int __init io_thread_init(void) +{ + struct task_struct *task = NULL; + + task = create_io_thread(io_worker, NULL, 0); + if (task == NULL) + return -EINVAL; + wake_up_new_task(task); + + /* when insmod exit, io thread got SIGKILL and exit, so... */ + while (1) + schedule_timeout(HZ); + return 0; +} + +static void __exit io_thread_exit(void) +{ + return; +} + +module_init(io_thread_init); +module_exit(io_thread_exit); + +MODULE_LICENSE("GPL"); diff --git a/samples/test/livepatch-sample.c b/samples/test/livepatch-sample.c new file mode 100644 index 000000000000..c35b494f5c5a --- /dev/null +++ b/samples/test/livepatch-sample.c @@ -0,0 +1,43 @@ +#include <linux/module.h> +#include <linux/kernel.h> +#include <linux/livepatch.h> + +static void new_func(void) +{ + schedule_timeout(HZ * 5); + printk("new_func\n"); +} + +static struct klp_func funcs[] = { + { + .old_name = "func", + .new_func = new_func, + }, { } +}; + +static struct klp_object objs[] = { + { + .name = "io_thread", + .funcs = funcs, + }, { } +}; + +static struct klp_patch patch = { + .mod = THIS_MODULE, + .objs = objs, +}; + +static int livepatch_init(void) +{ + return klp_enable_patch(&patch); +} + +static void livepatch_exit(void) +{ +} + +module_init(livepatch_init); +module_exit(livepatch_exit); +MODULE_INFO(livepatch, "Y"); + +MODULE_LICENSE("GPL"); -- >> Here we take the same fix action by treating PF_IO_WORKERS as PF_KTHREAD >> within klp_send_signal function. > > Yes, this sounds reasonable. > > Miroslav > >> Signed-off-by: Dong Kai <dongkai11@huawei.com> >> --- >> note: >> the io threads freeze issue links: >> [1] https://lore.kernel.org/io-uring/YEgnIp43%2F6kFn8GL@kevinlocke.name/ >> [2] https://lore.kernel.org/io-uring/d7350ce7-17dc-75d7-611b-27ebf2cb539b@kernel.dk/ >> >> kernel/livepatch/transition.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/kernel/livepatch/transition.c b/kernel/livepatch/transition.c >> index f6310f848f34..0e1c35c8f4b4 100644 >> --- a/kernel/livepatch/transition.c >> +++ b/kernel/livepatch/transition.c >> @@ -358,7 +358,7 @@ static void klp_send_signals(void) >> * Meanwhile the task could migrate itself and the action >> * would be meaningless. It is not serious though. >> */ >> - if (task->flags & PF_KTHREAD) { >> + if (task->flags & (PF_KTHREAD | PF_IO_WORKER)) { >> /* >> * Wake up a kthread which sleeps interruptedly and >> * still has not been migrated. > ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH] livepatch: klp_send_signal should treat PF_IO_WORKER like PF_KTHREAD 2021-03-25 1:48 [PATCH] livepatch: klp_send_signal should treat PF_IO_WORKER like PF_KTHREAD Dong Kai 2021-03-25 2:51 ` Joe Lawrence 2021-03-25 9:26 ` Miroslav Benes @ 2021-03-25 16:29 ` Jens Axboe 2 siblings, 0 replies; 9+ messages in thread From: Jens Axboe @ 2021-03-25 16:29 UTC (permalink / raw) To: Dong Kai, jpoimboe, jikos, mbenes, pmladek, joe.lawrence Cc: live-patching, linux-kernel On 3/24/21 7:48 PM, Dong Kai wrote: > commit 15b2219facad ("kernel: freezer should treat PF_IO_WORKER like > PF_KTHREAD for freezing") is to fix the freezeing issue of IO threads > by making the freezer not send them fake signals. > > Here live patching consistency model call klp_send_signals to wake up > all tasks by send fake signal to all non-kthread which only check the > PF_KTHREAD flag, so it still send signal to io threads which may lead to > freezeing issue of io threads. > > Here we take the same fix action by treating PF_IO_WORKERS as PF_KTHREAD > within klp_send_signal function. Reviewed-by: Jens Axboe <axboe@kernel.dk> -- Jens Axboe ^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2021-03-26 8:40 UTC | newest] Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-03-25 1:48 [PATCH] livepatch: klp_send_signal should treat PF_IO_WORKER like PF_KTHREAD Dong Kai 2021-03-25 2:51 ` Joe Lawrence 2021-03-25 9:30 ` Miroslav Benes 2021-03-25 16:30 ` Jens Axboe 2021-03-26 8:39 ` Miroslav Benes 2021-03-25 9:26 ` Miroslav Benes 2021-03-25 16:43 ` Joe Lawrence 2021-03-26 6:31 ` dongkai (H) 2021-03-25 16:29 ` Jens Axboe
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).