* [PATCH v4 1/3] printk: move can_use_console out of console_trylock_for_printk
2016-02-12 18:37 [PATCH v4 0/2] let printk()/console_trylock() callers to cond_resched() Sergey Senozhatsky
@ 2016-02-12 18:37 ` Sergey Senozhatsky
2016-02-12 18:37 ` [PATCH v4 2/3] printk: set may_schedule for some of console_trylock callers Sergey Senozhatsky
2016-02-12 18:37 ` [PATCH v4 3/3] printk: check CON_ENABLED in have_callable_console() Sergey Senozhatsky
2 siblings, 0 replies; 8+ messages in thread
From: Sergey Senozhatsky @ 2016-02-12 18:37 UTC (permalink / raw)
To: Andrew Morton
Cc: Petr Mladek, Jan Kara, Tejun Heo, Kyle McMartin, Dave Jones,
Calvin Owens, linux-kernel, Sergey Senozhatsky,
Sergey Senozhatsky
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
notifiers 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(). This also means that
console_trylock_for_printk() is not really needed anymore
and can be removed.
Reviewed-by: Petr Mladek <pmladek@suse.com>
Signed-off-by: Sergey Senozhatsky <sergey.senozhatsky@gmail.com>
---
kernel/printk/printk.c | 97 ++++++++++++++++++++++----------------------------
1 file changed, 42 insertions(+), 55 deletions(-)
diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
index 7ebcfea..0d65a81 100644
--- a/kernel/printk/printk.c
+++ b/kernel/printk/printk.c
@@ -1484,58 +1484,6 @@ static void zap_locks(void)
sema_init(&console_sem, 1);
}
-/*
- * 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
- * is successful, false otherwise.
- */
-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;
-}
-
int printk_delay_msec __read_mostly;
static inline void printk_delay(void)
@@ -1683,7 +1631,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 +1654,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;
@@ -1832,7 +1780,7 @@ asmlinkage int vprintk_emit(int facility, int level,
* semaphore. The release will print out buffers and wake up
* /dev/kmsg and syslog() users.
*/
- if (console_trylock_for_printk())
+ if (console_trylock())
console_unlock();
preempt_enable();
lockdep_on();
@@ -2177,6 +2125,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 +2222,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.1
^ permalink raw reply related [flat|nested] 8+ messages in thread* [PATCH v4 2/3] printk: set may_schedule for some of console_trylock callers
2016-02-12 18:37 [PATCH v4 0/2] let printk()/console_trylock() callers to cond_resched() Sergey Senozhatsky
2016-02-12 18:37 ` [PATCH v4 1/3] printk: move can_use_console out of console_trylock_for_printk Sergey Senozhatsky
@ 2016-02-12 18:37 ` Sergey Senozhatsky
2016-02-16 14:43 ` Petr Mladek
2016-02-12 18:37 ` [PATCH v4 3/3] printk: check CON_ENABLED in have_callable_console() Sergey Senozhatsky
2 siblings, 1 reply; 8+ messages in thread
From: Sergey Senozhatsky @ 2016-02-12 18:37 UTC (permalink / raw)
To: Andrew Morton
Cc: Petr Mladek, Jan Kara, Tejun Heo, Kyle McMartin, Dave Jones,
Calvin Owens, linux-kernel, Sergey Senozhatsky,
Sergey Senozhatsky
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 | 23 ++++++++++++++---------
1 file changed, 14 insertions(+), 9 deletions(-)
diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
index 0d65a81..9f0c3e1 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,20 @@ int console_trylock(void)
return 0;
}
console_locked = 1;
- console_may_schedule = 0;
+ /*
+ * When PREEMPT_COUNT disabled 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 there
+ * and preempt_count() is always 0.
+ *
+ * RCU read sections have a separate preemption counter when
+ * PREEMPT_RCU enabled thus we must take extra care and check
+ * rcu_preempt_depth(), otherwise RCU read sections modify
+ * preempt_count().
+ */
+ console_may_schedule = !oops_in_progress &&
+ preemptible() &&
+ !rcu_preempt_depth();
return 1;
}
EXPORT_SYMBOL(console_trylock);
--
2.7.1
^ permalink raw reply related [flat|nested] 8+ messages in thread* [PATCH v4 3/3] printk: check CON_ENABLED in have_callable_console()
2016-02-12 18:37 [PATCH v4 0/2] let printk()/console_trylock() callers to cond_resched() Sergey Senozhatsky
2016-02-12 18:37 ` [PATCH v4 1/3] printk: move can_use_console out of console_trylock_for_printk Sergey Senozhatsky
2016-02-12 18:37 ` [PATCH v4 2/3] printk: set may_schedule for some of console_trylock callers Sergey Senozhatsky
@ 2016-02-12 18:37 ` Sergey Senozhatsky
2016-02-16 14:44 ` Petr Mladek
2 siblings, 1 reply; 8+ messages in thread
From: Sergey Senozhatsky @ 2016-02-12 18:37 UTC (permalink / raw)
To: Andrew Morton
Cc: Petr Mladek, Jan Kara, Tejun Heo, Kyle McMartin, Dave Jones,
Calvin Owens, linux-kernel, Sergey Senozhatsky,
Sergey Senozhatsky
have_callable_console() must also test CON_ENABLED bit, not just
CON_ANYTIME. We may have disabled CON_ANYTIME console so printk
can wrongly assume that it's safe to call_console_drivers().
Signed-off-by: Sergey Senozhatsky <sergey.senozhatsky@gmail.com>
---
kernel/printk/printk.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
index 9f0c3e1..9917f69 100644
--- a/kernel/printk/printk.c
+++ b/kernel/printk/printk.c
@@ -2139,7 +2139,8 @@ static int have_callable_console(void)
struct console *con;
for_each_console(con)
- if (con->flags & CON_ANYTIME)
+ if ((con->flags & CON_ENABLED) &&
+ (con->flags & CON_ANYTIME))
return 1;
return 0;
--
2.7.1
^ permalink raw reply related [flat|nested] 8+ messages in thread