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

[Rafael J. Wysocki]

On Thursday, August 17, 2017 7:55:58 AM CEST Sergey Senozhatsky wrote:
> Hello Rafael,
> 
> On (08/16/17 14:58), Rafael J. Wysocki wrote:
> [..]
> > > 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.
> 
> to avoid any PM breakage :)
> 
> > 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. :-)
> 
> ideally yes :) but in reality I'd probably prefer to switch to emergency
> printk ASAP during PM. we have reports of broken PM because of offloading
> from Linaro (well... a long time ago, and printk kthread patch set was
> completely different back then).
> 
> > In that case I would call printk_emergency_begin_sync() from
> > dpm_prepare() and printk_emergency_end_sync() from dpm_complete().
> 
> hm, isn't it the case that dpm_prepare/dpm_complete are invoked only
> by hibernate path? or does suspend path (s2ram, etc.) also calls
> dpm_prepare/dpm_complete?

Yes, it does.

> the 3 things we need to have (in PM context) for offloading:
> - unparked printk kthread
> - running scheduler
> - online non-boot CPUs (on a UP system, or with non-boot CPUs disabled,
>   offloading is a bit questionable)
> 
> - hm, may be something else...

All of that is there during the entire device suspend/resume including
dpm_suspend/resume_noirq().

But probably dpm_prepare/complete() are better places for the hooks at
least for now.

> 
> [..]
> > > 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?
> [..]
> > I just don't see much point in using the notifier thing if you can
> > achieve basically the same without using it. :-)
> 
> sure, I just didn't want to mix printk internals with PM internals.
> that would put us in position of verifying future PM changes from
> printk-kthread point of view as well; and it can be quite complex,
> because printk offloading brings in big guns like scheduler and
> timekeeping. so the notifiers interface looks like a good
> alternative, besides those notifications happen early (and late)
> enough to keep us on the safe side.
> 
> well, I may be wrong.

I prefer direct invocations, becasue they generally allow to figure out
what's going on by simply following the code instead of having to
track all of the users of the notifiers to see what they may have
registered.

Moreover, the ordering of what happens is clear then, whereas with notifiers it
depends on the registration ordering and the entry and exit path orderings of
notifiers are the same which may be problematic sometimes.

In fact, the PM notifiers are mostly for stuff that cannot be done with frozen
user space and surely not for core subsystems.

Let alone that adding two lines of code is better than adding 50 lines for the
same purpose IMO ...

Thanks,
Rafael