Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1385596

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

Show all headers | View raw

Hello,

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

thanks.

	-ss

Back to linux.kernel | Previous | Next — Previous in thread | Next in thread | Find similar | Unroll thread

Thread

[PATCH v12 3/3] printk: make printk.synchronous param rw Sergey Senozhatsky <sergey.senozhatsky@gmail.com> - 2016-04-22 15:00 +0200
  Re: [PATCH v12 3/3] printk: make printk.synchronous param rw Jan Kara <jack@suse.cz> - 2016-04-23 09:00 +0200
    Re: [PATCH v12 3/3] printk: make printk.synchronous param rw Sergey Senozhatsky <sergey.senozhatsky.work@gmail.com> - 2016-04-23 10:30 +0200
  [PATCH] printk: make printk.synchronous param rw Sergey Senozhatsky <sergey.senozhatsky@gmail.com> - 2016-04-23 16:10 +0200
    Re: [PATCH] printk: make printk.synchronous param rw Petr Mladek <pmladek@suse.com> - 2016-04-26 16:20 +0200
      Re: [PATCH] printk: make printk.synchronous param rw Sergey Senozhatsky <sergey.senozhatsky.work@gmail.com> - 2016-04-27 03:10 +0200

csiph-web