Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1606735 > unrolled thread

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

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [RFC][PATCH 1/4] printk: introduce printing kernel thread Petr Mladek <pmladek@suse.com> - 2017-03-22 17:50 +0100
    Re: [RFC][PATCH 1/4] printk: introduce printing kernel thread Sergey Senozhatsky <sergey.senozhatsky.work@gmail.com> - 2017-03-23 09:10 +0100
      Re: [RFC][PATCH 1/4] printk: introduce printing kernel thread Petr Mladek <pmladek@suse.com> - 2017-03-23 11:50 +0100
        Re: [RFC][PATCH 1/4] printk: introduce printing kernel thread Sergey Senozhatsky <sergey.senozhatsky.work@gmail.com> - 2017-03-24 06:30 +0100

#1606735 — 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

[toc] | [next] | [standalone]

#1607230

On (03/22/17 17:40), Petr Mladek wrote:
[..]
> > +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 ;-)

well, I guess I can rename it.

one observation here is that those functions neither turn off nor
disable printk_kthread. we mark the point after which we will not
wake_up printk_kthread, but that does not mean that printk_kthread
is already or soon will be inactive. it actually can be in running
state. so the name

console_do_not_offload_printing_to_printk_kthread_unless_its_already_running()

will describe it better.  :)

---

overall the whole need of off()/on() sections is slightly worrisome.
there are probably not so many cases when we need to forcibly avoid
wake_up(printk_kthread) calls, but we don't have any checks/mechanisms
to verify it (like, for example, might_sleep()). so it's case by case.


> 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();

hm, I certainly see what you meant here, but I suspect this naming may
be a bit misleading - "so printk_deferred_mode_off() disables printk_deferred()?"


> 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.

yes, this is why I avoided mentioning "printk_kthread" (directly)
in API naming. console_printing_thread is sort of neutral (well,
sort of). not insisting that the naming is perfect, of course.


> 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.

hm, sorry. not sure I see how we can do this.
LOGLEVEL_SCHED hack is... hacky. yes.

when we examine `printk_kthread_disable' we decide wether we must
	a) wake_up() printk_kthread
or
	b) do console_trylock()

the LOGLEVEL_SCHED thing is completely different tho. it tells us that
neither of the above is safe -- both wake_up() and console_trylock()
can potentially call into the scheduler. so I'm not sure we can easily
replace LOGLEVEL_SCHED with `printk_kthread_disable'.

	-ss

[toc] | [prev] | [next] | [standalone]

#1607342

On Thu 2017-03-23 14:12:42, Sergey Senozhatsky wrote:
> On (03/22/17 17:40), Petr Mladek wrote:
> [..]
> > > +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--;
> > > +}
> > 
> 
> one observation here is that those functions neither turn off nor
> disable printk_kthread. we mark the point after which we will not
> wake_up printk_kthread, but that does not mean that printk_kthread
> is already or soon will be inactive. it actually can be in running
> state. so the name

Yup, allow/deny might might be better than on/off.

> hm, I certainly see what you meant here, but I suspect this naming may
> be a bit misleading - "so printk_deferred_mode_off() disables printk_deferred()?"

True, sigh.

> > 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.
> 
> yes, this is why I avoided mentioning "printk_kthread" (directly)
> in API naming. console_printing_thread is sort of neutral (well,
> sort of). not insisting that the naming is perfect, of course.

It is too close to "printk_thread" used on different place.
So, it confuses me :-)


> the LOGLEVEL_SCHED thing is completely different tho. it tells us that
> neither of the above is safe -- both wake_up() and console_trylock()
> can potentially call into the scheduler. so I'm not sure we can easily
> replace LOGLEVEL_SCHED with `printk_kthread_disable'.

Grr, you are right. LOGLEVEL_SHED is deferred another way.

Let me do one more attempt for a generic name. What about?

    printk_console_press();    or try_harder() or push()
    printk_console_relax();

or something like this?

Best Regards,
Petr

[toc] | [prev] | [next] | [standalone]

#1608131

On (03/23/17 11:40), Petr Mladek wrote:
[..]
> Let me do one more attempt for a generic name. What about?
> 
>     printk_console_press();    or try_harder() or push()
>     printk_console_relax();
> 
> or something like this?

I replied in another thread.
how about printk_rescue?

	-ss

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web