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

[Sergey Senozhatsky]

Hello,

On (08/17/17 17:43), Rafael J. Wysocki wrote:
[..]
> > > 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.

oh, good.

> > 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.

ok, thanks.

> > > 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.

I see the point: notifiers, in some sense, help us to alter sub-systems
without ever touching them or even explaining anything to the maintainers.
we just register a notifier and all of a sudden sub-system FOO begins to
execute our code at some point. all done entirely with in the printk.c file.
there is some power/flexibility and, perhaps, beauty in it, but there is
also some potential for abuse/wrongdoing in it. if direct calls work better
for you (and PM), then no objections from my side :)

> 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 ...

ok. I can rework the PM patch and get rid of notifiers for the next
submission (unless there will be objections from others). will take
a look.

thanks!

-ss

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

[Sergey Senozhatsky]

Hello,

On (08/17/17 17:43), Rafael J. Wysocki wrote:
[..]
> > > 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.

oh, good.

> > 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.

ok, thanks.

> > > 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.

I see the point: notifiers, in some sense, help us to alter sub-systems
without ever touching them or even explaining anything to the maintainers.
we just register a notifier and all of a sudden sub-system FOO begins to
execute our code at some point. all done entirely with in the printk.c file.
there is some power/flexibility and, perhaps, beauty in it, but there is
also some potential for abuse/wrongdoing in it. if direct calls work better
for you (and PM), then no objections from my side :)

> 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 ...

ok. I can rework the PM patch and get rid of notifiers for the next
submission (unless there will be objections from others). will take
a look.

thanks!

-ss

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

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

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

[Sergey Senozhatsky]

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?

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...

[..]
> > 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.

-ss

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

[Sergey Senozhatsky]

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?

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...

[..]
> > 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.

-ss

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

[Rafael J. Wysocki]

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

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

[Rafael J. Wysocki]

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

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

[Sergey Senozhatsky]

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

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

[Sergey Senozhatsky]

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

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

[Rafael J. Wysocki]

On Tuesday, August 15, 2017 4:56:18 AM CEST Sergey Senozhatsky wrote:
> It's not always possible/safe to wake_up() printk kernel
> thread. For example, late suspend/early resume may printk()
> while timekeeping is not initialized yet, so calling into the
> scheduler may result in recursive warnings.
> 
> Another thing to notice is the fact that PM at some point
> freezes user space and kernel threads: freeze_processes()
> and freeze_kernel_threads(), correspondingly. Thus we need
> printk() to operate in emergency mode there and attempt to
> immediately flush pending kernel message to the console.
> 
> 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?

> Signed-off-by: Sergey Senozhatsky 
> Suggested-by: Andreas Mohr 
> ---
>  kernel/printk/printk.c | 35 +++
>  1 file changed, 35 insertions(+)
> 
> diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
> index 05165f008bc8..d3f149fad85c 100644
> --- a/kernel/printk/printk.c
> +++ b/kernel/printk/printk.c
> @@ -49,6 +49,7 @@
>  #include 
>  #include 
>  #include 
> +#include 
>  
>  #include 
>  #include 
> @@ -2913,6 +2914,33 @@ static DEFINE_PER_CPU(struct irq_work, 
> wake_up_klogd_work) = {
>   .flags = IRQ_WORK_LAZY,
>  };
>  
> +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.

Thanks,
Rafael

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

[Rafael J. Wysocki]

On Tuesday, August 15, 2017 4:56:18 AM CEST Sergey Senozhatsky wrote:
> It's not always possible/safe to wake_up() printk kernel
> thread. For example, late suspend/early resume may printk()
> while timekeeping is not initialized yet, so calling into the
> scheduler may result in recursive warnings.
> 
> Another thing to notice is the fact that PM at some point
> freezes user space and kernel threads: freeze_processes()
> and freeze_kernel_threads(), correspondingly. Thus we need
> printk() to operate in emergency mode there and attempt to
> immediately flush pending kernel message to the console.
> 
> 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?

> Signed-off-by: Sergey Senozhatsky 
> Suggested-by: Andreas Mohr 
> ---
>  kernel/printk/printk.c | 35 +++
>  1 file changed, 35 insertions(+)
> 
> diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
> index 05165f008bc8..d3f149fad85c 100644
> --- a/kernel/printk/printk.c
> +++ b/kernel/printk/printk.c
> @@ -49,6 +49,7 @@
>  #include 
>  #include 
>  #include 
> +#include 
>  
>  #include 
>  #include 
> @@ -2913,6 +2914,33 @@ static DEFINE_PER_CPU(struct irq_work, 
> wake_up_klogd_work) = {
>   .flags = IRQ_WORK_LAZY,
>  };
>  
> +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.

Thanks,
Rafael

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

[Sergey Senozhatsky]

It's not always possible/safe to wake_up() printk kernel
thread. For example, late suspend/early resume may printk()
while timekeeping is not initialized yet, so calling into the
scheduler may result in recursive warnings.

