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

Groups > linux.kernel > #1315539 > unrolled thread

[RFC][PATCH v3 0/4] let printk()/console_trylock() callers to cond_resched()

Back to article view | Back to linux.kernel

Contents

  [RFC][PATCH v3 0/4] let printk()/console_trylock() callers to cond_resched() Sergey Senozhatsky <sergey.senozhatsky@gmail.com> - 2016-01-23 09:20 +0100
    [RFC][PATCH v3 4/4] printk: set may_schedule for some of console_trylock callers Sergey Senozhatsky <sergey.senozhatsky@gmail.com> - 2016-01-23 09:20 +0100
    [RFC][PATCH v3 1/4] printk: move can_use_console out of console_trylock_for_printk Sergey Senozhatsky <sergey.senozhatsky@gmail.com> - 2016-01-23 09:20 +0100

#1315539 — [RFC][PATCH v3 0/4] let printk()/console_trylock() callers to cond_resched()

[was: cond_resched() some of console_trylock callers]

Hello,

console_unlock() allows to cond_resched() if its caller has
set `console_may_schedule' to 1 (this functionality present
since commit 'printk: do cond_resched() between lines while
outputting to consoles').

The rules are:
-- console_lock() always sets `console_may_schedule' to 1
-- console_trylock() always sets `console_may_schedule' to 0

printk() calls console_unlock() with preemption desabled, which
basically can lead to RCU stalls, watchdog soft lockups, etc. if
something is simultaneously calling printk() frequent enough (IOW,
console_sem owner always has new data to send to console divers
and can't leave console_unlock() for a long time).

printk()->console_trylock() callers do not necessarily execute in
atomic contexts, and some of them can cond_resched() in
console_unlock(). console_trylock() can set `console_may_schedule'
to 1 (allow cond_resched() later in consoe_unlock()) when it's safe.

0001 addresses a theoretical loss of printk messages.
0002 is an optional optimization patch
0003 drops console_trylock_for_printk
0004 lets some of printk()/console_trylock() callers to cond_resched()
     in console_unlock()

v2-v3 (thanks to Petr Mladek for reviews):
-- do not call can_use_console() on every iteration in console_unlock() (Petr Mladek)
-- move "This stops the holder of console_sem.." comment (noted by Petr Mladek)
-- take extra care of !PREEMPT_COUNT kernels (Petr Mladek)
-- call_console_drivers() still must check cpu_online && CON_ANYTIME (Petr Mladek)
-- removed console_trylock_for_printk() (noted by Petr Mladek)

v1-v2:
-- make have_callable_console() available for !PRINTK configs (lkp@intel.com)
-- take care of RCU preempt kernels in console_trylock()


Sergey Senozhatsky (4):
  printk: move can_use_console out of console_trylock_for_printk
  printk: do not console_cont_flush() on every jump to again
  printk: remove console_trylock_for_printk
  printk: set may_schedule for some of console_trylock callers

 kernel/printk/printk.c | 121 ++++++++++++++++++++++---------------------------
 1 file changed, 55 insertions(+), 66 deletions(-)

-- 
2.7.0

[toc] | [next] | [standalone]

#1315541 — [RFC][PATCH v3 4/4] printk: set may_schedule for some of console_trylock callers

console_unlock() allows to cond_resched() if its caller has
set `console_may_schedule' to 1, since
'commit 8d91f8b15361 ("printk: do cond_resched() between lines while
outputting to consoles")'.

The rules are:
-- console_lock() always sets `console_may_schedule' to 1
-- console_trylock() always sets `console_may_schedule' to 0

However, console_trylock() callers (among them is printk()) do
not always call printk() from atomic contexts, and some of them
can cond_resched() in console_unlock(), so console_trylock()
can set `console_may_schedule' to 1 for such processes.

