Re: [PATCHv6 6/7] printk: use printk_safe buffers in printk

[Petr Mladek <pmladek@suse.com>]

On Thu 2016-12-22 14:31:19, Sergey Senozhatsky wrote:
> Hello,
> 
> On (12/21/16 23:36), Sergey Senozhatsky wrote:
> > Use printk_safe per-CPU buffers in printk recursion-prone blocks:
> > -- around logbuf_lock protected sections in vprintk_emit() and
> >    console_unlock()
> > -- around down_trylock_console_sem() and up_console_sem()
> > 
> > Note that this solution addresses deadlocks caused by printk()
> > recursive calls only. That is vprintk_emit() and console_unlock().
> 
> several questions.

Good questions!

> so my plan was to introduce printk-safe and to switch vprintk_emit()
> and console_sem related functions (like console_unlock(), etc.) to
> printk-safe first. and switch the remaining logbuf_lock users, like
> devkmsg_open()/syslog_print()/etc, in a followup, pretty much
> mechanical "find logbuf_lock - add printk_safe", patch. but that
> followup patch is bigger than I expected (still mechanical tho);
> so I want to re-group.
> 
> there are
> 	9 raw_spin_lock_irq(&logbuf_lock)
> 	7 raw_spin_lock_irqsave(&logbuf_lock, flags)
> and
> 	12 raw_spin_lock_irq(&logbuf_lock)
> 	8 raw_spin_unlock_irqrestore(&logbuf_lock, flags)

I see.

> wrapping each one of them in printk_safe_enter()/printk_safe_enter_irq()
> and printk_safe_exit()/printk_safe_exit_irq() is a bit boring. so I have
> several options: one of them is to add printk_safe_{enter,exit}_irq() and,
> along with it, a bunch of help macros (to printk.c):
> 
> (questions below)
> 
> /*
>  * Helper macros to lock/unlock logbuf_lock in deadlock safe
>  * manner (logbuf_lock may spin_dump() in lock/unlock).
>  */
> #define lock_logbuf(flags)			\
> 	do {					\
> 		printk_safe_enter(flags);	\
> 		raw_spin_lock(&logbuf_lock);	\
> 	} while (0)

There are many callers. I think that such wrappers make sense.
I would only like to keep naming scheme similar to the classic
locks. I mean:

printk_safe_enter_irq()
printk_safe_exit_irq()

printk_safe_enter_irqsave(flags)
printk_safe_exit_irqrestore(flags)

and

logbuf_lock_irq()
logbuf_unlock_irq()

logbuf_lock_irqsave(flags)
logbuf_lock_irqrestore(flags)

I actually like this change. It makes it clear that the operation
has a side effect (disables/enables irq) which was not visible
from the original name.


Well, I wonder how many times we need to call printk_save_enter/exit
standalone (ouside these macros).

The question is if we really need all the variants of
printk_safe_enter()/exit(). Alternative solution would be
to handle only the printk_context in pritnk_safe_enter()
and make sure that it is called with IRQs disabled.
I mean to define only __printk_safe_enter()/exit()
and do:

#define logbuf_lock_irqsave(flags)		\
	do {					\
		local_irq_save(flags)		\
		__printk_safe_enter();		\
		raw_spin_lock(&logbuf_lock);	\
	} while (0)


> -- the approach
>  another solution? switch those raw_spin_{lock,unlock}_irq to irqsave/irqrestore
>  (?) and use the existing printk_safe_enter()/printk_safe_exit(),
>  so *_irq() versions of lock_logbuf/printk_safe macros will not be needed?

This is bad idea. We should keep the information where the flags need
to be stored and where not. It helps to understand in which context
the function might be called.

Best Regards,
Petr

PS: I still think if we could come with a better name than
printk_safe() but I cannot find one.