Hello,
On (04/23/16 08:56), Jan Kara wrote:
> >
> > Signed-off-by: Sergey Senozhatsky <sergey.senozhat...@gmail.com>
>
> The patch looks good to me. One suggestion below:
>
> > @@ -1785,7 +1782,7 @@ asmlinkage int vprintk_emit(int facility, int level,
> > * operate in sync mode once panic() occurred.
> > */
> > if (console_loglevel != CONSOLE_LOGLEVEL_MOTORMOUTH &&
> > - printk_kthread) {
> > + !printk_sync && printk_kthread) {
> > /* Offload printing to a schedulable context. */
> > printk_kthread_need_flush_console = true;
> > wake_up_process(printk_kthread);
>
> It would seem more future-proof to hide '!printk_sync && printk_kthread'
> into a wrapper function as it is somewhat subtle detail that printk_kthread
> needn't exist while !printk_sync and I can imagine someone forgetting to
> check that in the future. Something like 'can_print_async()'? But I don't
> feel too strongly about that so feel free to add:
hm, yes. this is what I eventually do in "yet to be posted"
make-console_unlock()-async patch. I move printing kthread
wakeup-s and those async printing checks out of vprintk_emit()
and wake_up_klogd_work_func() to a special function:
static bool console_unlock_async_flush(void)
{
...
if (console_loglevel != CONSOLE_LOGLEVEL_MOTORMOUTH &&
!printk_sync && printk_kthread) {
/* Offload printing to a schedulable context. */
printk_kthread_need_flush_console = true;
console_locked = 0;
up_console_sem();
wake_up_process(printk_kthread);
return true;
}
return false;
}
so async_printk flags live in one place (which makes it easier
to maintain) and vprintk_emit()/wake_up_klogd_work_func() simply
do:
if (console_trylock())
console_unlock();
console_unlock() is the one who decides if it can do async
printk or a 'direct printing' via console_flush_and_unlock().
void console_unlock(void)
{
if (console_unlock_async_flush())
return;
console_flush_and_unlock();
}
console_flush_and_unlock() is what was previously known
as console_unlock() - emit the messages and call_console_drivers().
I guess I can send out an updated version of 0003 as a reply
to the initial patch and hide '!printk_sync && printk_kthread'.
> Reviewed-by: Jan Kara <j...@suse.cz>
>
> regardless whether you change this or not.
thanks.
-ss
