Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1306599 > unrolled thread

[PATCH] printk: clear console_may_schedule on panic flushing

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [PATCH] printk: clear console_may_schedule on panic flushing Tejun Heo <tj@kernel.org> - 2016-01-11 19:50 +0100
    Re: [PATCH] printk: clear console_may_schedule on panic flushing Andrew Morton <akpm@linux-foundation.org> - 2016-01-11 23:00 +0100
      Re: [PATCH] printk: clear console_may_schedule on panic flushing Tejun Heo <tj@kernel.org> - 2016-01-11 23:10 +0100
        Re: [PATCH] printk: clear console_may_schedule on panic flushing Tejun Heo <tj@kernel.org> - 2016-01-11 23:30 +0100
      Re: [PATCH] printk: clear console_may_schedule on panic flushing Sergey Senozhatsky <sergey.senozhatsky@gmail.com> - 2016-01-12 15:00 +0100

#1306599 — [PATCH] printk: clear console_may_schedule on panic flushing

Commit "printk: do cond_resched() between lines while outputting to
consoles" made console flushing perform cond_resched() after each line
if the context allows as determined by whether the console lock was
acquired with console_lock().  The condition is carried in
console_may_schedule.

During panic, console lock status is ignored and the messages are
forcifully flushed which is implemented by performing
console_trylock(); console_unlock(); sequence ignoring whether trylock
succeeds or fails.  This means that the emergency flushing, after
trylock failure, may enter flushing path with console_may_schedule set
from the actual holder.

As a system may panic from any context, this can lead to
cond_resched() being invoked from a non-sleepable context triggering
an extra warning dump while panicking which is noisy and can be
confusing.  Besides, even when panicking from a sleepable context, we
don't want to be yielding during emergency message dumping.

Currently, the emergency dumping is opencoded in panic().  This patch
replaces the open coded implementation with a new function,
console_flush_on_panic() and makes it explicitly clear
console_may_schedule before starting the emergency flushing.

Signed-off-by: Tejun Heo <tj@kernel.org>
Reported-by: kernel test robot <ying.huang@linux.intel.com>
Link: http://lkml.kernel.org/g/878u3w95ke.fsf@yhuang-dev.intel.com
---
 include/linux/console.h |    1 +
 kernel/panic.c          |    3 +--
 kernel/printk/printk.c  |   19 +++++++++++++++++++
 3 files changed, 21 insertions(+), 2 deletions(-)

diff --git a/include/linux/console.h b/include/linux/console.h
index bd19434..ea731af 100644
--- a/include/linux/console.h
+++ b/include/linux/console.h
@@ -150,6 +150,7 @@ extern int console_trylock(void);
 extern void console_unlock(void);
 extern void console_conditional_schedule(void);
 extern void console_unblank(void);
+extern void console_flush_on_panic(void);
 extern struct tty_driver *console_device(int *);
 extern void console_stop(struct console *);
 extern void console_start(struct console *);
diff --git a/kernel/panic.c b/kernel/panic.c
index b333380..d96469d 100644
--- a/kernel/panic.c
+++ b/kernel/panic.c
@@ -180,8 +180,7 @@ void panic(const char *fmt, ...)
 	 * panic() is not being callled from OOPS.
 	 */
 	debug_locks_off();
-	console_trylock();
-	console_unlock();
+	console_flush_on_panic();
 
 	if (!panic_blink)
 		panic_blink = no_blink;
diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
index d0b8697..7ebcfea 100644
--- a/kernel/printk/printk.c
+++ b/kernel/printk/printk.c
@@ -2386,6 +2386,25 @@ void console_unblank(void)
 	console_unlock();
 }
 
+/**
+ * console_flush_on_panic - flush console content on panic
+ *
+ * Immediately output all pending messages no matter what.
+ */
+void console_flush_on_panic(void)
+{
+	/*
+	 * If someone else is holding the console lock, trylock will fail
+	 * and may_schedule may be set.  Ignore and proceed to unlock so
+	 * that messages are flushed out.  As this can be called from any
+	 * context and we don't want to get preempted while flushing,
+	 * ensure may_schedule is cleared.
+	 */
+	console_trylock();
+	console_may_schedule = 0;
+	console_unlock();
+}
+
 /*
  * Return the console tty driver structure and its associated index
  */

[toc] | [next] | [standalone]

#1306737

On Mon, 11 Jan 2016 13:43:48 -0500 Tejun Heo <tj@kernel.org> wrote:

