From owner-cvs-src@FreeBSD.ORG Thu Apr 15 18:51:21 2004 Return-Path: Delivered-To: cvs-src@freebsd.org Received: from mx1.FreeBSD.org (mx1.freebsd.org [216.136.204.125]) by hub.freebsd.org (Postfix) with ESMTP id 07BCD16A4CE; Thu, 15 Apr 2004 18:51:21 -0700 (PDT) Received: from mailout2.pacific.net.au (mailout2.pacific.net.au [61.8.0.85]) by mx1.FreeBSD.org (Postfix) with ESMTP id 610E243D1F; Thu, 15 Apr 2004 18:51:20 -0700 (PDT) (envelope-from bde@zeta.org.au) Received: from mailproxy2.pacific.net.au (mailproxy2.pacific.net.au [61.8.0.87])i3G1pJ5v011113; Fri, 16 Apr 2004 11:51:19 +1000 Received: from gamplex.bde.org (katana.zip.com.au [61.8.7.246]) i3G1pGHW006022; Fri, 16 Apr 2004 11:51:17 +1000 Date: Fri, 16 Apr 2004 11:51:16 +1000 (EST) From: Bruce Evans X-X-Sender: bde@gamplex.bde.org To: Bill Paul In-Reply-To: <20040415150939.F6628@gamplex.bde.org> Message-ID: <20040416115056.J11580@gamplex.bde.org> References: <20040414220453.7806316A4CF@hub.freebsd.org> <20040415150939.F6628@gamplex.bde.org> MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII cc: cvs-src@freebsd.org cc: src-committers@freebsd.org cc: cvs-all@freebsd.org cc: John Baldwin Subject: Re: cvs commit: src/sys/compat/ndis hal_var.h kern_ndis.cndis_var.h ntoskrnl_var.h pe_var.h subr_hal.c subr_ndis.c subr_ntoskrn X-BeenThere: cvs-src@freebsd.org X-Mailman-Version: 2.1.1 Precedence: list List-Id: CVS commit messages for the src tree List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 16 Apr 2004 01:51:21 -0000 [Resending after bounce.] On Wed, 14 Apr 2004, Bill Paul wrote: > > > Now, I'm sure many people will be seized by the urge to criticize > > > me for doing an end run around our own spinlock implementation, but > > > it makes more sense to do it this way. Well, it does to me anyway. > > > > If you don't use atomic ops with memory barriers somewhere (like the mutex > > implementation does) then NDIS won't work on SMP. Really, IRQL is basically > > spl()s, and we don't use an spl-like model anymore. Just using mutexes for > > locking should give you all the protection you need. > > Protection is all well and good, but I need to provide the right semantics > as well. I need to fool the drivers into thinking they can depend on the > usual Windows behavior, and make it easy to use the Windows data types, > and it's a pain in the butt to do that with regular mutexes. > > And besides, I wanna. > > Now, from subr_ntoskrnl.c: > > __stdcall void > ntoskrnl_lock_dpc(/*lock*/ void) > { > kspin_lock *lock; > > __asm__ __volatile__ ("" : "=c" (lock)); > > while (atomic_cmpset_int((volatile u_int *)lock, 0, 1) == 0) > /* do nothing */; > > return; > } > > __stdcall void > ntoskrnl_unlock_dpc(/*lock*/ void) > { > kspin_lock *lock; > > __asm__ __volatile__ ("" : "=c" (lock)); > > atomic_cmpset_int((volatile u_int *)lock, 1, 0); > > return; > } > > These two routines do the actual work of acquiring and releasing the > lock (they map to KefAcquireSpinLockAtDpcLevel() and > KefReleaseSpinLockFromDpcLevel). Are you saying the former routine > should use atomic_cmpset_acq_int() and the latter atomic_cmpset_rel_int()? I think using the "acq" versions is semantically required. However, using them would make no difference, since this code is very i386-dependent despite it being in in an MI directory, and atomic_cmpset_{acq,rel}_int are just aliases for atomic_cmpset_int on i386's. Using atomic_cmpset_int() for unlocking is bogus, especially since it is used without a loop to check that the store succeeded. It only works if the lock is already held so that the "cmp" part has no effect. This can be expressed more clearly using atomic_store_rel_int() like the normal mutex code does in _release_lock_quick(). The above code also has lots of style bugs. Fixing these problems gives: %%% __stdcall void ntoskrnl_lock_dpc(/* lock */ void) { int *lock; __asm __volatile("" : "=c" (lock)); while (atomic_cmpset_int(lock, 0, 1) == 0) ; } __stdcall void ntoskrnl_unlock_dpc(/* lock */ void) { int *lock; __asm __volatile("" : "=c" (lock)); atomic_store_rel(lock, 0); } %%% The asm is still too fragile. Isn't there a way to directly specify that "int *lock" is an arg passed in %ecx? I copied half of the fixed version from my end run around mutexes in my version of the cy driver, which was in turn half copied from the simple locking (simple_lock interfaces) in RELENG_4. %%% static void CY_LOCK(void) { while (atomic_cmpset_acq_int(&cy_slock, 0, 1) == 0) ; } static void CY_UNLOCK(void) { atomic_store_rel_int(&cy_slock, 0); } %%% This leaves out half of the complications for locking in the cy driver. Simple locks are only used in the SMP case. For UP, locking at this level consists of disabling all interrupts. For SMP, locking consists of the above plus disabling all interrupts on the running CPU. Disabling interrupts on the running CPU is needed to avoid deadlock. I'm not sure how ndis works without it. In the SMP case, interrupts can still be received on other CPUs; if interrupt handling gets as far as CY_LOCK() then CY_LOCK() just spins if necessary. This behaviour is the same as given by ordinary spin mutexes, except: - disabling of interrupts on the running CPU is decoupled from acquiring a spinlock. This makes no difference except to add complications in the cy driver, since the disabling is still required. The difference is that all the other places that acquire a spinlock and hold it for too long don't prevent the cy driver seeing and handling interrupts. Wiht FreeBSD mutexes, holding a spinlock stops seeing interrupts in the UP case; in the SMP case, the interrupts may be seen on other CPUs but their handling may block, with blocking on sched_lock being too common. - it loses all of the mutex debugging facilities in drivers that don't use normal mutexes. This loss is not large, at least in the cy driver, because only a simple leaf lock is involved and mutex debugging facilities often need to be turned off anyway for low level locks. Bruce