Re: [PATCH v10 1/2] printk: Make printk() completely async

[Sergey Senozhatsky]

On (08/19/16 21:00), Jan Kara wrote:
> > > depending on .config BUG() may never return back -- passing control
> > > to do_exit(), so printk_deferred_exit() won't be executed. thus we
> > > probably need to have a per-cpu variable that would indicate that
> > > we are in deferred_bug. hm... but do we really need deferred BUG()
> > > in the first place?
> > 
> > Good question. I am not aware of any BUG_ON() that would be called from
> > wake_up_process() but it is hard to check everything.
> > 
> > A conservative approach would be to force synchronous printk from
> > BUG_ON().
> 
> Just a quick thought: Cannot we just do printk_deferred_enter() when we are
> about to call into the scheduler from printk code and printk_deferred_exit()
> when leaving it? That would look like the least error-prone way how
> handling this kind of recursion...

interesting idea.
printk_deferred_enter() increments preempt count, so there may be additional
obstacles and, as a result, ad-hocs, that scheduler people will sincerely hate.
need to think more.

> OTOH there's also the other possible direction for the recursion when we
> are in the scheduler, holding some scheduler locks, decide to WARN which
> enters printk, that ends up calling wake_up_process() which deadlocks
> on scheduler locks... I don't see how to handle this type of recursion
> inside the printk code itself easily and so far the answer was - use
> printk_deferred() in the scheduler and don't use WARN...

the recursion detection is really tricky, yes. it seems (and I haven't
thought of it good enough) to be a bit simpler when we operate in async
printk mode, because we remove this uncontrollable console_unlock().
so we can do something like this:

vprintk_emit(....)
{
	local_irq_save();

	if (this_cpu_read(in_printk)) {
		log_store(BUG: printk recursion!");
		goto out;
	}

	this_cpu_write(in_printk) = 1;

	raw_spin_lock(&logbuf_lock);
	log_store();
	raw_spin_unlock(&logbuf_lock);

	if (!in_sched) {
		if (console_loglevel != CONSOLE_LOGLEVEL_MOTORMOUTH &&
				can_printk_async()) {
			printk_kthread_need_flush_console = true;
			wake_up_process(printk_kthread);
		}
	}

	this_cpu_write(in_printk) = 0;
out:
	local_irq_restore();
}

async printk mode from this point of view is sort of atomic.
we can even set different values of per-CPU `in_printk' on various
stages of printk, which will permit to have better recursion handling.
for example, if we recurse from raw_spin_unlock(&logbuf_lock) then we
must re-init logbuf_lock, because it's 99% corrupted... and so on. but
I haven't really thought of it yet. it obviously doesn't work for sync
printk mode.

> Hum, maybe we could add lockdep annotation to a WARN_ON and BUG_ON macros so
> that it would grab and release console_sem (even if the condition is false).
> That way we'd get lockdep splats for all the possible WARN_ON and BUG_ON
> calls that could deadlock.

hm.

	-ss