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

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

[Sergey Senozhatsky]

[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

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

[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
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

[RFC][PATCH v3 2/4] printk: do not console_cont_flush() on every jump to again

[Sergey Senozhatsky]

Every jump to `again' label will call console_cont_flush(),
which is not really something big -- it just adds one extra
raw_spin_lock_irqsave/raw_spin_unlock_irqrestore. However, to
keep the previous behaviour we can call console_cont_flush()
only when `retry' is false -- which happens once, all jumps
to `again' label have `retry' set to true.

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

diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
index c39232a..dc722fc 100644
--- a/kernel/printk/printk.c
+++ b/kernel/printk/printk.c
@@ -2213,7 +2213,7 @@ void console_unlock(void)
 	static u64 seen_seq;
 	unsigned long flags;
 	bool wake_klogd = false;
-	bool do_cond_resched, retry;
+	bool do_cond_resched, retry = false;
 
 	if (console_suspended) {
 		up_console_sem();
@@ -2246,7 +2246,8 @@ again:
 	}
 
 	/* flush buffered message fragment immediately to console */
-	console_cont_flush(text, sizeof(text));
+	if (!retry)
+		console_cont_flush(text, sizeof(text));
 
 	for (;;) {
 		struct printk_log *msg;
-- 
2.7.0

[RFC][PATCH v3 3/4] printk: remove console_trylock_for_printk

[Sergey Senozhatsky]

Remove console_trylock_for_printk() function, it can be
replaced with console_trylock() in vprintk_emit().

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

diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
index dc722fc..99925ce 100644
--- a/kernel/printk/printk.c
+++ b/kernel/printk/printk.c
@@ -1484,17 +1484,6 @@ static void zap_locks(void)
 	sema_init(&console_sem, 1);
 }
 
-/*
- * 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)
-{
-	return console_trylock();
-}
-
 int printk_delay_msec __read_mostly;
 
 static inline void printk_delay(void)
@@ -1791,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();
-- 
2.7.0

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

[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 | 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

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

[Sergey Senozhatsky]

Hello,

On (01/23/16 17:15), Sergey Senozhatsky wrote:
> [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').

gentlemen, did you get a chance to look into this?

	-ss

> 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
> 

Re: [RFC][PATCH v3 1/4] printk: move can_use_console out of console_trylock_for_printk

[Petr Mladek]

On Sat 2016-01-23 17:15:10, Sergey Senozhatsky wrote:
> 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
> @@ -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();
>  }

I would personally remove console_trylock_for_printk() already in this
patch. I mean to fold the 3rd patch into this one.

>  int printk_delay_msec __read_mostly;

> @@ -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;
> +	}

This is a bug fix and a nice clean up together.

Reviewed-by: Petr Mladek <pmladek@suse.com>

Best Regards,
Petr

Re: [RFC][PATCH v3 2/4] printk: do not console_cont_flush() on every jump to again

[Petr Mladek]

On Sat 2016-01-23 17:15:11, Sergey Senozhatsky wrote:
> Every jump to `again' label will call console_cont_flush(),
> which is not really something big -- it just adds one extra
> raw_spin_lock_irqsave/raw_spin_unlock_irqrestore. However, to
> keep the previous behaviour we can call console_cont_flush()
> only when `retry' is false -- which happens once, all jumps
> to `again' label have `retry' set to true.

The patch is correct and restores the original behavior, so

Reviewed-by: Petr Mladek <pmladek@suse.com>

But I would personally omit it. We see the cont buffer earlier
without this patch.

Best Regards,
Petr

PS: I will review the other patches tomorrow.

Re: [RFC][PATCH v3 1/4] printk: move can_use_console out of console_trylock_for_printk

[Sergey Senozhatsky]

Hello Petr,

On (02/10/16 17:48), Petr Mladek wrote:
[..]
> I would personally remove console_trylock_for_printk() already in this
> patch. I mean to fold the 3rd patch into this one.

will do.

> >  int printk_delay_msec __read_mostly;
> 
> > @@ -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;
> > +	}
> 
> This is a bug fix and a nice clean up together.
> 
> Reviewed-by: Petr Mladek <pmladek@suse.com>

thanks.

	-ss

Re: [RFC][PATCH v3 2/4] printk: do not console_cont_flush() on every jump to again

[Sergey Senozhatsky]

Hello Petr,

On (02/10/16 17:58), Petr Mladek wrote:
> On Sat 2016-01-23 17:15:11, Sergey Senozhatsky wrote:
> > Every jump to `again' label will call console_cont_flush(),
> > which is not really something big -- it just adds one extra
> > raw_spin_lock_irqsave/raw_spin_unlock_irqrestore. However, to
> > keep the previous behaviour we can call console_cont_flush()
> > only when `retry' is false -- which happens once, all jumps
> > to `again' label have `retry' set to true.
> 
> The patch is correct and restores the original behavior, so
> 
> Reviewed-by: Petr Mladek <pmladek@suse.com>
> 
> But I would personally omit it. We see the cont buffer earlier
> without this patch.

ok.

> PS: I will review the other patches tomorrow.

thanks.

	-ss

Re: [RFC][PATCH v3 3/4] printk: remove console_trylock_for_printk

[Petr Mladek]

On Sat 2016-01-23 17:15:12, Sergey Senozhatsky wrote:
> Remove console_trylock_for_printk() function, it can be
> replaced with console_trylock() in vprintk_emit().
> 
> Signed-off-by: Sergey Senozhatsky <sergey.senozhatsky@gmail.com>

Reviewed-by: Petr Mladek <pmladek@suse.com>

But as I already mentioned. I would fold this patch into the first
one of this series.

Best Regards,
Petr

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

[Petr Mladek]

On Sat 2016-01-23 17:15:13, Sergey Senozhatsky wrote:
> 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;

We discussed this a lot but I am still a bit nervous ;-)

Avoid scheduling when oops_in_progress makes sense.

preemptible() takes care of preemption and IRQ contexts.
The comment above explains that it is safe to use here.

The check for rcu_preempt_depth() makes sense. But is it
safe, please?

rcu_preempt_depth() returns 0 if CONFIG_PREEMPT_RCU is not
enabled. It means that you are not able to detect RCU read
section and it might cause problems.

I rather add Paul into CC.

Best Regards,
Petr

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

[Sergey Senozhatsky]

Hello Petr,

On (02/11/16 15:41), Petr Mladek wrote:
[..]
> > +	console_may_schedule = !oops_in_progress &&
> > +			preemptible() &&
> > +			!rcu_preempt_depth();
> >  	return 1;
> 
> We discussed this a lot but I am still a bit nervous ;-)

sure, no prob :-)

> Avoid scheduling when oops_in_progress makes sense.
> 
> preemptible() takes care of preemption and IRQ contexts.
> The comment above explains that it is safe to use here.
> 
> The check for rcu_preempt_depth() makes sense. But is it
> safe, please?
> 
> rcu_preempt_depth() returns 0 if CONFIG_PREEMPT_RCU is not
> enabled. It means that you are not able to detect RCU read
> section and it might cause problems.

well, I believe it's ok. __rcu_read_lock() for CONFIG_PREEMPT_RCU
does current->rcu_read_lock_nesting++, so rcu_preempt_depth() works
as expected. otherwise, for !CONFIG_PREEMPT_RCU kernel,
__rcu_read_lock() does

	if (IS_ENABLED(CONFIG_PREEMPT_COUNT))
		preempt_disable()


- if we run "CONFIG_PREEMPT_RCU" then rcu_preempt_depth()
  works here.

- if we run "!CONFIG_PREEMPT_RCU && CONFIG_PREEMPT_COUNT"
  then preemptible() works for us

- if we run "!CONFIG_PREEMPT_RCU && !CONFIG_PREEMPT_COUNT"
  then preemptible() is always 0.

> I rather add Paul into CC.

thanks.

	-ss

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

[Petr Mladek]

On Fri 2016-02-12 00:02:17, Sergey Senozhatsky wrote:
> Hello Petr,
> 
> On (02/11/16 15:41), Petr Mladek wrote:
> [..]
> > > +	console_may_schedule = !oops_in_progress &&
> > > +			preemptible() &&
> > > +			!rcu_preempt_depth();
> > >  	return 1;
> > 
> > We discussed this a lot but I am still a bit nervous ;-)
> 
> sure, no prob :-)
> 
> > Avoid scheduling when oops_in_progress makes sense.
> > 
> > preemptible() takes care of preemption and IRQ contexts.
> > The comment above explains that it is safe to use here.
> > 
> > The check for rcu_preempt_depth() makes sense. But is it
> > safe, please?
> > 
> > rcu_preempt_depth() returns 0 if CONFIG_PREEMPT_RCU is not
> > enabled. It means that you are not able to detect RCU read
> > section and it might cause problems.
> 
> well, I believe it's ok. __rcu_read_lock() for CONFIG_PREEMPT_RCU
> does current->rcu_read_lock_nesting++, so rcu_preempt_depth() works
> as expected. otherwise, for !CONFIG_PREEMPT_RCU kernel,
> __rcu_read_lock() does
> 
> 	if (IS_ENABLED(CONFIG_PREEMPT_COUNT))
> 		preempt_disable()
> 
> 
> - if we run "CONFIG_PREEMPT_RCU" then rcu_preempt_depth()
>   works here.
> 
> - if we run "!CONFIG_PREEMPT_RCU && CONFIG_PREEMPT_COUNT"
>   then preemptible() works for us
> 
> - if we run "!CONFIG_PREEMPT_RCU && !CONFIG_PREEMPT_COUNT"
>   then preemptible() is always 0.

I feel convinced. But we should somehow document it. I think how
to do it effectively. I think that the following text would help
me if I read it:

	/*
	 * Safe context for rescheduling is detected only when
	 * PREEMPT_COUNT is enabled. preemptible() always returns
	 * false otherwise.
	 *
	 * RCU read sections must be detected separately. They
	 * have a separate preemption counter when PREEMPT_RCU
	 * is enabled.
	 */

I wanted to highlight why exactly the check returns 0 in !PREEMPT_COUNT
kernel. I missed this a bit in you original comment. But feel free
to change it as you like.

Best Regards,
Petr

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

[Sergey Senozhatsky]

On (02/11/16 17:10), Petr Mladek wrote:
[..]
> > well, I believe it's ok. __rcu_read_lock() for CONFIG_PREEMPT_RCU
> > does current->rcu_read_lock_nesting++, so rcu_preempt_depth() works
> > as expected. otherwise, for !CONFIG_PREEMPT_RCU kernel,
> > __rcu_read_lock() does
> > 
> > 	if (IS_ENABLED(CONFIG_PREEMPT_COUNT))
> > 		preempt_disable()
> > 
> > 
> > - if we run "CONFIG_PREEMPT_RCU" then rcu_preempt_depth()
> >   works here.
> > 
> > - if we run "!CONFIG_PREEMPT_RCU && CONFIG_PREEMPT_COUNT"
> >   then preemptible() works for us
> > 
> > - if we run "!CONFIG_PREEMPT_RCU && !CONFIG_PREEMPT_COUNT"
> >   then preemptible() is always 0.
> 
> I feel convinced. But we should somehow document it. I think how
> to do it effectively. I think that the following text would help
> me if I read it:
> 
> 	/*
> 	 * Safe context for rescheduling is detected only when
> 	 * PREEMPT_COUNT is enabled. preemptible() always returns
> 	 * false otherwise.
> 	 *
> 	 * RCU read sections must be detected separately. They
> 	 * have a separate preemption counter when PREEMPT_RCU
> 	 * is enabled.
> 	 */
> 
> I wanted to highlight why exactly the check returns 0 in !PREEMPT_COUNT
> kernel. I missed this a bit in you original comment. But feel free
> to change it as you like.

good point. thanks! will re-spin the patch set later today,
have no reliable internet connection at the moment.

	-ss