Date: Fri, 16 Apr 2004 11:51:16 +1000 (EST) From: Bruce Evans <bde@zeta.org.au> To: Bill Paul <wpaul@freebsd.org> Cc: John Baldwin <jhb@freebsd.org> 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 Message-ID: <20040416115056.J11580@gamplex.bde.org> In-Reply-To: <20040415150939.F6628@gamplex.bde.org> References: <20040414220453.7806316A4CF@hub.freebsd.org> <20040415150939.F6628@gamplex.bde.org>
next in thread | previous in thread | raw e-mail | index | archive | help
[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
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20040416115056.J11580>