Another thing to notice is the fact that PM at some point
freezes user space and kernel threads: freeze_processes()
and freeze_kernel_threads(), correspondingly. Thus we need
printk() to operate in emergency mode there and attempt to
immediately flush pending kernel message to the console.

This patch registers PM notifier, so PM can switch printk
to emergency mode from PM_FOO_PREPARE notifiers and return
back to printk threaded mode from PM_POST_FOO notifiers.

Signed-off-by: Sergey Senozhatsky 
Suggested-by: Andreas Mohr 
---
 kernel/printk/printk.c | 35 +++
 1 file changed, 35 insertions(+)

diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
index 05165f008bc8..d3f149fad85c 100644
--- a/kernel/printk/printk.c
+++ b/kernel/printk/printk.c
@@ -49,6 +49,7 @@
 #include 
 #include 
 #include 
+#include 
 
 #include 
 #include 
@@ -2913,6 +2914,33 @@ static DEFINE_PER_CPU(struct irq_work, 
wake_up_klogd_work) = {
.flags = IRQ_WORK_LAZY,
 };
 
+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();
+   break;
+
+   case PM_POST_SUSPEND:
+   case PM_POST_HIBERNATION:
+   case PM_POST_RESTORE:
+   printk_emergency_end_sync();
+   break;
+
+   default:
+   return NOTIFY_DONE;
+   }
+
+   return NOTIFY_OK;
+}
+
+static struct notifier_block printk_pm_nb = {
+   .notifier_call = printk_pm_notify,
+};
+
 static int printk_kthread_func(void *data)
 {
while (1) {
@@ -2960,9 +2988,16 @@ static int __init init_printk_kthread(void)
if (!alloc_cpumask_var(_offload_cpus, GFP_KERNEL))
return -ENOMEM;
 
+   if (register_pm_notifier(_pm_nb) != 0) {
+   pr_err("printk: unable to register PM notifier\n");
+   free_cpumask_var(printk_offload_cpus);
+   return -EINVAL;
+   }
+
thread = kthread_run(printk_kthread_func, NULL, "printk");
if (IS_ERR(thread)) {
pr_err("printk: unable to create printing thread\n");
+   unregister_pm_notifier(_pm_nb);
free_cpumask_var(printk_offload_cpus);
return PTR_ERR(thread);
}
-- 
2.14.1

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

[Sergey Senozhatsky]

It's not always possible/safe to wake_up() printk kernel
thread. For example, late suspend/early resume may printk()
while timekeeping is not initialized yet, so calling into the
scheduler may result in recursive warnings.

Another thing to notice is the fact that PM at some point
freezes user space and kernel threads: freeze_processes()
and freeze_kernel_threads(), correspondingly. Thus we need
printk() to operate in emergency mode there and attempt to
immediately flush pending kernel message to the console.

This patch registers PM notifier, so PM can switch printk
to emergency mode from PM_FOO_PREPARE notifiers and return
back to printk threaded mode from PM_POST_FOO notifiers.

Signed-off-by: Sergey Senozhatsky 
Suggested-by: Andreas Mohr 
---
 kernel/printk/printk.c | 35 +++
 1 file changed, 35 insertions(+)

diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
index 05165f008bc8..d3f149fad85c 100644
--- a/kernel/printk/printk.c
+++ b/kernel/printk/printk.c
@@ -49,6 +49,7 @@
 #include 
 #include 
 #include 
+#include 
 
 #include 
 #include 
@@ -2913,6 +2914,33 @@ static DEFINE_PER_CPU(struct irq_work, 
wake_up_klogd_work) = {
.flags = IRQ_WORK_LAZY,
 };
 
+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();
+   break;
+
+   case PM_POST_SUSPEND:
+   case PM_POST_HIBERNATION:
+   case PM_POST_RESTORE:
+   printk_emergency_end_sync();
+   break;
+
+   default:
+   return NOTIFY_DONE;
+   }
+
+   return NOTIFY_OK;
+}
+
+static struct notifier_block printk_pm_nb = {
+   .notifier_call = printk_pm_notify,
+};
+
 static int printk_kthread_func(void *data)
 {
while (1) {
@@ -2960,9 +2988,16 @@ static int __init init_printk_kthread(void)
if (!alloc_cpumask_var(_offload_cpus, GFP_KERNEL))
return -ENOMEM;
 
+   if (register_pm_notifier(_pm_nb) != 0) {
+   pr_err("printk: unable to register PM notifier\n");
+   free_cpumask_var(printk_offload_cpus);
+   return -EINVAL;
+   }
+
thread = kthread_run(printk_kthread_func, NULL, "printk");
if (IS_ERR(thread)) {
pr_err("printk: unable to create printing thread\n");
+   unregister_pm_notifier(_pm_nb);
free_cpumask_var(printk_offload_cpus);
return PTR_ERR(thread);
}
-- 
2.14.1