From owner-svn-src-head@FreeBSD.ORG Mon Sep 26 13:59:18 2011 Return-Path: Delivered-To: svn-src-head@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:4f8:fff6::34]) by hub.freebsd.org (Postfix) with ESMTP id B92EA106564A; Mon, 26 Sep 2011 13:59:18 +0000 (UTC) (envelope-from asmrookie@gmail.com) Received: from mail-ey0-f182.google.com (mail-ey0-f182.google.com [209.85.215.182]) by mx1.freebsd.org (Postfix) with ESMTP id E53F58FC0A; Mon, 26 Sep 2011 13:59:17 +0000 (UTC) Received: by eyg7 with SMTP id 7so4808023eyg.13 for ; Mon, 26 Sep 2011 06:59:16 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=gamma; h=mime-version:sender:in-reply-to:references:date :x-google-sender-auth:message-id:subject:from:to:cc:content-type :content-transfer-encoding; bh=WU915msPYJUVkrZEUfLf1e3/Dmf4dYkwMaPcg0AmnhM=; b=v/hJHreD0ftsOQKCVbs2zSOC3COjZIq+dvqMTzJFnyuIalG6HtlDmcR8c49RNAB2ND mrG2qxPO2cu77GaMuW5s+tZ8i0rXlTCubl1X0J1Aabtcda3nP49EVJKpD8+pr6rQGtH9 AQwTyWwjWCEYKogP1GzmZyw9aM0f++l/71n+s= MIME-Version: 1.0 Received: by 10.216.138.29 with SMTP id z29mr7517730wei.42.1317045556714; Mon, 26 Sep 2011 06:59:16 -0700 (PDT) Sender: asmrookie@gmail.com Received: by 10.216.179.78 with HTTP; Mon, 26 Sep 2011 06:59:16 -0700 (PDT) In-Reply-To: <20110905023251.C832@besplex.bde.org> References: <201109041307.p84D72GY092462@svn.freebsd.org> <20110905023251.C832@besplex.bde.org> Date: Mon, 26 Sep 2011 15:59:16 +0200 X-Google-Sender-Auth: YhNFDxMjRghuH6UULO5WE0-lmJs Message-ID: From: Attilio Rao To: Bruce Evans Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable Cc: svn-src-head@freebsd.org, svn-src-all@freebsd.org, src-committers@freebsd.org Subject: Re: svn commit: r225372 - head/sys/kern X-BeenThere: svn-src-head@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: SVN commit messages for the src tree for head/-current List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 26 Sep 2011 13:59:18 -0000 2011/9/4 Bruce Evans : > 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