Re: [PATCH v11 3/3] printk: make printk.synchronous param rw

[Sergey Senozhatsky]

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