On Wednesday, August 16, 2017 9:31:17 AM CEST Sergey Senozhatsky wrote:
> 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.
Well, that depends on what your goal is really.
I thought you wanted to do the offloading as far into the suspend as it
was safe to do (and analogously for resume), but now I see you want to
stop doing it as early as it makes sense. :-)
In that case I would call printk_emergency_begin_sync() from
dpm_prepare() and printk_emergency_end_sync() from dpm_complete().
>
> [..]
> > > +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.
But this adds one invocation of each of them anyway *plus* some
extra code around those. Wouldn't it be cleaner to add those
invocations alone?
>
> 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?
I just don't see much point in using the notifier thing if you can
achieve basically the same without using it. :-)
Thanks,
Rafael
