Messages in this thread |  | | From | "Rafael J. Wysocki" <> | Subject | Re: [RFC][PATCHv5 06/13] printk: register PM notifier | Date | Wed, 16 Aug 2017 14:58:15 +0200 |
| |
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
|  |