For !CONFIG_PREEMPT_COUNT kernels, however, console_trylock()
always sets `console_may_schedule' to 0.

It's possible to drop explicit preempt_disable()/preempt_enable()
in vprintk_emit(), because console_unlock() and console_trylock()
are now smart enough:
a) console_unlock() does not cond_resched() when it's unsafe
  (console_trylock() takes care of that)
b) console_unlock() does can_use_console() check.

Signed-off-by: Sergey Senozhatsky <sergey.senozhatsky@gmail.com>
---
 kernel/printk/printk.c | 19 ++++++++++---------
 1 file changed, 10 insertions(+), 9 deletions(-)

diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
index 99925ce..097ca8b 100644
--- a/kernel/printk/printk.c
+++ b/kernel/printk/printk.c
@@ -1769,20 +1769,12 @@ asmlinkage int vprintk_emit(int facility, int level,
 	if (!in_sched) {
 		lockdep_off();
 		/*
-		 * Disable preemption to avoid being preempted while holding
-		 * console_sem which would prevent anyone from printing to
-		 * console
-		 */
-		preempt_disable();
-
-		/*
 		 * Try to acquire and then immediately release the console
 		 * semaphore.  The release will print out buffers and wake up
 		 * /dev/kmsg and syslog() users.
 		 */
 		if (console_trylock())
 			console_unlock();
-		preempt_enable();
 		lockdep_on();
 	}
 
@@ -2115,7 +2107,16 @@ int console_trylock(void)
 		return 0;
 	}
 	console_locked = 1;
-	console_may_schedule = 0;
+	/*
+	 * On !PREEMPT_COUNT kernels we can't reliably detect if it's safe
+	 * to schedule -- e.g. calling printk while holding a spin_lock,
+	 * because preempt_disable()/preempt_enable() are just barriers and
+	 * don't modify preempt_count() there. console_may_schedule is
+	 * always 0 on !PREEMPT_COUNT kernels.
+	 */
+	console_may_schedule = !oops_in_progress &&
+			preemptible() &&
+			!rcu_preempt_depth();
 	return 1;
 }
 EXPORT_SYMBOL(console_trylock);
-- 
2.7.0

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

#1315542 — [RFC][PATCH v3 1/4] printk: move can_use_console out of console_trylock_for_printk

vprintk_emit() disables preemption around console_trylock_for_printk()
and console_unlock() calls for a strong reason -- can_use_console()
check. The thing is that vprintl_emit() can be called on a CPU that
is not fully brought up yet (!cpu_online()), which potentially can
cause problems if console driver wants to access per-cpu data. A
console driver can explicitly state that it's safe to call it from
!online cpu by setting CON_ANYTIME bit in console ->flags. That's why
for !cpu_online() can_use_console() iterates all the console to find
out if there is a CON_ANYTIME console, otherwise console_unlock() must
be avoided.

can_use_console() ensures that console_unlock() call is safe in
vprintk_emit() only; console_lock() and console_trylock() are not
covered by this check. Even though call_console_drivers(), invoked
from console_cont_flush() and console_unlock(), tests
`!cpu_online() && CON_ANYTIME' for_each_console(), it may be
too late, which can result in messages loss.

Assume that we have 2 cpus -- CPU0 is online, CPU1 is !online, and
no CON_ANYTIME consoles available.

CPU0 online                        CPU1 !online
                                 console_trylock()
                                 ...
                                 console_unlock()
                                   console_cont_flush
                                     spin_lock logbuf_lock
                                     if (!cont.len) {
                                        spin_unlock logbuf_lock
                                        return
                                     }
                                   for (;;) {
vprintk_emit
  spin_lock logbuf_lock
  log_store
  spin_unlock logbuf_lock
                                     spin_lock logbuf_lock
  !console_trylock_for_printk        msg_print_text
 return                              console_idx = log_next()
                                     console_seq++
                                     console_prev = msg->flags
                                     spin_unlock logbuf_lock

                                     call_console_drivers()
                                       for_each_console(con) {
                                         if (!cpu_online() &&
                                             !(con->flags & CON_ANYTIME))
                                                 continue;
                                         }
                                   /*
                                    * no message printed, we lost it
                                    */
vprintk_emit
  spin_lock logbuf_lock
  log_store
  spin_unlock logbuf_lock
  !console_trylock_for_printk
 return
                                   /*
                                    * go to the beginning of the loop,
                                    * find out there are new messages,
                                    * lose it
                                    */
                                   }

console_trylock()/console_lock() call on CPU1 may come from cpu
notifier registered on that CPU. Since notifiers are not getting
unregistered when CPU is going DOWN, all of the notifiers receive
notifications during CPU UP. For example, on my x86_64, I see around 50
notification sent from offline CPU to itself

 [swapper/2] from cpu:2 to:2 action:CPU_STARTING hotplug_hrtick
 [swapper/2] from cpu:2 to:2 action:CPU_STARTING blk_mq_main_cpu_notify
 [swapper/2] from cpu:2 to:2 action:CPU_STARTING blk_mq_queue_reinit_notify
 [swapper/2] from cpu:2 to:2 action:CPU_STARTING console_cpu_notify

while doing
  echo 0 > /sys/devices/system/cpu/cpu2/online
  echo 1 > /sys/devices/system/cpu/cpu2/online

