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

[Petr Mladek]

On Tue 2016-01-19 09:42:36, Sergey Senozhatsky wrote:
> Hello,
> thanks for review.
> 
> On (01/18/16 16:42), Petr Mladek wrote:
> > On Thu 2016-01-14 13:57:22, Sergey Senozhatsky wrote:
> > > vprintk_emit() disables preemption around console_trylock_for_printk()
> > > and console_unlock() calls for a strong reason -- can_use_console()
> > > check. The thing is that vprintl_emit() can be called on a CPU that
> > > is not fully brought up yet (!cpu_online()), which potentially can
> > > cause problems if console driver accesses per-cpu data. A console
> > > driver can explicitly state that it's safe to call it from !online
> > > cpu by setting CON_ANYTIME bit in console ->flags. That's why for
> > > !cpu_online() can_use_console() iterates all the console to find out
> > > if there is a CON_ANYTIME console, otherwise console_unlock() must be
> > > avoided.
> > > 
> > > call_console_drivers(), called from console_cont_flush() and
> > > console_unlock(), does the same test during for_each_console() loop.
> > > However, we can have the following corner case. Assume that we have 2
> > > cpus -- CPU0 is online, CPU1 is !online; and no CON_ANYTIME consoles
> > > available.
> > > 
> > > CPU0 online                        CPU1 !online
> > >                                  console_trylock()
> > >                                  ...
> > >                                  console_unlock()
> > 
> > Please, where this console_unlock() comes from?
> 
> from UP* or DOWN* (_PREPARE f.e.) notifiers on this CPU, for example, we don't
> know what's going on there. what prevents it from calling console_trylock(),
> grabbing the console_sem and eventually doing console_unlock()? there is
> a can_use_console() check, but it handles only one case -- printk().

So, is it a theoretical problem or do you know about any
particular path where this happens?

Well, it might make sense to get rid of console_trylock_for_printk()
and do the can_use_console() check at the beginning of
unlock_console(). I mean to do

-	if (console_trylock_for_printk())
+	if (console_trylock())
		unlock_console();

But do we really need to repeat the check in every cycle?
can_use_console() checks available consoles and if the CPU
is online. Consoles could not get added or removed when
we own console_sem. It seems that CPUs get disabled
in a process context. Therefore it seems that it might happen
only when unlock_console() gets rescheduled. I guess that
it could not get scheduled back on an offline CPU. So, it
seems that it is enough to check can_use_console() only once at
the beginning.

Or did I miss anything?

Best Regards,
Petr