linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Petr Mladek <pmladek@suse.com>
To: Sergey Senozhatsky <sergey.senozhatsky.work@gmail.com>
Cc: Linus Torvalds <torvalds@linux-foundation.org>,
	Andrew Morton <akpm@linux-foundation.org>,
	Jan Kara <jack@suse.cz>, Tejun Heo <tj@kernel.org>,
	Calvin Owens <calvinowens@fb.com>,
	Steven Rostedt <rostedt@goodmis.org>,
	Ingo Molnar <mingo@redhat.com>,
	Peter Zijlstra <peterz@infradead.org>,
	Andy Lutomirski <luto@kernel.org>,
	Peter Hurley <peter@hurleysoftware.com>,
	linux-kernel@vger.kernel.org,
	Sergey Senozhatsky <sergey.senozhatsky@gmail.com>
Subject: Re: [PATCHv6 6/7] printk: use printk_safe buffers in printk
Date: Thu, 22 Dec 2016 18:10:12 +0100	[thread overview]
Message-ID: <20161222171012.GC14894@pathway.suse.cz> (raw)
In-Reply-To: <20161222053119.GE644@jagdpanzerIV.localdomain>

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.

  reply	other threads:[~2016-12-22 17:10 UTC|newest]

Thread overview: 19+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2016-12-21 14:35 [PATCHv6 0/7] printk: use printk_safe to handle printk() recursive calls Sergey Senozhatsky
2016-12-21 14:35 ` [PATCHv6 1/7] printk: use vprintk_func in vprintk() Sergey Senozhatsky
2016-12-21 14:36 ` [PATCHv6 2/7] printk: rename nmi.c and exported api Sergey Senozhatsky
2016-12-21 19:45   ` Linus Torvalds
2016-12-22  1:17     ` Sergey Senozhatsky
2016-12-21 14:36 ` [PATCHv6 3/7] printk: introduce per-cpu safe_print seq buffer Sergey Senozhatsky
2016-12-22  0:53   ` kbuild test robot
2016-12-22  1:18     ` Sergey Senozhatsky
2016-12-22 16:36       ` Petr Mladek
2016-12-21 14:36 ` [PATCHv6 4/7] printk: always use deferred printk when flush printk_safe lines Sergey Senozhatsky
2016-12-21 14:36 ` [PATCHv6 5/7] printk: report lost messages in printk safe/nmi contexts Sergey Senozhatsky
2016-12-23 10:54   ` Petr Mladek
2016-12-23 15:08     ` Sergey Senozhatsky
2016-12-21 14:36 ` [PATCHv6 6/7] printk: use printk_safe buffers in printk Sergey Senozhatsky
2016-12-22  5:31   ` Sergey Senozhatsky
2016-12-22 17:10     ` Petr Mladek [this message]
2016-12-23  1:46       ` Sergey Senozhatsky
2016-12-23  9:53         ` Petr Mladek
2016-12-21 14:36 ` [PATCHv6 7/7] printk: remove zap_locks() function Sergey Senozhatsky

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20161222171012.GC14894@pathway.suse.cz \
    --to=pmladek@suse.com \
    --cc=akpm@linux-foundation.org \
    --cc=calvinowens@fb.com \
    --cc=jack@suse.cz \
    --cc=linux-kernel@vger.kernel.org \
    --cc=luto@kernel.org \
    --cc=mingo@redhat.com \
    --cc=peter@hurleysoftware.com \
    --cc=peterz@infradead.org \
    --cc=rostedt@goodmis.org \
    --cc=sergey.senozhatsky.work@gmail.com \
    --cc=sergey.senozhatsky@gmail.com \
    --cc=tj@kernel.org \
    --cc=torvalds@linux-foundation.org \
    --subject='Re: [PATCHv6 6/7] printk: use printk_safe buffers in printk' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).