Re: [RFC][PATCH -next 1/2] printk: move can_use_console out of console_trylock_for_printk

[Petr Mladek <pmladek@suse.com>]

On Wed 2016-01-20 00:00:40, Sergey Senozhatsky wrote:
> > But do we really need to repeat the check in every cycle?
> 
> well, on every iteration in the best case we check cpu_online()
> only. which is what we would have done anyway in vprintk_emit(),
> so no additional checks added. at the same time call_console_drivers
> does not do '!cpu_online && !CON_ANYTIME' for each console now, so
> in some sense there are less checks now (this is far even from a
> micro-optimization, just noted).

Hmm, we need to keep the check in call_console_drivers(). It iterates
over all registered consoles. Only some of them could habe CON_ANYTIME
flag. We need to skip the others when the CPU is not online.


> console_unlock()    /* w/o can_use_console() in logbuf_lock section */
> ....
> again:
> 	for (;;) {
> 		raw_spin_lock_irqsave logbuf_lock
> 		msg_print_text
> 		raw_spin_unlock logbuf_lock
> 		call_console_drivers
> 		local_irq_restore
> 	}
> 
> 	up_console_sem
> 
> 	raw_spin_lock logbuf_lock
> 	retry = console_seq != log_next_seq
> 	raw_spin_unlock_irqrestore logbuf_lock
> 	
> 	if retry && console_trylock()
> 		goto again;
> 
> 
> when we up_console_sem(), consoles may appear and disappear, since we
> don't keep the semaphore. Suppose that we have OFFLINE cpu and we had a
> CON_ANYTIME console, but in between up_console_sem--console_trylock
> that single CON_ANYTIME console was removed. So now we have !cpu_online
> and !CON_ANYTIME.

Ah, I have missed that the console_sem is released/taken before goto
again. Hmm, your proposed solution adds even more twists into the code.
I think how to make it easier. I think that we could move the again:
target and call console_cont_flush() when there is some new content.
It is a corner case, anyway. Then we could do:


	do_cond_resched = console_may_schedule;
	console_may_schedule = 0;

+again:
+	if (!can_use_console()) {
+		console_locked = 0;
+		up_console_sem();
+		return;
	}

	/* flush buffered message fragment immediately to console */
	console_cont_flush(text, sizeof(text));
-again:
	for (;;) {
		struct printk_log *msg;


Then we remove this check from console_trylock_for_printk() and
eventually remove this function altogether.

It means that the code will be easier and protected against the
theoretical race.

How does that sound, please?

Best Regards,
Petr