Re: [RFC][PATCH -next 2/2] printk: set may_schedule for some of console_trylock callers

[Petr Mladek]

On Tue 2016-01-19 10:15:10, Sergey Senozhatsky wrote:
> Thanks for the review.
> 
> On (01/18/16 17:17), Petr Mladek wrote:
> > On Sun 2016-01-17 23:23:32, Sergey Senozhatsky wrote:
> > >  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
> > > 
> > > However, console_trylock() callers (among them is printk()) are
> > > not necessarily executing in atomic contexts, and some of them
> > > can cond_resched() in console_unlock(). So console_trylock()
> > > can set `console_may_schedule' to 0 only if cond_resched() is
> > > invalid in the current context, and set it to 1 otherwise.
> > > 
> > > The patch also drops explicit preempt_disable()/preempt_enable()
> > > calls in vprintk_emit().
> > 
> > I do not see any explanation why it is safe to remove these
> > calls in this patch. If they were required only because of the
> > can_use_console() call
> 
> The comment in the code states
> 
> 	/*
> 	 * Disable preemption to avoid being preempted while holding
> 	 * console_sem which would prevent anyone from printing to
> 	 * console
> 	 */

Thanks for the pointer. I missed this in the patch and it was not
obvious from the commit message.

> which is not really a problem -- we schedule from console_unlock() with
> the console_sem being held, it's fine.
> 
> https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/kernel/printk/printk.c?id=8d91f8b15361dfb438ab6eb3b319e2ded43458ff

Yup, I agree that this commit is contradicting the above comment.
The question is if the comment describes the only reason for
disabled preemption.

> [..]
> > >  	console_locked = 1;
> > > -	console_may_schedule = 0;
> > > +	console_may_schedule = !(oops_in_progress ||
> > > +			irqs_disabled() ||

IMHO, we should also check if we are in irq context or not.

> > > +			in_atomic() ||

Hmm, it seems that in_atomic() is not safe enough. There is the
following comment:

/*
 * Are we running in atomic context?  WARNING: this macro cannot
 * always detect atomic context; in particular, it cannot know about
 * held spinlocks in non-preemptible kernels.  Thus it should not be
 * used in the general case to determine whether sleeping is possible.
 * Do not use in_atomic() in driver code.
 */
#define in_atomic()	(preempt_count() != 0)

It might mean that there is no safe way to detect a safe context
on non-preemptible kernels. This might be the most important reason
why we disable preemption when calling console in vprint_emit().


> > > +			rcu_preempt_depth());
> > 
> > Is it safe to call cond_resched() when the CPU is not online
> > and preemption is enabled, please? Your previous patch
> > suggests that this situation might happen. I guess that
> > it might break the CPU initialization code.
> 
> CPU notifies are preemptible. CPU_UP_PREPARE/etc. can schedule,
> there are GFP_KERNEL kmalloc-s from CPU_UP_PREPARE (where cpu
> is not yet online), mutex_lock() calls in cpu_notify handlers,
> and so on.

Hmm, the notifiers are called via __raw_notifier_call_chain().
There is a comment above this function:

 *	Calls each function in a notifier chain in turn.  The functions
 *	run in an undefined context.
 *	All locking must be provided by the caller.

But hmm, you are right that the notifiers do malloc, take mutextes,
etc. The question is if schedule does something in this case. I would
expect that the is no running task assigned to this CPU at this stage.
So, cond_resched is probably a noop in this case.

I am always surprised that printk-world is so tricky.

Best Regards,
Petr