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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20170322164058.GE4008@pathway.suse.cz>
Date:   Wed, 22 Mar 2017 17:40:58 +0100
From:   Petr Mladek <pmladek@...e.com>
To:     Sergey Senozhatsky <sergey.senozhatsky.work@...il.com>
Cc:     Steven Rostedt <rostedt@...dmis.org>,
        Andrew Morton <akpm@...ux-foundation.org>,
        Linus Torvalds <torvalds@...ux-foundation.org>,
        Peter Zijlstra <peterz@...radead.org>,
        "Rafael J . Wysocki" <rjw@...ysocki.net>,
        linux-kernel@...r.kernel.org,
        Sergey Senozhatsky <sergey.senozhatsky@...il.com>,
        Jan Kara <jack@...e.cz>
Subject: Re: [RFC][PATCH 1/4] printk: introduce printing kernel thread

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