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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Fri, 18 Aug 2017 08:19:50 +0900
From:   Sergey Senozhatsky <sergey.senozhatsky.work@...il.com>
To:     "Rafael J. Wysocki" <rjw@...ysocki.net>
Cc:     Sergey Senozhatsky <sergey.senozhatsky.work@...il.com>,
        Sergey Senozhatsky <sergey.senozhatsky@...il.com>,
        Petr Mladek <pmladek@...e.com>,
        Steven Rostedt <rostedt@...dmis.org>, Jan Kara <jack@...e.cz>,
        Andrew Morton <akpm@...ux-foundation.org>,
        Peter Zijlstra <peterz@...radead.org>,
        Eric Biederman <ebiederm@...ssion.com>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Jiri Slaby <jslaby@...e.com>, Pavel Machek <pavel@....cz>,
        Andreas Mohr <andi@...as.de>,
        Tetsuo Handa <penguin-kernel@...ove.sakura.ne.jp>,
        linux-kernel@...r.kernel.org
Subject: Re: [RFC][PATCHv5 06/13] printk: register PM notifier

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