lkml.org 
[lkml]   [2016]   [Apr]   [21]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [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

\
 
 \ /
  Last update: 2016-04-22 03:41    [W:0.094 / U:0.168 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site