RE: [PATCH v9 05/12] x86, hotplug, suspend: Online CPU0 for suspendor hibernate

[Yu, Fenghua]

> > > On Tuesday, October 16, 2012 10:19 PM Rafael J. Wysocki wrote:
> On Tuesday 16 of October 2012 22:12:27 Yu, Fenghua wrote:
> > > On 10/16/2012 09:47 PM, Rafael J. Wysocki wrote:
> > > > On Tuesday 16 of October 2012 11:05:18 Srivatsa S. Bhat wrote:
> > > >> On 10/16/2012 02:20 AM, Rafael J. Wysocki wrote:
> > > >>> On Friday 12 of October 2012 09:09:42 Fenghua Yu wrote:
> > > >>>> From: Fenghua Yu <fenghua.yu@xxxxxxxxx>
> > > fix the bug I
> > > pointed out in my other mail.
> Because things like this are often overlooked by people trying to
> optimize
> stuff and the fact that you have to add a comment explaining that
> dependency
> only means that it is not exactly straightforward.

If people try to optimize pm notifier, they really need to know
pm_notifier() API and its priority. If they ignore the priority, they will
screw up kernel no matter how many comments in it.

> 
> Moreover, you should also add a corresponding comment in the other
> notifier
> indicating that its priority should be higher than the priority of the
> new thing and explaining why.

The comment in the patch explains this very clearly. I don't think it's
necessary to add comments in other notifiers.

If adding comments in other notifiers each time when you add a new notifier,
will you add 10 more comments in all other notifiers if you add 10 new notifiers?

I would think when people try to change notifier priority, they should
know what they are going to do and search the notifiers and see the comments.

BTW, the other notifier is in generic code. Adding a paranoid comment in
it doesn't seem to be worth. The comment in this patch code is very clear
already.

> 
> I really think that notifiers are fragile in general and this
> particular
> one is no exception.

If we think pm_notifier API is fragile, we need to fix the API instead of
leaving it there and not allowing people to use it because it's suspected
fragile.

It's simply not realistic to tell people not to use the API each
time in code review while in the meantime the API and its priority are staying
in kernel to allow people to use it.

> 
> > It's not a good idea to move bsp_pm_callback() into
> > cpu_hotplug_pm_callback(). There is no architecture specific hook to
> > call x86 bsp specific hotplug in cpu_hotplug_pm_callback(). Moving
> > bsp_pm_callback() into cpu_hotplug_pm_callback() is hacking while
> > this patch follows well defined API and has better coding.
> 
> I'm not sure it is better coding.  You really want one piece of code
> to always follow another piece of code and the best way to ensure
> that is to execute them both explicitly in sequence.

Besides notifiers, there are other places relying on priority (e.g. initcall).
Again, if we think the priority in notifiers and other APIs are fragile,
why don't we fix the APIs?

The problem of calling bsp_pm_callback() in cpu_hotplug_pm_callback() is that
cpu_hotplug_pm_callback() is defined in generic kernel/cpu.c. This bsp hotplug
patchset is x86 arch specific code so far. Changing kernel/cpu.c to hook 
arch specific code is more like hacking kernel compared to the systematic way
to do so in the patch.

Thanks.

-Fenghua
èº{.nÇ+‰·Ÿ®‰­†+%ŠËlzwm…ébëæìr¸›zX§»®w¥Š{ayºÊÚë,j­¢f£¢·hš‹àz¹®w¥¢¸¢·¦j:+v‰¨ŠwèjØm¶Ÿÿ¾«‘êçzZ+ƒùšŽŠÝj"ú!¶iO•æ¬z·švØ^¶m§ÿðÃnÆàþY&—