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
