From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752227AbdHPNGo (ORCPT ); Wed, 16 Aug 2017 09:06:44 -0400 Received: from cloudserver094114.home.net.pl ([79.96.170.134]:59775 "EHLO cloudserver094114.home.net.pl" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752148AbdHPNGm (ORCPT ); Wed, 16 Aug 2017 09:06:42 -0400 From: "Rafael J. Wysocki" To: Sergey Senozhatsky Cc: Sergey Senozhatsky , Petr Mladek , Steven Rostedt , Jan Kara , Andrew Morton , Peter Zijlstra , Eric Biederman , Greg Kroah-Hartman , Jiri Slaby , Pavel Machek , Andreas Mohr , Tetsuo Handa , linux-kernel@vger.kernel.org Subject: Re: [RFC][PATCHv5 06/13] printk: register PM notifier Date: Wed, 16 Aug 2017 14:58:15 +0200 Message-ID: <1933345.ycXYzBI3bM@aspire.rjw.lan> In-Reply-To: <20170816073116.GB522@jagdpanzerIV.localdomain> References: <20170815025625.1977-1-sergey.senozhatsky@gmail.com> <10043624.VvthE8rrlX@aspire.rjw.lan> <20170816073116.GB522@jagdpanzerIV.localdomain> MIME-Version: 1.0 Content-Transfer-Encoding: 7Bit Content-Type: text/plain; charset="us-ascii" Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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