Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1653059 > unrolled thread

Re: [RFC][PATCHv3 5/5] printk: register PM notifier

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [RFC][PATCHv3 5/5] printk: register PM notifier Petr Mladek <pmladek@suse.com> - 2017-05-30 12:00 +0200
    Re: [RFC][PATCHv3 5/5] printk: register PM notifier Sergey Senozhatsky <sergey.senozhatsky.work@gmail.com> - 2017-05-31 08:50 +0200

#1653059 — Re: [RFC][PATCHv3 5/5] printk: register PM notifier

On Tue 2017-05-09 17:28:59, 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 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 <sergey.senozhatsky@gmail.com>
> Suggested-by: Andreas Mohr <andi@lisas.de>
> ---
>  kernel/printk/printk.c | 27 +++++++++++++++++++++++++++
>  1 file changed, 27 insertions(+)
> 
> diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
> index 81ea575728b9..6aae36a29aca 100644
> --- a/kernel/printk/printk.c
> +++ b/kernel/printk/printk.c
> @@ -2928,6 +2929,30 @@ 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();
> +		break;
> +
> +	case PM_POST_SUSPEND:
> +	case PM_POST_HIBERNATION:
> +	case PM_POST_RESTORE:
> +		printk_emergency_end();
> +		break;
> +	}
> +
> +	return 0;

Heh, it seems that the meaning of return values
is a bit unclear and messy. For example see an older problem
with android memory handlers at https://lkml.org/lkml/2012/4/9/177

My understanding is that notifiers should return NOTIFY_OK
when they handled it and NOTIFY_DONE when they did nothing.
For example, see cpu_hotplug_pm_callback().

In reality, there does not seem to be a big difference,
see notifier_to_errno() in __pm_notifier_call_chain().

Anyway, I would try to follow the cpu_hotplug_pm_callback()
example.


> +}
> +
> +static struct notifier_block printk_pm_nb = {
> +	.notifier_call = printk_pm_notify,
> +};
> +
>  static int printk_kthread_func(void *data)
>  {
>  	while (1) {
> @@ -2961,6 +2986,8 @@ static int __init init_printk_kthread(void)
>  
>  	sched_setscheduler(thread, SCHED_FIFO, &param);
>  	printk_kthread = thread;
> +
> +	WARN_ON(register_pm_notifier(&printk_pm_nb) != 0);

I think that a simple error message might be enough.

Also we might want to force the emergency mode in case of error.
Otherwise, the messages will not appear during suspend and such
operations.


BTW: Do you know about some locations that would need to be
patched explicitly, for example, kexec?

Some old versions of this patch touched console_suspend().
This brought me to snapshot_ioctl(). It looks like an
API that allows to create snapshots from user space.
I wonder if we should switch to the emergency mode
there are well, probably in the SNAPSHOT_FREEZE
stage.

Best Regards,
Petr

[toc] | [next] | [standalone]

#1653879

Hello Petr,

thanks for taking a look.

On (05/30/17 11:55), Petr Mladek wrote:
[..]
> My understanding is that notifiers should return NOTIFY_OK
> when they handled it and NOTIFY_DONE when they did nothing.
> For example, see cpu_hotplug_pm_callback().
> 
> In reality, there does not seem to be a big difference,
> see notifier_to_errno() in __pm_notifier_call_chain().
> 
> Anyway, I would try to follow the cpu_hotplug_pm_callback()
> example.

good points, will fix.

> > +	WARN_ON(register_pm_notifier(&printk_pm_nb) != 0);
> 
> I think that a simple error message might be enough.
> 
> Also we might want to force the emergency mode in case of error.
> Otherwise, the messages will not appear during suspend and such
> operations.

good point, will fix.

> BTW: Do you know about some locations that would need to be
> patched explicitly, for example, kexec?

a tricky question and I need to think more. I'm a bit overloaded
with some other stuff now, sorry for long replies.

in case of kexec_image->preserve_context my assumption was that
PM would send out PM notifications. but it seems that I need to have
a second look. quite possible we will have to register yet another
PM notifier - add syscore_ops to syscore_ops_list via
register_syscore_ops() - the one that would be called from
syscore_suspend().

the other branch invokes kernel_restart_prepare(), which updates
`system_statem' variable and we test it in console_offload_printing().

some other cases also update `system_statem'. the exception is maybe
sysrq emergency restart. but I'm a bit uncertain here. on one hand the
expected emergency restart behavior is "just reboot the system, OK?",
on the other hand there are people who expect kernel log messages to
be flushed before that "just reboot the system" thing.

> Some old versions of this patch touched console_suspend().
> This brought me to snapshot_ioctl(). It looks like an
> API that allows to create snapshots from user space.
> I wonder if we should switch to the emergency mode
> there are well, probably in the SNAPSHOT_FREEZE
> stage.

my assumption was that PM ioctl sends out PM notifications.
see snapshot_open()/snapshot_release().

	-ss

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web