linux-kernel - Re: [RFC][PATCH v2 1/2] printk: Make printk() completely async

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Thu, 10 Mar 2016 10:27:49 +0100
From:	Jan Kara <jack@...e.cz>
To:	Sergey Senozhatsky <sergey.senozhatsky.work@...il.com>
Cc:	Jan Kara <jack@...e.cz>,
	Tetsuo Handa <penguin-kernel@...ove.SAKURA.ne.jp>,
	akpm@...ux-foundation.org, jack@...e.com, pmladek@...e.com,
	tj@...nel.org, linux-kernel@...r.kernel.org,
	sergey.senozhatsky@...il.com
Subject: Re: [RFC][PATCH v2 1/2] printk: Make printk() completely async

On Wed 09-03-16 15:09:50, Sergey Senozhatsky wrote:
> Hello Jan,
> 
> On (03/07/16 13:16), Jan Kara wrote:
> [..]
> > > So if this will be a problem in practice, using a kthread will probably be
> > > the easiest solution.
> > 
> > Hum, and thinking more about it: Considering that WQ_MEM_RECLAIM workqueues
> > create kthread anyway as a rescuer thread, it may be the simplest to just
> > go back to using a single kthread for printing. What do you think?
> 
> I have this patch on top of the series now. In short, it closes one more
> possibility of lockups -- console_lock()/console_unlock() calls. the patch
> splits console_unlock() in two parts:
> -- the fast one just wake up printing kthread
> -- the slow one does call_console_drivers() loop
> 
> I think it sort of makes sense to tweak the patch below a bit and fold it
> into 0001, and move _some_ of the vprintk_emit() checks to console_unlock().
> 
> very schematically, after folding, vprintk_emit() will be
> 
> 	if (in_sched) {
> 		if (!printk_sync && printk_thread)
> 			wake_up()
> 		else
> 			irq_work_queue()
> 	}
> 	
> 	if (!in_sched)
> 		if (console_trylock())
> 			console_unlock()
> 
> and console_unlock() will be
> 
> 	if (!in_panic && !printk_sync && printk_thread) {
> 		up_console_sem()
> 		wake_up()
> 	} else {
> 		console_unlock_for_printk()
> 	}
> 
> console_unlock_for_printk() does the call_console_drivers() loop.
> 
> console_flush_on_panic() and printing_func() call console_unlock_for_printk()
> directly.
> 
> 
> What do you think? Or would you prefer to first introduce async
> printk() rework, and move to console_unlock() in vprintk_emit()
> one release cycle later?
> IOW, in 3 steps:
> -- first make printk() async
> -- then console_unlock() async, and use console_unlock_for_printk() in
>    vprintk_emit()
> -- then switch to console_unlock() in vprintk_emit().
> 
> 
> below is the patch which introduces console_unlock_for_printk().
> not the squashed console_unlock_for_printk() and 0001.

So I think this should definitely stay as a separate patch since it
possibly changes user visible behavior and sometimes blocking may be
actually desirable for userspace. I don't have that strong opinion whether
it should be in a separate patch set or part of this one. Maybe a separate
patch set would be somewhat better so that we first hash out possible issues
with async printk and once that's settled we start messing more with the
code changing the behavior of console_unlock() as well.

								Honza
-- 
Jan Kara <jack@...e.com>
SUSE Labs, CR