Re: [RFC][PATCH v8 1/2] printk: Make printk() completely async

Petr Mladek
2016-04-01T11:30:33Z

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