Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752618AbcDULH0 (ORCPT ); Thu, 21 Apr 2016 07:07:26 -0400 Received: from mx2.suse.de ([195.135.220.15]:39187 "EHLO mx2.suse.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752364AbcDULHV (ORCPT ); Thu, 21 Apr 2016 07:07:21 -0400 Date: Thu, 21 Apr 2016 13:07:15 +0200 From: Petr Mladek To: Sergey Senozhatsky Cc: Pan Xinhui , Sergey Senozhatsky , Andrew Morton , Jan Kara , Tejun Heo , Tetsuo Handa , linux-kernel@vger.kernel.org, Byungchul Park Subject: Re: [PATCH v11 3/3] printk: make printk.synchronous param rw Message-ID: <20160421110715.GA2749@pathway.suse.cz> References: <1460050307-3718-1-git-send-email-sergey.senozhatsky@gmail.com> <1460050307-3718-4-git-send-email-sergey.senozhatsky@gmail.com> <57072DE7.50806@linux.vnet.ibm.com> <20160408052927.GA614@swordfish> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20160408052927.GA614@swordfish> User-Agent: Mutt/1.5.21 (2010-09-15) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 4340 Lines: 113 On Fri 2016-04-08 14:29:27, Sergey Senozhatsky wrote: > On (04/08/16 12:04), Pan Xinhui wrote: > [..] > > > +/* > > > + * Init async printk via late_initcall, after core/arch/device/etc. > > > + * initialization. > > > + */ > > > +static __init int init_printk_kthread(void) > > > +{ > > > + printk_initcall_done = true; > > > + return __init_printk_kthread(); > > hello, > > > > One confusion, Why not use a lock to protect __init_printk_kthread from parallel call? Otherwise I think there is a race. > > But for simplicity, maybe you could write codes as below. > > > > + int ret = __init_printk_kthread(); > > + printk_initcall_done = true; > > + return ret; > > > > In my opinion, using a lock is better. > > Hello, > > I though about this, but isn't late_initcall() happening before kernel > starts /sbin/init? who can race with > > late_initcall() -> init_printk_kthread() -> __init_printk_kthread()? > > looking at > > static int __ref kernel_init(void *unused) > { > int ret; > > kernel_init_freeable(); > /* need to finish all async __init code before freeing the memory */ > async_synchronize_full(); > free_initmem(); > .. > > if (!try_to_run_init_process("/sbin/init") || > !try_to_run_init_process("/etc/init") || > !try_to_run_init_process("/bin/init") || > !try_to_run_init_process("/bin/sh")) > return 0; > > __init (and init_printk_kthread is __init) is finished and freed by the > time kernel try_to_run_init_process. isn't it? Please, what is the purpose of "printk_initcall_done" then? If I get this correctly, it will always be true when printk_sync_set() is called. > sysfs knob -> __init_printk_kthread() is protected by printk_sync_lock > mutex, obviously there can be parallel calls from user space. I think that this could not happen. We have discussed similar problem with livepatching some time ago. AFAIK, writes to sysfs are synchronized out of box. Huh, the code is not trivial, so I rather generated a backtrace to be on the safe side: [ 370.628592] WARNING: CPU: 2 PID: 3768 at kernel/printk/printk.c:2827 printk_sync_set+0x42/0x70 [ 370.628593] Modules linked in: [ 370.628597] CPU: 2 PID: 3768 Comm: bash Not tainted 4.6.0-rc4-4-default+ #2691 [ 370.628599] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS Bochs 01/01/2011 [ 370.628600] 0000000000000000 ffff88003709fd28 ffffffff814206fc 0000000000000000 [ 370.628604] 0000000000000000 ffff88003709fd68 ffffffff8106b4e1 00000b0b00000246 [ 370.628606] 0000000000000000 ffffffff81dbce18 ffff88013923a3e0 ffff88013a705c80 [ 370.628609] Call Trace: [ 370.628614] [] dump_stack+0x85/0xc9 [ 370.628617] [] __warn+0xd1/0xf0 [ 370.628619] [] warn_slowpath_null+0x1d/0x20 [ 370.628621] [] printk_sync_set+0x42/0x70 [ 370.628624] [] param_attr_store+0x6a/0xd0 [ 370.628627] [] module_attr_store+0x1d/0x30 [ 370.628631] [] sysfs_kf_write+0x44/0x60 [ 370.628634] [] kernfs_fop_write+0x144/0x190 [ 370.628637] [] __vfs_write+0x28/0xe0 [ 370.628641] [] ? percpu_down_read+0x49/0x80 [ 370.628643] [] ? __sb_start_write+0xd1/0xf0 [ 370.628645] [] ? __sb_start_write+0xd1/0xf0 [ 370.628647] [] vfs_write+0xa2/0x1a0 [ 370.628650] [] ? __fget_light+0x66/0x90 [ 370.628652] [] SyS_write+0x49/0xa0 [ 370.628656] [] entry_SYSCALL_64_fastpath+0x1f/0xbd [ 370.628658] ---[ end trace eb22d849233b4c4c ]--- and the writes are really synchronized. At least here: kernfs_fop_write() mutex_lock(&of->mutex) ops->write() mutex_unlock(&of->mutex); It means that the printk_sync_lock is superfluous. Otherwise the patch looks fine. I am bit concerned because the synchronization between the setting and testing of printk_sync/printk_kthread is a bit weak. But it works because we always either wakeup the kthread or call the console directly. So, we are on the safe side. Best Regards, Petr PS: I am sorry for the late comment. I was not able to do so immediately and I wrongly marked the mail as read :-(