Messages in this thread | | | Date | Fri, 22 Apr 2016 10:28:26 +0900 | From | Sergey Senozhatsky <> | Subject | Re: [PATCH v11 3/3] printk: make printk.synchronous param rw |
| |
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
| |