svn commit: r225372 - head/sys/kern

[Attilio Rao]

2011/9/4 Bruce Evans <brde at optusnet.com.au>:
> On Sun, 4 Sep 2011, Attilio Rao wrote:
>
>> Also please notice that intr enable/disable happens in the wrong way
>> as it is done via the MD (x86 specific likely) interface. This is
>> wrong for 2 reasons:
>
> No, intr_disable() is MI.  It is also used by witness.  disable_intr()
> is the corresponding x86 interface that you may be thinking of.  The MI
> interface intr_disable() was introduced to avoid the MD'ness of
> intr_disable().

I was a bit surprised to verify that you are right but
spinlock_enter() has the big difference besides disable_intr() of also
explicitly disabling preemption via critical_enter() which some
codepath can trigger without even noticing it.
This means it is more safer in presence of PREEMPTION option on and
thus should be preferred to the normal intr_disable(), in particular
for convoluted codepaths.

>> 1) There may be some codepaths leading to explicit preemption
>> 2) It should  really use an MI interface
>>
>> The right way to do this should be via spinlock_enter().
>
> spinlock_enter() is MI, but has wrong semantics.  In my version of i386,
> spinlocks don't disable any h/w interrupt, as is needed for fast interrupt
> handlers to actually work.  I believe sparc64 is similar, except its
> spinlock_enter() disables most h/w interrupts and this includes fast
> interrupt handlers.  I don't understand sparc64, but it looks like its
> spinlock_enter() disables all interrupts visible in C code, but not
> all interrupts:

Can you please explain more about the 'h/w interrupts not disabled' in X86?
Are you speaking about NMIs? For those the only way to effectively
mask them would be to reprogram the LAPIC entry, but I don't really
think we may want that.

> from cpufunc.h:
> % static __inline register_t
> % intr_disable(void)
> % {
> %       register_t s;
> % %     s = rdpr(pstate);
> %       wrpr(pstate, s & ~PSTATE_IE, 0);
> %       return (s);
> % }
>
> This seems to mask all interrupts, as required.
>
>    (The interface here is slightly broken (non-MI).  It returns register_t.
>    This assumes that the interrupt state can be represented in a single
>    register.  The type critical_t exists to avoid the same bug in an
>    old version of critical_enter().  Now this type is just bogus.
>    critical_enter() no longer returns it.  Instead, spinlock_enter() uses
>    a non-reentrant interface which stores what used to be the return value
>    of critical_enter() in a per-thread MD data structure (md_saved_pil
>    in the above).  Most or all arches use register_t for this.  This
>    leaves critical_t as pure garbage -- the only remaining references to
>    it are for its definition.)

I mostly agree, I think we should have an MD specified type to replace
register_t for this (it could alias it, if it is suitable, but this
interface smells a lot like x86-centric).

Thanks,
Attilio


-- 
Peace can only be achieved by understanding - A. Einstein