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

From: Sergey Senozhatsky
Date: Tue Jan 19 2016 - 22:50:42 EST


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