> Commit "printk: do cond_resched() between lines while outputting to
> consoles" made console flushing perform cond_resched() after each line
> if the context allows as determined by whether the console lock was
> acquired with console_lock().  The condition is carried in
> console_may_schedule.
> 
> During panic, console lock status is ignored and the messages are
> forcifully flushed which is implemented by performing
> console_trylock(); console_unlock(); sequence ignoring whether trylock
> succeeds or fails.  This means that the emergency flushing, after
> trylock failure, may enter flushing path with console_may_schedule set
> from the actual holder.
> 
> As a system may panic from any context, this can lead to
> cond_resched() being invoked from a non-sleepable context triggering
> an extra warning dump while panicking which is noisy and can be
> confusing.  Besides, even when panicking from a sleepable context, we
> don't want to be yielding during emergency message dumping.
> 
> Currently, the emergency dumping is opencoded in panic().  This patch
> replaces the open coded implementation with a new function,
> console_flush_on_panic() and makes it explicitly clear
> console_may_schedule before starting the emergency flushing.
> 

Got that, thanks.  Because the patch has significant information
content I'd normally make it a standalone thing, but as it's destined
for -stable I think I'll scrunch it into
printk-do-cond_resched-between-lines-while-outputting-to-consoles.patch
and merge the changelogs.

You didn't comment on Sergey's observations
(https://lkml.org/lkml/2015/12/2/1192), but I'm believing this is
a separate issue and that this patch is still good.

[toc] | [prev] | [next] | [standalone]

#1306747

Hello,

On Mon, Jan 11, 2016 at 01:59:51PM -0800, Andrew Morton wrote:
> Got that, thanks.  Because the patch has significant information
> content I'd normally make it a standalone thing, but as it's destined
> for -stable I think I'll scrunch it into
> printk-do-cond_resched-between-lines-while-outputting-to-consoles.patch
> and merge the changelogs.

Sure thing.

> You didn't comment on Sergey's observations
> (https://lkml.org/lkml/2015/12/2/1192), but I'm believing this is
> a separate issue and that this patch is still good.

Missed that one somehow.  Will respond.

Thanks.

-- 
tejun

[toc] | [prev] | [next] | [standalone]

#1306773

Hello, again.

On Mon, Jan 11, 2016 at 05:09:27PM -0500, Tejun Heo wrote:
> > You didn't comment on Sergey's observations
> > (https://lkml.org/lkml/2015/12/2/1192), but I'm believing this is
> > a separate issue and that this patch is still good.
> 
> Missed that one somehow.  Will respond.

So, I agree with Jan's response there.  My patch wasn't a full
solution to the whole problem.  It solves a specific case (hang during
registration of a slow console device) and is also needed for future
improvements but we still need Jan's patchset and more to actually get
the problem solved.

Thanks.

-- 
tejun

[toc] | [prev] | [next] | [standalone]

#1307424

On (01/11/16 13:59), Andrew Morton wrote:
> > Commit "printk: do cond_resched() between lines while outputting to
> > consoles" made console flushing perform cond_resched() after each line
> > if the context allows as determined by whether the console lock was
> > acquired with console_lock().  The condition is carried in
> > console_may_schedule.
> > 
> > During panic, console lock status is ignored and the messages are
> > forcifully flushed which is implemented by performing
> > console_trylock(); console_unlock(); sequence ignoring whether trylock
> > succeeds or fails.  This means that the emergency flushing, after
> > trylock failure, may enter flushing path with console_may_schedule set
> > from the actual holder.
> > 
> > As a system may panic from any context, this can lead to
> > cond_resched() being invoked from a non-sleepable context triggering
> > an extra warning dump while panicking which is noisy and can be
> > confusing.  Besides, even when panicking from a sleepable context, we
> > don't want to be yielding during emergency message dumping.
> > 
> > Currently, the emergency dumping is opencoded in panic().  This patch
> > replaces the open coded implementation with a new function,
> > console_flush_on_panic() and makes it explicitly clear
> > console_may_schedule before starting the emergency flushing.
> > 
> 
> Got that, thanks.  Because the patch has significant information
> content I'd normally make it a standalone thing, but as it's destined
> for -stable I think I'll scrunch it into
> printk-do-cond_resched-between-lines-while-outputting-to-consoles.patch
> and merge the changelogs.
> 
> You didn't comment on Sergey's observations
> (https://lkml.org/lkml/2015/12/2/1192), but I'm believing this is
> a separate issue and that this patch is still good.

Thanks for Cc-ing.

Yes, this problem is different.


FWIW,
what I ended up implementing in my private builds is a bit different
thing -- I added a new console_panic_mode() function which basically
calls zap_locks(). The reason I did it this way was that console_unlock()
can potentially have other locks in it some day, not just `console_sem'.
For example, like in one of Jan's deferred printk implementations. Sort
of a defensive move to avoid possible problems.

	-ss

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web