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

[Petr Mladek]

On Wed 2016-01-20 12:50:56, Sergey Senozhatsky wrote:
> so in_atomic() covers both preemption disabled and in_interrupt().

I see.

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

I though about it from some other side and I wonder if we need all
the console_may_schedule stuff at all.

printk_emit() has the following code:

	/* This stops the holder of console_sem just where we want him */
	local_irq_save(flags);
[...]
	lockdep_off();
	raw_spin_lock(&logbuf_lock);
	logbuf_cpu = this_cpu;

[...]

	logbuf_cpu = UINT_MAX;
	raw_spin_unlock(&logbuf_lock);
	lockdep_on();
	local_irq_restore(flags);

	/* If called from the scheduler, we can not call up(). */
	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_for_printk())
			console_unlock();
		preempt_enable();


First, the message "This stops the holder of console_sem just where we
want him" is suspitious. It is there sice the initial git commit on
2005-04-16. I do not understand how this could block the console
holder on another CPU. I think that it rather allows to make the
recursion detection without the lockbuf lock. But this is not
that important.

More interesting is the counter part:

	raw_spin_unlock(&logbuf_lock);
	lockdep_on();
	local_irq_restore(flags);

raw_spin_unlock() calls preempt_enable() that calls
preempt_schedule(). It does nothing here because IRQs
are still disabled.

But if we do not need the lockdep_on() hack, we could use
raw_spin_unlock_irqrestore(&lockbuf_lock, flags). It means
that we do not call cond_resched here "only by chance".

In each case, preemtible kernel seems to be free to reschedule
after we enable the inrerrupts. By other words, preemptible kernel
seems to be able to reschedule in printk() already these days.
Why it might work?

I think that it might work because cond_resched() or rather
preempt_schedule() are clever enough. They both check
preempt_count and do nothing if preemtion is disabled.

As a result, I think that we do not need the extra checks
for the save context in printk(). IMHO, it is safe to remove
all the console_may_schedule stuff and also remove the extra
preempt_disable/preempt_enable() in vprintk_emit().

Or did I miss anything?

Best Regards,
Petr