Date: Fri, 16 Apr 2004 22:10:46 +1000 (EST) From: Bruce Evans <bde@zeta.org.au> To: Bill Paul <wpaul@FreeBSD.org> Cc: 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: <20040416205956.K13468@gamplex.bde.org> In-Reply-To: <20040416072519.AD62016A4D2@hub.freebsd.org> References: <20040416072519.AD62016A4D2@hub.freebsd.org>
next in thread | previous in thread | raw e-mail | index | archive | help
On Fri, 16 Apr 2004, Bill Paul wrote:
> [...]
[...]
> > 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().
>
> ntoskrnl_unlock_dpc() should only ever be called when a lock is already
> held. If it's ever called without a lock held, that's a bug.
That's why the code is bogus. To release a lock that is known to be held,
just store_rel to it.
> > The asm is still too fragile. Isn't there a way to directly specify that
> > "int *lock" is an arg passed in %ecx?
>
> Yes: upgrade to gcc 3.4, which has __attribute__((__fastcall__)). Of course,
> first you have to wait for gcc 3.4 to be released.
>
> People have suggested various different hacks to duplicate _fastcall.
> semantics. This is the only one which produced the correct result in all
> cases that I tested, with optimization turned on. You can engage in a
> certain amount of deception with the __regparm()__ attribute, but gcc has
> certain ideas about what arguments go into which registers when you use
> regparm which don't agree with the way _fastcall does things. (This is
> why I don't use __regparm(3)__: it will use %ecx and %edx, but it will
> also drag %eax into the mix.)
__regparm__(3) on i386's is documented to cause the first 3 function
parameters to be in %eax, %edx and %ecx, in that order. In function
definitions you can use it after declaring placeholders for register
parameters that aren't actually passed. E.g.:
__stdcall __attribute__((__regparm__(3))) void
ntoskrnl_lock_dpc(int eax /* placeholder */, int edx /* placeholder */,
kspin_lock *lock)
{
while (atomic_cmpset_int(lock, 0, 1) == 0)
;
}
Passing only a nonstandard subset of the registers specified by regparm
is harder, but this is already handled by doing it in asm. (The asm has
similar (but much larger) fragility to that fixed above; the call should
be in asm too to prevent clobbering the parameter registers, and such
clobbering is very close -- the call is through a pointer and nothing
prevents loading the pointer into a parameter register.)
> > 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.
> [...]
> At this point, I am forced to conclude that you didn't actually read
> the code, since if you had, you might have understood. I will try to
> explain it again, hopefully in a way that makes it clear how Windows
> does things.
That's more than I want to understand about Windows :-). This thread was
just about correct implementation of the lowest level of spinlocks when
not using mutexes.
Bruce
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20040416205956.K13468>