So grabbing the console_sem lock while CPU is !online is possible,
in theory.

This patch moves can_use_console() check out of
console_trylock_for_printk(). Instead it calls it in
console_unlock(), so now console_lock()/console_unlock() are
also 'protected' by can_use_console().

Signed-off-by: Sergey Senozhatsky <sergey.senozhatsky@gmail.com>
---
 kernel/printk/printk.c | 86 ++++++++++++++++++++++++--------------------------
 1 file changed, 42 insertions(+), 44 deletions(-)

diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
index 7ebcfea..c39232a 100644
--- a/kernel/printk/printk.c
+++ b/kernel/printk/printk.c
@@ -1485,33 +1485,6 @@ static void zap_locks(void)
 }
 
 /*
- * Check if we have any console that is capable of printing while cpu is
- * booting or shutting down. Requires console_sem.
- */
-static int have_callable_console(void)
-{
-	struct console *con;
-
-	for_each_console(con)
-		if (con->flags & CON_ANYTIME)
-			return 1;
-
-	return 0;
-}
-
-/*
- * Can we actually use the console at this time on this cpu?
- *
- * Console drivers may assume that per-cpu resources have been allocated. So
- * unless they're explicitly marked as being able to cope (CON_ANYTIME) don't
- * call them until this CPU is officially up.
- */
-static inline int can_use_console(unsigned int cpu)
-{
-	return cpu_online(cpu) || have_callable_console();
-}
-
-/*
  * Try to get console ownership to actually show the kernel
  * messages from a 'printk'. Return true (and with the
  * console_lock held, and 'console_locked' set) if it
@@ -1519,21 +1492,7 @@ static inline int can_use_console(unsigned int cpu)
  */
 static int console_trylock_for_printk(void)
 {
-	unsigned int cpu = smp_processor_id();
-
-	if (!console_trylock())
-		return 0;
-	/*
-	 * If we can't use the console, we need to release the console
-	 * semaphore by hand to avoid flushing the buffer. We need to hold the
-	 * console semaphore in order to do this test safely.
-	 */
-	if (!can_use_console(cpu)) {
-		console_locked = 0;
-		up_console_sem();
-		return 0;
-	}
-	return 1;
+	return console_trylock();
 }
 
 int printk_delay_msec __read_mostly;
@@ -1683,7 +1642,6 @@ asmlinkage int vprintk_emit(int facility, int level,
 	boot_delay_msec(level);
 	printk_delay();
 
-	/* This stops the holder of console_sem just where we want him */
 	local_irq_save(flags);
 	this_cpu = smp_processor_id();
 
@@ -1707,6 +1665,7 @@ asmlinkage int vprintk_emit(int facility, int level,
 	}
 
 	lockdep_off();
+	/* This stops the holder of console_sem just where we want him */
 	raw_spin_lock(&logbuf_lock);
 	logbuf_cpu = this_cpu;
 
@@ -2177,6 +2136,33 @@ int is_console_locked(void)
 	return console_locked;
 }
 
+/*
+ * Check if we have any console that is capable of printing while cpu is
+ * booting or shutting down. Requires console_sem.
+ */
+static int have_callable_console(void)
+{
+	struct console *con;
+
+	for_each_console(con)
+		if (con->flags & CON_ANYTIME)
+			return 1;
+
+	return 0;
+}
+
+/*
+ * Can we actually use the console at this time on this cpu?
+ *
+ * Console drivers may assume that per-cpu resources have been allocated. So
+ * unless they're explicitly marked as being able to cope (CON_ANYTIME) don't
+ * call them until this CPU is officially up.
+ */
+static inline int can_use_console(void)
+{
+	return cpu_online(raw_smp_processor_id()) || have_callable_console();
+}
+
 static void console_cont_flush(char *text, size_t size)
 {
 	unsigned long flags;
@@ -2247,9 +2233,21 @@ void console_unlock(void)
 	do_cond_resched = console_may_schedule;
 	console_may_schedule = 0;
 
+again:
+	/*
+	 * We released the console_sem lock, so we need to recheck if
+	 * cpu is online and (if not) is there at least one CON_ANYTIME
+	 * console.
+	 */
+	if (!can_use_console()) {
+		console_locked = 0;
+		up_console_sem();
+		return;
+	}
+
 	/* flush buffered message fragment immediately to console */
 	console_cont_flush(text, sizeof(text));
-again:
+
 	for (;;) {
 		struct printk_log *msg;
 		size_t ext_len = 0;
-- 
2.7.0

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web