On Fri 2016-04-01 18:36:02, Sergey Senozhatsky wrote: > Hello Petr, > > On (04/01/16 10:59), Petr Mladek wrote: > [..] > > CPU0 CPU1 > > > > printk() > > > > if (printk_kthread) > > # fails and need_flush_console > > # stays false > > > > init_printk_kthread() > > # put printk_thread into > > # run queue > > printk_kthread = ...; > > > > if (!in_panic && printk_kthread) > > wake_up_process(printk_kthread); > > > > > > # printk kthread finally gets > > # scheduled > > printk_kthread_func() > > > > set_current_state(TASK_INTERRUPTIBLE); > > if (!need_flush_console) > > schedule(); > > > > => printk_kthread is happily sleeping without calling console. > > oohh, that tiny race. well, looks quite harmless, it's unlikely that > we had printk()-s up until late_initcall(init_printk_kthread) and not > a single one ever after. but good find! > > so the check > if (printk_kthread) > need_flush_console = 1 > > can be replaced with > if (!printk_sync) > need_flush_console = 1 > > or... may be dropped.
Yup or yup, see below. > > I do not see any code that will modify need_flush_console when > > printk.synchronous is modified at runtime. > > printk.synchronous is RO; no runtime modification. > > > I know that all this is rather theoretical. My main point is to remove > > unnecessary checks that make the code harder to read and does not bring > > any big advantage. > > my point is that those checks are just .loads, which help to avoid > spurious .stores from various CPUs. > > CPU1 CPU2 CPU3 ... CPU1024 > > lock logbuf > need_flush=1 > unlock logbuf > lock logbuf > need_flush=1 > unlock logbuf > lock logbuf > need_flush=1 > unlock logbuf > wakeup kthread > ... > lock logbuf > need_flush=1 > unlock logbuf > > isn't it a bit useless need_flush=1 traffic? So, it is a small trade off between code readability and data writes in a slow path. I do not have strong opinion about it. Feel free to use what you like, just please use the same approach in both vprintk_emit() and console_unlock(). Best Regards, Petr
