Re: [RFC][PATCHv5 06/13] printk: register PM notifier

From: Sergey Senozhatsky <sergey.senozhatsky.work@gmail.com>
To: "Rafael J. Wysocki" <rjw@rjwysocki.net>
Cc: Sergey Senozhatsky <sergey.senozhatsky@gmail.com>,
	Petr Mladek <pmladek@suse.com>,
	Steven Rostedt <rostedt@goodmis.org>, Jan Kara <jack@suse.cz>,
	Andrew Morton <akpm@linux-foundation.org>,
	Peter Zijlstra <peterz@infradead.org>,
	Eric Biederman <ebiederm@xmission.com>,
	Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
	Jiri Slaby <jslaby@suse.com>, Pavel Machek <pavel@ucw.cz>,
	Andreas Mohr <andi@lisas.de>,
	Tetsuo Handa <penguin-kernel@i-love.sakura.ne.jp>,
	linux-kernel@vger.kernel.org,
	Sergey Senozhatsky <sergey.senozhatsky.work@gmail.com>
Subject: Re: [RFC][PATCHv5 06/13] printk: register PM notifier
Date: Wed, 16 Aug 2017 16:31:17 +0900	[thread overview]
Message-ID: <20170816073116.GB522@jagdpanzerIV.localdomain> (raw)
In-Reply-To: <10043624.VvthE8rrlX@aspire.rjw.lan>

On (08/15/17 13:51), Rafael J. Wysocki wrote:
> On Tuesday, August 15, 2017 4:56:18 AM CEST Sergey Senozhatsky wrote:
[..]
> > This patch registers PM notifier, so PM can switch printk
> > to emergency mode from PM_FOO_PREPARE notifiers and return
> 
> Isn't that too early?  That's before user space is frozen even.
> 
> > back to printk threaded mode from PM_POST_FOO notifiers.
> 
> And isn't that too late?

hm, those two are interesting questions. in short - well, it might
be. I don't want to interfere with PM by doing 'accidental' offloading
etc., PM is too complicated already. so I'd prefer to switch to old
printk behavior early (besides, I tend to see lockups reports more
often when the kernel is up and running, rather than during PM events.)
but, once again, may be it is too early and we can move emergency_mode
switch.

[..]
> > +static int printk_pm_notify(struct notifier_block *notify_block,
> > +			    unsigned long mode, void *unused)
> > +{
> > +	switch (mode) {
> > +	case PM_HIBERNATION_PREPARE:
> > +	case PM_SUSPEND_PREPARE:
> > +	case PM_RESTORE_PREPARE:
> > +		printk_emergency_begin_sync();
> 
> I'm not sure what would be wrong with calling this directly
> from dpm_suspend_noirq().
> 
> > +		break;
> > +
> > +	case PM_POST_SUSPEND:
> > +	case PM_POST_HIBERNATION:
> > +	case PM_POST_RESTORE:
> > +		printk_emergency_end_sync();
> 
> And this could be called from dpm_resume_noirq().
> 
> In which case you wouldn't really need the stuff below.

we didn't want to spread printk_emergency_{begin, end}
calls across the kernel.

as of dpm_suspend_noirq/dpm_resume_noirq - I need to look more.
isn't dpm_{suspend, resume}_noirq too late/early? :)

dpm_resume_noirq() happens much earlier than
suspend_finish()->suspend_thaw_processes(), right?
do we want to enable offloading this early?

currently what we have is the following sequence

suspend_finish()
  suspend_thaw_processes()
    pm_notifier_call_chain(PM_POST_SUSPEND)    // enable offloading
      pm_restore_console()

which looks OK to me, frankly.
do you see any problems here?

	-ss