From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <linux-kernel-owner@vger.kernel.org>
Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand
	id S1754777AbcDALad (ORCPT <rfc822;w@1wt.eu>);
	Fri, 1 Apr 2016 07:30:33 -0400
Received: from mx2.suse.de ([195.135.220.15]:51184 "EHLO mx2.suse.de"
	rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP
	id S1751902AbcDALac (ORCPT <rfc822;linux-kernel@vger.kernel.org>);
	Fri, 1 Apr 2016 07:30:32 -0400
Date: Fri, 1 Apr 2016 13:30:28 +0200
From: Petr Mladek <pmladek@suse.com>
To: Sergey Senozhatsky <sergey.senozhatsky.work@gmail.com>
Cc: Sergey Senozhatsky <sergey.senozhatsky@gmail.com>,
        Andrew Morton <akpm@linux-foundation.org>, Jan Kara <jack@suse.com>,
        Tejun Heo <tj@kernel.org>,
        Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>,
        linux-kernel@vger.kernel.org, Byungchul Park <byungchul.park@lge.com>,
        Jan Kara <jack@suse.cz>
Subject: Re: [RFC][PATCH v8 1/2] printk: Make printk() completely async
Message-ID: <20160401113028.GP5522@pathway.suse.cz>
References: <1458834203-3392-1-git-send-email-sergey.senozhatsky@gmail.com>
 <1458834203-3392-2-git-send-email-sergey.senozhatsky@gmail.com>
 <20160331111229.GB1023@pathway.suse.cz>
 <20160401010803.GA501@swordfish>
 <20160401085912.GO5522@pathway.suse.cz>
 <20160401093602.GB503@swordfish>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20160401093602.GB503@swordfish>
User-Agent: Mutt/1.5.21 (2010-09-15)
Sender: linux-kernel-owner@vger.kernel.org
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

On Fri 2016-04-01 18:36:02, Sergey Senozhatsky wrote:
> Hello Petr,
> 
> On (04/01/16 10:59), Petr Mladek wrote:
> [..]
> > CPU0					CPU1
> > 
> > printk()
> > 
> >   if (printk_kthread)
> >   # fails and need_flush_console
> >   # stays false
> > 
> > 					init_printk_kthread()
> > 					  # put printk_thread into
> > 					  # run queue
> > 					  printk_kthread = ...;
> > 
> >   if (!in_panic && printk_kthread)
> >     wake_up_process(printk_kthread);
> > 
> > 
> > 					# printk kthread finally gets
> > 					# scheduled
> > 					printk_kthread_func()
> > 
> > 					set_current_state(TASK_INTERRUPTIBLE);
> > 					if (!need_flush_console)
> > 					  schedule();
> > 
> > => printk_kthread is happily sleeping without calling console.
> 
> oohh, that tiny race. well, looks quite harmless, it's unlikely that
> we had printk()-s up until late_initcall(init_printk_kthread) and not
> a single one ever after. but good find!
> 
> so the check
> 	if (printk_kthread)
> 		need_flush_console = 1
> 
> can be replaced with
> 	if (!printk_sync)
> 		need_flush_console = 1
> 
> or... may be dropped.

Yup or yup, see below.

> > I do not see any code that will modify need_flush_console when
> > printk.synchronous is modified at runtime.
> 
> printk.synchronous is RO; no runtime modification.
> 
> > I know that all this is rather theoretical. My main point is to remove
> > unnecessary checks that make the code harder to read and does not bring
> > any big advantage.
> 
> my point is that those checks are just .loads, which help to avoid
> spurious .stores from various CPUs.
> 
> CPU1		CPU2		CPU3		...	CPU1024
> 
> lock logbuf
> need_flush=1
> unlock logbuf
> 		lock logbuf
> 		need_flush=1
> 		unlock logbuf
> 				lock logbuf
> 				need_flush=1
> 				unlock logbuf
> wakeup kthread					
> 						...
> 							lock logbuf
> 							need_flush=1
> 							unlock logbuf
> 
> isn't it a bit useless need_flush=1 traffic?

So, it is a small trade off between code readability and data writes
in a slow path. I do not have strong opinion about it. Feel free to
use what you like, just please use the same approach in both
vprintk_emit() and console_unlock().

Best Regards,
Petr