Re: [PATCH v12 3/3] printk: make printk.synchronous param rw

[Sergey Senozhatsky]

Hello,

On (04/23/16 08:56), Jan Kara wrote:
> > 
> > Signed-off-by: Sergey Senozhatsky <sergey.senozhatsky@xxxxxxxxx>
> 
> 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 <jack@xxxxxxx>
> 
> regardless whether you change this or not.

thanks.

	-ss