Re: [RFC][PATCH 1/4] printk: introduce printing kernel thread

[Petr Mladek <pmladek@suse.com>]

On Mon 2017-03-06 21:45:51, Sergey Senozhatsky wrote:
> This patch introduces a dedicated printing kernel thread - printk_kthread.
> The main purpose of this kthread is to offload printing to a non-atomic
> and always scheduleable context, which eliminates 4) and makes 1)-3) less
> critical. printk() now just appends log messages to the kernel log buffer
> and wake_up()s printk_kthread instead of locking console_sem and calling
> into potentially unsafe console_unlock().
> 
> --- a/kernel/printk/printk.c
> +++ b/kernel/printk/printk.c
> +/*
> + * This disables printing offloading and instead attempts
> + * to do the usual console_trylock()->console_unlock().
> + *
> + * Note, this does not stop the printk_kthread if it's already
> + * printing logbuf messages.
> + */
> +void console_printing_thread_off(void)
> +{
> +	printk_kthread_disable++;
> +	barrier();
> +}
> +
> +/* This re-enables printk_kthread offloading. */
> +void console_printing_thread_on(void)
> +{
> +	barrier();
> +	printk_kthread_disable--;
> +}

I really like that these functions are re-entrant. It will make
our life much easier.

Just a small nitpicking. I would prefer to use the name
console_printk_kthread_off()/on(). I was several times confused
by "printing_thread" when searching the sources. The common
sub-string "printk_kthread" for all the related stuff would
make my life easier ;-)

Just an idea. printk() and printk_deferred() behave the same way
when the kthread is enabled. I wonder if we should make it more
explicit by using names like:

   printk_deferred_mode_disabled++;
   printk_deferred_mode_off();
   printk_deferred_mode_on();

Also it is an already know term and a more generic name. This API
is used globally while the kthread is an implementation detail.
The offloading might be done another way in the future.

Finally, I think about using this variable instead of the ugly
LOGLEVEL_SCHED. The catch is that LOGLEVEL_SCHED forces
the deferred mode while these functions force the opposite.

I just think loudly. I wonder what is better to help
people understand the code in the future.


Otherwise, the patch looks fine to me. Well, there are some
related things discussed in the other patches that might
affect it.

Best Regards,
Petr