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

[Sergey Senozhatsky]

Hello,

On (01/19/16 16:18), Petr Mladek wrote:
[..]
> > > >  	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)
> 

I had in_interrupt() check in trylock, but dropped it. As of 4.4 in_interrupt()
does the following

#define hardirq_count()	(preempt_count() & HARDIRQ_MASK)
#define softirq_count()	(preempt_count() & SOFTIRQ_MASK)
#define irq_count()	(preempt_count() & (HARDIRQ_MASK | SOFTIRQ_MASK \
				 | NMI_MASK))

#define in_interrupt()		(irq_count())

while in_atomic() does

#define in_atomic()	(preempt_count() != 0)

so in_atomic() covers both preemption disabled and in_interrupt().


I take your `non-preemptible kernel' point, thanks.


> 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().

well, if we run a !PREEMPT_COUNT kernel, then
#define preempt_disable()	barrier()
#define preempt_enable()	barrier()
#define preemptible()		0

so I don't think that preempt_disable()/preempt_enable() were for
non-preempt kernels there.

otoh, preempt_count still counts irqs, because in_interrupt() (and friends)
macro is supposed to work regardless the preempt config selection, so we
just lose preempt_disable bit from preempt_count... hm... I think I'll
just introduce preemptible() check there.

for preempt kernels, preemptible() does
#define preemptible()	(preempt_count() == 0 && !irqs_disabled())

and for !preempt kernels
#define preemptible()	0

iow, no cond_resched() in console_unlock() called from vprintk_emit()
on non-preempt kernels.

so the console_may_schedule now turns into (composed in mail-app, not
actually tested):


console_may_schedule = !oops_in_progress && preemptible() &&
			!rcu_preempt_depth();

	/*
	 * or a slightly less readable version
	 * !(oops_in_progress || !preemptible() || rcu_preempt_depth())
	 */


preemptible() implies "preempt_enabled && !in_interrupt() && !irqs_disabled()";
or is always 0 (so it can be optimized out).


will test it, but looks legit to me.


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

CPU's run queue is not marked online yet at this point (it becomes online
in response to CPU_ONLINE notification). so there is not too many tasks to
schedule on that CPU -- swapper/X perhaps.. migration/X?.

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

	-ss