Date: Mon, 26 Sep 2011 15:59:16 +0200 From: Attilio Rao <attilio@freebsd.org> To: Bruce Evans <brde@optusnet.com.au> Cc: svn-src-head@freebsd.org, svn-src-all@freebsd.org, src-committers@freebsd.org Subject: Re: svn commit: r225372 - head/sys/kern Message-ID: <CAJ-FndD%2BfWPW_XFAChG5UbOR3iAvU5i7Lo35=8J_1gdRWmGcLg@mail.gmail.com> In-Reply-To: <20110905023251.C832@besplex.bde.org> References: <201109041307.p84D72GY092462@svn.freebsd.org> <CAJ-FndDa=xmvrcn9CdgMvDZ_vG3pjUdFqLH=Q%2BVK%2BSWjvGFO9g@mail.gmail.com> <20110905023251.C832@besplex.bde.org>
next in thread | previous in thread | raw e-mail | index | archive | help
2011/9/4 Bruce Evans <brde@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. =C2=A0It is also used by witness. =C2=A0disable=
_intr()
> is the corresponding x86 interface that you may be thinking of. =C2=A0The=
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 =C2=A0really use an MI interface
>>
>> The right way to do this should be via spinlock_enter().
>
> spinlock_enter() is MI, but has wrong semantics. =C2=A0In my version of i=
386,
> spinlocks don't disable any h/w interrupt, as is needed for fast interrup=
t
> handlers to actually work. =C2=A0I believe sparc64 is similar, except its
> spinlock_enter() disables most h/w interrupts and this includes fast
> interrupt handlers. =C2=A0I don't understand sparc64, but it looks like i=
ts
> 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)
> % {
> % =C2=A0 =C2=A0 =C2=A0 register_t s;
> % % =C2=A0 =C2=A0 s =3D rdpr(pstate);
> % =C2=A0 =C2=A0 =C2=A0 wrpr(pstate, s & ~PSTATE_IE, 0);
> % =C2=A0 =C2=A0 =C2=A0 return (s);
> % }
>
> This seems to mask all interrupts, as required.
>
> =C2=A0 =C2=A0(The interface here is slightly broken (non-MI). =C2=A0It re=
turns register_t.
> =C2=A0 =C2=A0This assumes that the interrupt state can be represented in =
a single
> =C2=A0 =C2=A0register. =C2=A0The type critical_t exists to avoid the same=
bug in an
> =C2=A0 =C2=A0old version of critical_enter(). =C2=A0Now this type is just=
bogus.
> =C2=A0 =C2=A0critical_enter() no longer returns it. =C2=A0Instead, spinlo=
ck_enter() uses
> =C2=A0 =C2=A0a non-reentrant interface which stores what used to be the r=
eturn value
> =C2=A0 =C2=A0of critical_enter() in a per-thread MD data structure (md_sa=
ved_pil
> =C2=A0 =C2=A0in the above). =C2=A0Most or all arches use register_t for t=
his. =C2=A0This
> =C2=A0 =C2=A0leaves critical_t as pure garbage -- the only remaining refe=
rences to
> =C2=A0 =C2=A0it 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
--=20
Peace can only be achieved by understanding - A. Einstein
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?CAJ-FndD%2BfWPW_XFAChG5UbOR3iAvU5i7Lo35=8J_1gdRWmGcLg>