Re: [PATCH -v2 4/7] x86, NMI, Rewrite NMI handler

[Don Zickus]

On Mon, Sep 27, 2010 at 07:40:57PM +0200, Robert Richter wrote:
> On 27.09.10 11:29:16, Don Zickus wrote:
> > On Mon, Sep 27, 2010 at 03:25:38PM +0200, Robert Richter wrote:
> > > On 27.09.10 08:39:24, huang ying wrote:
> > > 
> > > Looking at all you comments below I would vote for the following:
> > > 
> > > We implement all handlers using DIE_NMI and set its priority
> > > accordingly in struct notifier_block when registering the the nmi
> > > handler. We define NMI priorities as macros such as
> > > NMI_PRIORITY_LOCAL, NMI_PRIORITY_WATCHDOG, NMI_PRIORITY_IO, etc. and
> > > require all handlers to set the priority. register_die_notifier() with
> > > (!nb->priority) should return -EINVAL. DIE_NMI_UNKNOWN should only be
> > > used if there is a handler for the case when all others fail such as
> > > implemented in the perf nmi handler or when reporting an unknown nmi.
> > > 
> > > This will avoid all the confusion below and also makes the code much
> > > cleaner.
> > 
> > This could be interesting and certainly simplify things (processing the
> > die_chain once, instead of 4 times I think).  But I would probably
> > recommend we do this as another patch on top of Huang's to layer the
> > changes in a way that we can easily bisect where things went wrong if
> > NMIs start mis-behaving.
> > 
> > I don't think any of the handlers really used the priority, except for
> > perf, which tried to be on the bottom of the list.  So assigning
> > priorities like this may work.
> 
> 
> Yes, we should do this in small steps, but also avoid to introduce new
> intermediate code. So maybe we keep most changes of this patch, but
> also remove DIE_NMI_IPI at all and add priorities to the affected
> handlers? Later we patch the rest.


Actually, looking through the handlers, by introducing priorities means
that people have to register multiple handlers to deal with the different
priorities.

For example, perf would need two handlers, one for DIE_NMI and one for
DIE_NMIUNKOWN.  I am not sure that is the way to go.

I would be inclined to leave the patch as is until we can come up with a
better way to handle the priorities.

Though I agree that DIE_NMI_IPI isn't a great name (doesn't it all go
through the local apic?), it isn't being introduced with this patch.

Thoughts?

Cheers,
Don

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/