Re: [patch 5/8] Immediate Values - x86 Optimization

[Rusty Russell <rusty@rustcorp.com.au>]

On Thursday 15 November 2007 16:37:51 Mathieu Desnoyers wrote:
> * Rusty Russell (rusty@rustcorp.com.au) wrote:
> > On Thursday 15 November 2007 15:06:10 Mathieu Desnoyers wrote:
> > > A - the NMI or MCE code calls any external kernel code (printk,
> > >   notify_die, spin_lock/unlock, die_nmi, lapic_wd_event (perfctr code,
> > >   calls printk too for debugging)...
> >
> > Sure, but as I pointed out previously, such calls are already best
> > effort. You can do very little safely from do_nmi(), and calling printk
> > isn't one of them,
>
> yes and no.. do_nmi uses the "bust spinlocks" exactly for this. So this
> is ok by design.

Yeah, in the "machine is dying" case, we do make best effort to get the 
message out.  Worrying about a GPF on the off chance we happen to be 
modifying an immediate in that case seems crazy.

> > nor is grabbing a spinlock (well, actually you could as long as it's
> > *only* used by NMI handlers.  See any of those?).
>
> Yup, see arch/x86/kernel/nmi_64.c : nmi_watchdog_tick()
>
> It defines a spinlock to "Serialise the printks". I guess it's good to
> protect against other nmi watchdogs running on other CPUs concurrently,
> I guess.

So that when it's printing a backtrace from an NMI.

> > > Therefore, if one decides to use the immediate values to
> > > leave dormant spinlock instrumentation in the kernel, I wouldn't want
> > > it to have undesirable side-effects (GPF) when the instrumentation is
> > > being enabled, as rare as it could be.
> >
> > It's overengineered, since it's less likely than deadlock already.
>
> So should we put a warning telling "enabling tracing or profiling on a
> production system that also uses NMI watchdog could potentially cause a
> crash"

Or, "if your system is crashing, this may make it worse".

> arch/x86/kernel/immediate.o : 2.4K
>
> let's compare..
>
> kernel/stop_machine.o : 3.9K
>
> so I think that code size is not an issue there, especially since the
> immediate values are not meant to be deployed on embedded systems.

stop_machine is necessary (although it could use simplification, patches 
pending).

> Yup, looping in IPIs with interrupts disabled should do the job there.
> It's just awful for interrupt latency on large SMP systems :(

Yet you could have implemented it in the time it took us to have this 
conversation.

Just because code is clever doesn't mean it should go in.  There are enough 
things in the kernel which have to be complex that we should always be on the 
lookout for things which can be made simpler.

Cheers,
Rusty.