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