Return-Path: <linux-kernel-owner@vger.kernel.org>
Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand
	id S1753256AbcDVB05 (ORCPT <rfc822;w@1wt.eu>);
	Thu, 21 Apr 2016 21:26:57 -0400
Received: from mail-pa0-f41.google.com ([209.85.220.41]:34986 "EHLO
	mail-pa0-f41.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org
	with ESMTP id S1751672AbcDVB0z (ORCPT
	<rfc822;linux-kernel@vger.kernel.org>);
	Thu, 21 Apr 2016 21:26:55 -0400
Date: Fri, 22 Apr 2016 10:28:26 +0900
From: Sergey Senozhatsky <sergey.senozhatsky.work@gmail.com>
To: Petr Mladek <pmladek@suse.com>
Cc: Sergey Senozhatsky <sergey.senozhatsky.work@gmail.com>,
        Pan Xinhui <xinhui@linux.vnet.ibm.com>,
        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>
Subject: Re: [PATCH v11 3/3] printk: make printk.synchronous param rw
Message-ID: <20160422012826.GA515@swordfish>
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>
 <20160421110715.GA2749@pathway.suse.cz>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20160421110715.GA2749@pathway.suse.cz>
User-Agent: Mutt/1.6.0 (2016-04-01)
Sender: linux-kernel-owner@vger.kernel.org
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org
Content-Length: 3618
Lines: 110

Hello,

On (04/21/16 13:07), Petr Mladek wrote:
[..]
> 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.

well, this is a bit ugly, yes. kernel_param_ops defines ->set callback
as printk_sync_set(). and this ->set callback is getting called from 2
different paths (but it's really about underlying __init_printk_kthread()).

__init_printk_kthread() can be executed from:


1) when command line is getting parsed, and we have printk.synchronous=[0|1]

[    0.000000] Kernel command line: BOOT_IMAGE=/vmlinuz-4.6.0-rc4-next-20160421 ... printk.synchronous=0
[..]
[    0.000000]  [<ffffffff8106857b>] printk_sync_set+0x12/0x52
[    0.000000]  [<ffffffff8104d9f7>] parse_args+0x1ad/0x2bb
[    0.000000]  [<ffffffff8106a3b8>] ? vprintk_default+0x18/0x1a
[    0.000000]  [<ffffffff8188fbda>] start_kernel+0x175/0x378
[    0.000000]  [<ffffffff8188f852>] ? set_init_arg+0x55/0x55
[    0.000000]  [<ffffffff8188f28e>] x86_64_start_reservations+0x2a/0x2c
[    0.000000]  [<ffffffff8188f3c8>] x86_64_start_kernel+0x138/0x148

can't invoke __init_printk_kthread().



2) late_initcall(init_printk_kthread)

can invoke __init_printk_kthread() at this point.



2) when write to /sys/module/printk/parameters/synchronous happens from user space

[   65.441956]  [<ffffffff8106857b>] printk_sync_set+0x12/0x52
[   65.441959]  [<ffffffff8104db6a>] param_attr_store+0x65/0x8b
[   65.441960]  [<ffffffff8104cf7d>] module_attr_store+0x19/0x25
[   65.441963]  [<ffffffff811411da>] sysfs_kf_write+0x36/0x38
[   65.441964]  [<ffffffff81140657>] kernfs_fop_write+0xe8/0x12e
[   65.441966]  [<ffffffff810f2535>] __vfs_write+0x21/0xc3
[   65.441967]  [<ffffffff810f1888>] ? filp_close+0x57/0x61
[   65.441969]  [<ffffffff81064ed9>] ? percpu_down_read+0xe/0x37
[   65.441970]  [<ffffffff810f2751>] vfs_write+0xb9/0x143
[   65.441971]  [<ffffffff810f28a8>] SyS_write+0x49/0x84
[   65.441974]  [<ffffffff8144959b>] entry_SYSCALL_64_fastpath+0x13/0x8f

can invoke __init_printk_kthread().



alternatively, I had this idea to re-define ->set() callback in init_printk_kthread().

IOW, by default we use param_set_bool(), so parse_args() will not cause any
problems:

 static /*** can't 'const' anymore ***/ struct kernel_param_ops param_ops_printk_sync = {
        .set = param_set_bool,
        .get = param_get_bool,
 };

and change it to printk_sync_set() in:

static __init int init_printk_kthread(void)
{
	param_ops_printk_sync.set = printk_sync_set;
	return __init_printk_kthread();
}

but I think that this bool flag is simpler to understand after all.

> > 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.

oh, indeed.

kernfs_fop_write(struct file *file)
{
	..
	mutex_lock(&((struct seq_file *)file->private_data)->mutex);
	ops->write();
	mutex_unlock(&((struct seq_file *)file->private_data)->mutex);
	..
}

good to know! will drop the mutex and re-spin.

[..]
> 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.

thanks.

> PS: I am sorry for the late comment. I was not able to do so
> immediately and I wrongly marked the mail as read :-(

no prob!

	-ss