* [PATCH 0/3] livepatch: Clean up of reliable stacktrace warnings
@ 2019-05-31 7:41 Petr Mladek
2019-05-31 7:41 ` [PATCH 1/3] stacktrace: Remove superfluous WARN_ONCE() from save_stack_trace_tsk_reliable() Petr Mladek
` (2 more replies)
0 siblings, 3 replies; 14+ messages in thread
From: Petr Mladek @ 2019-05-31 7:41 UTC (permalink / raw)
To: Jiri Kosina, Josh Poimboeuf, Miroslav Benes
Cc: Joe Lawrence, Kamalesh Babulal, live-patching, linux-kernel, Petr Mladek
I believe that all the changes make sense. I would like to get them
merged if nobody see a fundamental problem.
Changes against v2:
+ Put back the patch removing WARN_ONCE in the weak
save_stack_trace_tsk_reliable(). It is related.
+ Simplified patch removing the duplicate warning from klp_check_stack()
+ Update commit message for 3rd patch [Josh]
Petr Mladek (3):
stacktrace: Remove superfluous WARN_ONCE() from
save_stack_trace_tsk_reliable()
livepatch: Remove duplicate warning about missing reliable stacktrace
support
livepatch: Use static buffer for debugging messages under rq lock
kernel/livepatch/transition.c | 4 +---
kernel/stacktrace.c | 1 -
2 files changed, 1 insertion(+), 4 deletions(-)
--
2.16.4
^ permalink raw reply [flat|nested] 14+ messages in thread* [PATCH 1/3] stacktrace: Remove superfluous WARN_ONCE() from save_stack_trace_tsk_reliable() 2019-05-31 7:41 [PATCH 0/3] livepatch: Clean up of reliable stacktrace warnings Petr Mladek @ 2019-05-31 7:41 ` Petr Mladek 2019-05-31 12:25 ` Miroslav Benes 2019-05-31 7:41 ` [PATCH 2/3] livepatch: Remove duplicate warning about missing reliable stacktrace support Petr Mladek 2019-05-31 7:41 ` [PATCH 3/3] livepatch: Use static buffer for debugging messages under rq lock Petr Mladek 2 siblings, 1 reply; 14+ messages in thread From: Petr Mladek @ 2019-05-31 7:41 UTC (permalink / raw) To: Jiri Kosina, Josh Poimboeuf, Miroslav Benes Cc: Joe Lawrence, Kamalesh Babulal, live-patching, linux-kernel, Petr Mladek WARN_ONCE() in the generic save_stack_trace_tsk_reliable() is superfluous. The information is passed also via the return value. The only current user klp_check_stack() writes its own warning when the reliable stack traces are not supported. Other eventual users might want its own error handling as well. Signed-off-by: Petr Mladek <pmladek@suse.com> Acked-by: Miroslav Benes <mbenes@suse.cz> Reviewed-by: Kamalesh Babulal <kamalesh@linux.vnet.ibm.com> --- kernel/stacktrace.c | 1 - 1 file changed, 1 deletion(-) diff --git a/kernel/stacktrace.c b/kernel/stacktrace.c index 5667f1da3ede..8d088408928d 100644 --- a/kernel/stacktrace.c +++ b/kernel/stacktrace.c @@ -259,7 +259,6 @@ __weak int save_stack_trace_tsk_reliable(struct task_struct *tsk, struct stack_trace *trace) { - WARN_ONCE(1, KERN_INFO "save_stack_tsk_reliable() not implemented yet.\n"); return -ENOSYS; } -- 2.16.4 ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 1/3] stacktrace: Remove superfluous WARN_ONCE() from save_stack_trace_tsk_reliable() 2019-05-31 7:41 ` [PATCH 1/3] stacktrace: Remove superfluous WARN_ONCE() from save_stack_trace_tsk_reliable() Petr Mladek @ 2019-05-31 12:25 ` Miroslav Benes 2019-05-31 19:27 ` Josh Poimboeuf 0 siblings, 1 reply; 14+ messages in thread From: Miroslav Benes @ 2019-05-31 12:25 UTC (permalink / raw) To: Petr Mladek Cc: Jiri Kosina, Josh Poimboeuf, Joe Lawrence, Kamalesh Babulal, live-patching, linux-kernel On Fri, 31 May 2019, Petr Mladek wrote: > WARN_ONCE() in the generic save_stack_trace_tsk_reliable() is superfluous. > > The information is passed also via the return value. The only current > user klp_check_stack() writes its own warning when the reliable stack > traces are not supported. Other eventual users might want its own error > handling as well. > > Signed-off-by: Petr Mladek <pmladek@suse.com> > Acked-by: Miroslav Benes <mbenes@suse.cz> > Reviewed-by: Kamalesh Babulal <kamalesh@linux.vnet.ibm.com> > --- > kernel/stacktrace.c | 1 - > 1 file changed, 1 deletion(-) > > diff --git a/kernel/stacktrace.c b/kernel/stacktrace.c > index 5667f1da3ede..8d088408928d 100644 > --- a/kernel/stacktrace.c > +++ b/kernel/stacktrace.c > @@ -259,7 +259,6 @@ __weak int > save_stack_trace_tsk_reliable(struct task_struct *tsk, > struct stack_trace *trace) > { > - WARN_ONCE(1, KERN_INFO "save_stack_tsk_reliable() not implemented yet.\n"); > return -ENOSYS; > } Do we even need the weak function now after Thomas' changes to kernel/stacktrace.c? - livepatch is the only user and it calls stack_trace_save_tsk_reliable() - x86 defines CONFIG_ARCH_STACKWALK and CONFIG_HAVE_RELIABLE_STACKTRACE, so it has stack_trace_save_tsk_reliable() implemented and it calls arch_stack_walk_reliable() - powerpc defines CONFIG_HAVE_RELIABLE_STACKTRACE and does not have CONFIG_ARCH_STACKWALK. It also has stack_trace_save_tsk_reliable() implemented and it calls save_stack_trace_tsk_reliable(), which is implemented in arch/powerpc/ - all other archs do not have CONFIG_HAVE_RELIABLE_STACKTRACE and there is stack_trace_save_tsk_reliable() returning ENOSYS for these cases in include/linux/stacktrace.c Miroslav ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 1/3] stacktrace: Remove superfluous WARN_ONCE() from save_stack_trace_tsk_reliable() 2019-05-31 12:25 ` Miroslav Benes @ 2019-05-31 19:27 ` Josh Poimboeuf 0 siblings, 0 replies; 14+ messages in thread From: Josh Poimboeuf @ 2019-05-31 19:27 UTC (permalink / raw) To: Miroslav Benes Cc: Petr Mladek, Jiri Kosina, Joe Lawrence, Kamalesh Babulal, live-patching, linux-kernel On Fri, May 31, 2019 at 02:25:15PM +0200, Miroslav Benes wrote: > On Fri, 31 May 2019, Petr Mladek wrote: > > > WARN_ONCE() in the generic save_stack_trace_tsk_reliable() is superfluous. > > > > The information is passed also via the return value. The only current > > user klp_check_stack() writes its own warning when the reliable stack > > traces are not supported. Other eventual users might want its own error > > handling as well. > > > > Signed-off-by: Petr Mladek <pmladek@suse.com> > > Acked-by: Miroslav Benes <mbenes@suse.cz> > > Reviewed-by: Kamalesh Babulal <kamalesh@linux.vnet.ibm.com> > > --- > > kernel/stacktrace.c | 1 - > > 1 file changed, 1 deletion(-) > > > > diff --git a/kernel/stacktrace.c b/kernel/stacktrace.c > > index 5667f1da3ede..8d088408928d 100644 > > --- a/kernel/stacktrace.c > > +++ b/kernel/stacktrace.c > > @@ -259,7 +259,6 @@ __weak int > > save_stack_trace_tsk_reliable(struct task_struct *tsk, > > struct stack_trace *trace) > > { > > - WARN_ONCE(1, KERN_INFO "save_stack_tsk_reliable() not implemented yet.\n"); > > return -ENOSYS; > > } > > Do we even need the weak function now after Thomas' changes to > kernel/stacktrace.c? > > - livepatch is the only user and it calls stack_trace_save_tsk_reliable() > - x86 defines CONFIG_ARCH_STACKWALK and CONFIG_HAVE_RELIABLE_STACKTRACE, > so it has stack_trace_save_tsk_reliable() implemented and it calls > arch_stack_walk_reliable() > - powerpc defines CONFIG_HAVE_RELIABLE_STACKTRACE and does not have > CONFIG_ARCH_STACKWALK. It also has stack_trace_save_tsk_reliable() > implemented and it calls save_stack_trace_tsk_reliable(), which is > implemented in arch/powerpc/ > - all other archs do not have CONFIG_HAVE_RELIABLE_STACKTRACE and there is > stack_trace_save_tsk_reliable() returning ENOSYS for these cases in > include/linux/stacktrace.c I think you're right. stack_trace_save_tsk_reliable() in stacktrace.h returning -ENOSYS serves the same purpose as the old weak version of save_stack_trace_tsk_reliable() which is no longer called directly. -- Josh ^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH 2/3] livepatch: Remove duplicate warning about missing reliable stacktrace support 2019-05-31 7:41 [PATCH 0/3] livepatch: Clean up of reliable stacktrace warnings Petr Mladek 2019-05-31 7:41 ` [PATCH 1/3] stacktrace: Remove superfluous WARN_ONCE() from save_stack_trace_tsk_reliable() Petr Mladek @ 2019-05-31 7:41 ` Petr Mladek 2019-05-31 12:32 ` Miroslav Benes 2019-05-31 7:41 ` [PATCH 3/3] livepatch: Use static buffer for debugging messages under rq lock Petr Mladek 2 siblings, 1 reply; 14+ messages in thread From: Petr Mladek @ 2019-05-31 7:41 UTC (permalink / raw) To: Jiri Kosina, Josh Poimboeuf, Miroslav Benes Cc: Joe Lawrence, Kamalesh Babulal, live-patching, linux-kernel, Petr Mladek WARN_ON_ONCE() could not be called safely under rq lock because of console deadlock issues. It can be simply removed. A better descriptive message is written in klp_enable_patch() when klp_have_reliable_stack() fails. The remaining debug message is good enough. Signed-off-by: Petr Mladek <pmladek@suse.com> --- kernel/livepatch/transition.c | 1 - 1 file changed, 1 deletion(-) diff --git a/kernel/livepatch/transition.c b/kernel/livepatch/transition.c index abb2a4a2cbb2..1bf362df76e1 100644 --- a/kernel/livepatch/transition.c +++ b/kernel/livepatch/transition.c @@ -247,7 +247,6 @@ static int klp_check_stack(struct task_struct *task, char *err_buf) int ret, nr_entries; ret = stack_trace_save_tsk_reliable(task, entries, ARRAY_SIZE(entries)); - WARN_ON_ONCE(ret == -ENOSYS); if (ret < 0) { snprintf(err_buf, STACK_ERR_BUF_SIZE, "%s: %s:%d has an unreliable stack\n", -- 2.16.4 ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 2/3] livepatch: Remove duplicate warning about missing reliable stacktrace support 2019-05-31 7:41 ` [PATCH 2/3] livepatch: Remove duplicate warning about missing reliable stacktrace support Petr Mladek @ 2019-05-31 12:32 ` Miroslav Benes 2019-05-31 13:19 ` Petr Mladek 2019-05-31 19:37 ` Josh Poimboeuf 0 siblings, 2 replies; 14+ messages in thread From: Miroslav Benes @ 2019-05-31 12:32 UTC (permalink / raw) To: Petr Mladek Cc: Jiri Kosina, Josh Poimboeuf, Joe Lawrence, Kamalesh Babulal, live-patching, linux-kernel On Fri, 31 May 2019, Petr Mladek wrote: > WARN_ON_ONCE() could not be called safely under rq lock because > of console deadlock issues. > > It can be simply removed. A better descriptive message is written > in klp_enable_patch() when klp_have_reliable_stack() fails. > The remaining debug message is good enough. > > Signed-off-by: Petr Mladek <pmladek@suse.com> > --- > kernel/livepatch/transition.c | 1 - > 1 file changed, 1 deletion(-) > > diff --git a/kernel/livepatch/transition.c b/kernel/livepatch/transition.c > index abb2a4a2cbb2..1bf362df76e1 100644 > --- a/kernel/livepatch/transition.c > +++ b/kernel/livepatch/transition.c > @@ -247,7 +247,6 @@ static int klp_check_stack(struct task_struct *task, char *err_buf) > int ret, nr_entries; > > ret = stack_trace_save_tsk_reliable(task, entries, ARRAY_SIZE(entries)); > - WARN_ON_ONCE(ret == -ENOSYS); > if (ret < 0) { > snprintf(err_buf, STACK_ERR_BUF_SIZE, > "%s: %s:%d has an unreliable stack\n", The current situation is not the best, but I think the patch improves it only slightly. I see two possible solutions. 1. we either revert commit 1d98a69e5cef ("livepatch: Remove reliable stacktrace check in klp_try_switch_task()"), so that klp_check_stack() returns right away. 2. or we test ret from stack_trace_save_tsk_reliable() for ENOSYS and return. In my opinion either of them is better than what we have now (and what we would have with the patch), because klp_check_stack() returns, but it prints out that a task has an unreliable stack. Yes, it is pr_debug() only in the end, but still. I don't have a preference and my understanding is that Petr does not want to do v4. I can prepare a patch, but it would be nice to choose now. Josh? Anyone else? Miroslav ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 2/3] livepatch: Remove duplicate warning about missing reliable stacktrace support 2019-05-31 12:32 ` Miroslav Benes @ 2019-05-31 13:19 ` Petr Mladek 2019-05-31 13:29 ` Miroslav Benes 2019-05-31 19:37 ` Josh Poimboeuf 1 sibling, 1 reply; 14+ messages in thread From: Petr Mladek @ 2019-05-31 13:19 UTC (permalink / raw) To: Miroslav Benes Cc: Jiri Kosina, Josh Poimboeuf, Joe Lawrence, Kamalesh Babulal, live-patching, linux-kernel On Fri 2019-05-31 14:32:34, Miroslav Benes wrote: > On Fri, 31 May 2019, Petr Mladek wrote: > > > WARN_ON_ONCE() could not be called safely under rq lock because > > of console deadlock issues. > > > > It can be simply removed. A better descriptive message is written > > in klp_enable_patch() when klp_have_reliable_stack() fails. > > The remaining debug message is good enough. > > > > Signed-off-by: Petr Mladek <pmladek@suse.com> > > --- > > kernel/livepatch/transition.c | 1 - > > 1 file changed, 1 deletion(-) > > > > diff --git a/kernel/livepatch/transition.c b/kernel/livepatch/transition.c > > index abb2a4a2cbb2..1bf362df76e1 100644 > > --- a/kernel/livepatch/transition.c > > +++ b/kernel/livepatch/transition.c > > @@ -247,7 +247,6 @@ static int klp_check_stack(struct task_struct *task, char *err_buf) > > int ret, nr_entries; > > > > ret = stack_trace_save_tsk_reliable(task, entries, ARRAY_SIZE(entries)); > > - WARN_ON_ONCE(ret == -ENOSYS); > > if (ret < 0) { > > snprintf(err_buf, STACK_ERR_BUF_SIZE, > > "%s: %s:%d has an unreliable stack\n", > > The current situation is not the best, but I think the patch improves it > only slightly. I see two possible solutions. > > 1. we either revert commit 1d98a69e5cef ("livepatch: Remove reliable > stacktrace check in klp_try_switch_task()"), so that klp_check_stack() > returns right away. > > 2. or we test ret from stack_trace_save_tsk_reliable() for ENOSYS and > return. > > In my opinion either of them is better than what we have now (and what we > would have with the patch), because klp_check_stack() returns, but it > prints out that a task has an unreliable stack. Yes, it is pr_debug() only > in the end, but still. IMHO, any extra check will not improve the situation much. Quiet return is as useless as the misleading pr_debug() that will not normally get printed anyway. Adding a warning is complicated because we would need to pass it via the temporary buffer. It is not worth the complexity. IMHO, it does not make sense to complicate the code because of a corner case situation that would hit only people adding livepatch support for new architecture. > I don't have a preference and my understanding is that Petr does not want > to do v4. I can prepare a patch, but it would be nice to choose now. Josh? > Anyone else? Exactly. Fell free to take over the patches. I do not want to spend more time on endless discussions about this corner case. I made the 1st and 2nd patch only because of ideas mentioned when switching klp_have_reliable_stack() from error to warning, namely: https://lkml.kernel.org/r/20190418135858.n3lzq2oxkj52m6bi@treble https://lkml.kernel.org/r/alpine.LSU.2.21.1904231041500.19172@pobox.suse.cz I have never expected that it would need several more respins. Note that there is no good solution. We would need to choose between a complicated code or an imperfect error handling. Anything in between is just a compromise. Choosing between all the variants looks like a bikeshedding to me. I sent what I preferred and what looked acceptable. I am not going to block other approaches unless they make the code too complicated. Best Regards, Petr ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 2/3] livepatch: Remove duplicate warning about missing reliable stacktrace support 2019-05-31 13:19 ` Petr Mladek @ 2019-05-31 13:29 ` Miroslav Benes 0 siblings, 0 replies; 14+ messages in thread From: Miroslav Benes @ 2019-05-31 13:29 UTC (permalink / raw) To: Petr Mladek Cc: Jiri Kosina, Josh Poimboeuf, Joe Lawrence, Kamalesh Babulal, live-patching, linux-kernel On Fri, 31 May 2019, Petr Mladek wrote: > On Fri 2019-05-31 14:32:34, Miroslav Benes wrote: > > On Fri, 31 May 2019, Petr Mladek wrote: > > > > > WARN_ON_ONCE() could not be called safely under rq lock because > > > of console deadlock issues. > > > > > > It can be simply removed. A better descriptive message is written > > > in klp_enable_patch() when klp_have_reliable_stack() fails. > > > The remaining debug message is good enough. > > > > > > Signed-off-by: Petr Mladek <pmladek@suse.com> > > > --- > > > kernel/livepatch/transition.c | 1 - > > > 1 file changed, 1 deletion(-) > > > > > > diff --git a/kernel/livepatch/transition.c b/kernel/livepatch/transition.c > > > index abb2a4a2cbb2..1bf362df76e1 100644 > > > --- a/kernel/livepatch/transition.c > > > +++ b/kernel/livepatch/transition.c > > > @@ -247,7 +247,6 @@ static int klp_check_stack(struct task_struct *task, char *err_buf) > > > int ret, nr_entries; > > > > > > ret = stack_trace_save_tsk_reliable(task, entries, ARRAY_SIZE(entries)); > > > - WARN_ON_ONCE(ret == -ENOSYS); > > > if (ret < 0) { > > > snprintf(err_buf, STACK_ERR_BUF_SIZE, > > > "%s: %s:%d has an unreliable stack\n", > > > > The current situation is not the best, but I think the patch improves it > > only slightly. I see two possible solutions. > > > > 1. we either revert commit 1d98a69e5cef ("livepatch: Remove reliable > > stacktrace check in klp_try_switch_task()"), so that klp_check_stack() > > returns right away. > > > > 2. or we test ret from stack_trace_save_tsk_reliable() for ENOSYS and > > return. > > > > In my opinion either of them is better than what we have now (and what we > > would have with the patch), because klp_check_stack() returns, but it > > prints out that a task has an unreliable stack. Yes, it is pr_debug() only > > in the end, but still. > > IMHO, any extra check will not improve the situation much. Quiet > return is as useless as the misleading pr_debug() that will > not normally get printed anyway. I disagree here. I think the silent return would be perfectly fine. The user was warned in klp_enable_patch() already. Miroslav ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 2/3] livepatch: Remove duplicate warning about missing reliable stacktrace support 2019-05-31 12:32 ` Miroslav Benes 2019-05-31 13:19 ` Petr Mladek @ 2019-05-31 19:37 ` Josh Poimboeuf 1 sibling, 0 replies; 14+ messages in thread From: Josh Poimboeuf @ 2019-05-31 19:37 UTC (permalink / raw) To: Miroslav Benes Cc: Petr Mladek, Jiri Kosina, Joe Lawrence, Kamalesh Babulal, live-patching, linux-kernel On Fri, May 31, 2019 at 02:32:34PM +0200, Miroslav Benes wrote: > On Fri, 31 May 2019, Petr Mladek wrote: > > > WARN_ON_ONCE() could not be called safely under rq lock because > > of console deadlock issues. > > > > It can be simply removed. A better descriptive message is written > > in klp_enable_patch() when klp_have_reliable_stack() fails. > > The remaining debug message is good enough. > > > > Signed-off-by: Petr Mladek <pmladek@suse.com> > > --- > > kernel/livepatch/transition.c | 1 - > > 1 file changed, 1 deletion(-) > > > > diff --git a/kernel/livepatch/transition.c b/kernel/livepatch/transition.c > > index abb2a4a2cbb2..1bf362df76e1 100644 > > --- a/kernel/livepatch/transition.c > > +++ b/kernel/livepatch/transition.c > > @@ -247,7 +247,6 @@ static int klp_check_stack(struct task_struct *task, char *err_buf) > > int ret, nr_entries; > > > > ret = stack_trace_save_tsk_reliable(task, entries, ARRAY_SIZE(entries)); > > - WARN_ON_ONCE(ret == -ENOSYS); > > if (ret < 0) { > > snprintf(err_buf, STACK_ERR_BUF_SIZE, > > "%s: %s:%d has an unreliable stack\n", > > The current situation is not the best, but I think the patch improves it > only slightly. I see two possible solutions. > > 1. we either revert commit 1d98a69e5cef ("livepatch: Remove reliable > stacktrace check in klp_try_switch_task()"), so that klp_check_stack() > returns right away. > > 2. or we test ret from stack_trace_save_tsk_reliable() for ENOSYS and > return. > > In my opinion either of them is better than what we have now (and what we > would have with the patch), because klp_check_stack() returns, but it > prints out that a task has an unreliable stack. Yes, it is pr_debug() only > in the end, but still. > > I don't have a preference and my understanding is that Petr does not want > to do v4. I can prepare a patch, but it would be nice to choose now. Josh? > Anyone else? My vote would be #1 -- just revert 1d98a69e5cef. -- Josh ^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH 3/3] livepatch: Use static buffer for debugging messages under rq lock 2019-05-31 7:41 [PATCH 0/3] livepatch: Clean up of reliable stacktrace warnings Petr Mladek 2019-05-31 7:41 ` [PATCH 1/3] stacktrace: Remove superfluous WARN_ONCE() from save_stack_trace_tsk_reliable() Petr Mladek 2019-05-31 7:41 ` [PATCH 2/3] livepatch: Remove duplicate warning about missing reliable stacktrace support Petr Mladek @ 2019-05-31 7:41 ` Petr Mladek 2019-05-31 12:37 ` Miroslav Benes 2019-05-31 19:39 ` Josh Poimboeuf 2 siblings, 2 replies; 14+ messages in thread From: Petr Mladek @ 2019-05-31 7:41 UTC (permalink / raw) To: Jiri Kosina, Josh Poimboeuf, Miroslav Benes Cc: Joe Lawrence, Kamalesh Babulal, live-patching, linux-kernel, Petr Mladek The err_buf array uses 128 bytes of stack space. Move it off the stack by making it static. It's safe to use a shared buffer because klp_try_switch_task() is called under klp_mutex. Signed-off-by: Petr Mladek <pmladek@suse.com> Acked-by: Miroslav Benes <mbenes@suse.cz> Reviewed-by: Kamalesh Babulal <kamalesh@linux.vnet.ibm.com> --- kernel/livepatch/transition.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/kernel/livepatch/transition.c b/kernel/livepatch/transition.c index 1bf362df76e1..5c4f0c1f826e 100644 --- a/kernel/livepatch/transition.c +++ b/kernel/livepatch/transition.c @@ -280,11 +280,11 @@ static int klp_check_stack(struct task_struct *task, char *err_buf) */ static bool klp_try_switch_task(struct task_struct *task) { + static char err_buf[STACK_ERR_BUF_SIZE]; struct rq *rq; struct rq_flags flags; int ret; bool success = false; - char err_buf[STACK_ERR_BUF_SIZE]; err_buf[0] = '\0'; @@ -327,7 +327,6 @@ static bool klp_try_switch_task(struct task_struct *task) pr_debug("%s", err_buf); return success; - } /* -- 2.16.4 ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 3/3] livepatch: Use static buffer for debugging messages under rq lock 2019-05-31 7:41 ` [PATCH 3/3] livepatch: Use static buffer for debugging messages under rq lock Petr Mladek @ 2019-05-31 12:37 ` Miroslav Benes 2019-06-03 8:07 ` Petr Mladek 2019-05-31 19:39 ` Josh Poimboeuf 1 sibling, 1 reply; 14+ messages in thread From: Miroslav Benes @ 2019-05-31 12:37 UTC (permalink / raw) To: Petr Mladek Cc: Jiri Kosina, Josh Poimboeuf, Joe Lawrence, Kamalesh Babulal, live-patching, linux-kernel On Fri, 31 May 2019, Petr Mladek wrote: > The err_buf array uses 128 bytes of stack space. Move it off the stack > by making it static. It's safe to use a shared buffer because > klp_try_switch_task() is called under klp_mutex. > > Signed-off-by: Petr Mladek <pmladek@suse.com> > Acked-by: Miroslav Benes <mbenes@suse.cz> > Reviewed-by: Kamalesh Babulal <kamalesh@linux.vnet.ibm.com> > --- > kernel/livepatch/transition.c | 3 +-- > 1 file changed, 1 insertion(+), 2 deletions(-) > > diff --git a/kernel/livepatch/transition.c b/kernel/livepatch/transition.c > index 1bf362df76e1..5c4f0c1f826e 100644 > --- a/kernel/livepatch/transition.c > +++ b/kernel/livepatch/transition.c > @@ -280,11 +280,11 @@ static int klp_check_stack(struct task_struct *task, char *err_buf) > */ > static bool klp_try_switch_task(struct task_struct *task) > { > + static char err_buf[STACK_ERR_BUF_SIZE]; > struct rq *rq; > struct rq_flags flags; > int ret; > bool success = false; > - char err_buf[STACK_ERR_BUF_SIZE]; > > err_buf[0] = '\0'; > > @@ -327,7 +327,6 @@ static bool klp_try_switch_task(struct task_struct *task) > pr_debug("%s", err_buf); > > return success; > - > } This could go in separately as it is not connected to the rest of the series. Miroslav ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 3/3] livepatch: Use static buffer for debugging messages under rq lock 2019-05-31 12:37 ` Miroslav Benes @ 2019-06-03 8:07 ` Petr Mladek 0 siblings, 0 replies; 14+ messages in thread From: Petr Mladek @ 2019-06-03 8:07 UTC (permalink / raw) To: Miroslav Benes Cc: Jiri Kosina, Josh Poimboeuf, Joe Lawrence, Kamalesh Babulal, live-patching, linux-kernel On Fri 2019-05-31 14:37:52, Miroslav Benes wrote: > On Fri, 31 May 2019, Petr Mladek wrote: > > > The err_buf array uses 128 bytes of stack space. Move it off the stack > > by making it static. It's safe to use a shared buffer because > > klp_try_switch_task() is called under klp_mutex. > > > > diff --git a/kernel/livepatch/transition.c b/kernel/livepatch/transition.c > > index 1bf362df76e1..5c4f0c1f826e 100644 > > --- a/kernel/livepatch/transition.c > > +++ b/kernel/livepatch/transition.c > > @@ -327,7 +327,6 @@ static bool klp_try_switch_task(struct task_struct *task) > > pr_debug("%s", err_buf); > > > > return success; > > - > > } > > This could go in separately as it is not connected to the rest of the > series. I have never seen a standalone commit just removing an empty line. It is usually done when one touches the code around. If you resist, we could omit this hunk from the patch and leave the code as is. Best Regards, Petr ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 3/3] livepatch: Use static buffer for debugging messages under rq lock 2019-05-31 7:41 ` [PATCH 3/3] livepatch: Use static buffer for debugging messages under rq lock Petr Mladek 2019-05-31 12:37 ` Miroslav Benes @ 2019-05-31 19:39 ` Josh Poimboeuf 2019-06-06 13:27 ` Petr Mladek 1 sibling, 1 reply; 14+ messages in thread From: Josh Poimboeuf @ 2019-05-31 19:39 UTC (permalink / raw) To: Petr Mladek Cc: Jiri Kosina, Miroslav Benes, Joe Lawrence, Kamalesh Babulal, live-patching, linux-kernel On Fri, May 31, 2019 at 09:41:47AM +0200, Petr Mladek wrote: > The err_buf array uses 128 bytes of stack space. Move it off the stack > by making it static. It's safe to use a shared buffer because > klp_try_switch_task() is called under klp_mutex. > > Signed-off-by: Petr Mladek <pmladek@suse.com> > Acked-by: Miroslav Benes <mbenes@suse.cz> > Reviewed-by: Kamalesh Babulal <kamalesh@linux.vnet.ibm.com> Acked-by: Josh Poimboeuf <jpoimboe@redhat.com> -- Josh ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 3/3] livepatch: Use static buffer for debugging messages under rq lock 2019-05-31 19:39 ` Josh Poimboeuf @ 2019-06-06 13:27 ` Petr Mladek 0 siblings, 0 replies; 14+ messages in thread From: Petr Mladek @ 2019-06-06 13:27 UTC (permalink / raw) To: Josh Poimboeuf Cc: Jiri Kosina, Miroslav Benes, Joe Lawrence, Kamalesh Babulal, live-patching, linux-kernel On Fri 2019-05-31 14:39:08, Josh Poimboeuf wrote: > On Fri, May 31, 2019 at 09:41:47AM +0200, Petr Mladek wrote: > > The err_buf array uses 128 bytes of stack space. Move it off the stack > > by making it static. It's safe to use a shared buffer because > > klp_try_switch_task() is called under klp_mutex. > > > > Signed-off-by: Petr Mladek <pmladek@suse.com> > > Acked-by: Miroslav Benes <mbenes@suse.cz> > > Reviewed-by: Kamalesh Babulal <kamalesh@linux.vnet.ibm.com> > > Acked-by: Josh Poimboeuf <jpoimboe@redhat.com> The patch is committed into for-5.3/core branch. Note that the branch is based on the last merge from livepatch.git. As a result, the sefttest fails because of the regression in the reliable stacktrace code. You might want to base your development on the for-next branch. Or you chould cherry pick the commit 7eaf51a2e094229b75cc0c31 ("[PATCH] stacktrace: Unbreak stack_trace_save_tsk_reliable()"). Best Regards, Petr PS: I am leaving the fate of the other two patches into Miroslav's hands ;-) ^ permalink raw reply [flat|nested] 14+ messages in thread
end of thread, other threads:[~2019-06-06 13:27 UTC | newest] Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-05-31 7:41 [PATCH 0/3] livepatch: Clean up of reliable stacktrace warnings Petr Mladek 2019-05-31 7:41 ` [PATCH 1/3] stacktrace: Remove superfluous WARN_ONCE() from save_stack_trace_tsk_reliable() Petr Mladek 2019-05-31 12:25 ` Miroslav Benes 2019-05-31 19:27 ` Josh Poimboeuf 2019-05-31 7:41 ` [PATCH 2/3] livepatch: Remove duplicate warning about missing reliable stacktrace support Petr Mladek 2019-05-31 12:32 ` Miroslav Benes 2019-05-31 13:19 ` Petr Mladek 2019-05-31 13:29 ` Miroslav Benes 2019-05-31 19:37 ` Josh Poimboeuf 2019-05-31 7:41 ` [PATCH 3/3] livepatch: Use static buffer for debugging messages under rq lock Petr Mladek 2019-05-31 12:37 ` Miroslav Benes 2019-06-03 8:07 ` Petr Mladek 2019-05-31 19:39 ` Josh Poimboeuf 2019-06-06 13:27 ` Petr Mladek
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox
all inboxes | Powered by JetHome®