On 2005-09-09 ritter.alan@gmail.com wrote:

> OK, I added macros that raise the IPL, then grab a simple lock everyplace
> there is synchronization required between the top and bottom halves

  Pass the variable name to the macros and use do { ... } while (0) around
multiple statements in macros (so that if (...) MACRO; works as expected).

> There were a lot of places I was using lockmgr locks that I removed, and the
> driver seems to work just the same (maybe a bit faster).  I originally just
> replaced the freebsd mtx_lock(&lock) with lockmgr(&lock, LK_EXCLUSIVE,
> NULL), and I tried removing this entirely.  This seemed to cause problems
> however, so I put it back.  There are a few places in the FreeBSD code
> where they do mtx_lock(&Giant), so I'm thinking I might need to use lockmgr
> locks there.

  The other cases where lockmgr is used is where code being called expects
to be able to sleep without releasing the lock (vfs does this with vnode
locks) or acquire the lock recursively.  Both should be avoided if
possible.  In particular, I think it is a good idea to separate
refcounting where you only care that the object continues to exist from
locking where you want to access a consistant version of the object.
Both of these are necessary concerns even on SP machines to avoid
unexpected variable modification, invalid references, and memory leakage.
It is good to specify what the lock protects if not the entire data
structure that holds it.

  I would guess the Giant lock is equivalent to NetBSD's KERNEL_LOCK.  My
understanding is that you shouldn't have to worry about that right now
since currently only one CPU is allowed to be in the kernel at any time
except for a few special cases (most drivers do not yet do locking).

  The other places I see mtx_lock used are:
* kern_ndis.c as a pointless interlock (pointless because the test is done
outside the lock and the wakeup doesn't use the lock)

* kern_windrv.c currently need lockmgr due to malloc call in
ExAllocatePoolWithTag from IoCreateDevice which is called under the lock,
apparently to protect the driver_object from being deallocated.  However,
the time between windrv_lookup and use of the drv is not protected, so
this looks like bad lock usage.

  What problems did you see without the mtx_lock calls?

  Your hang with the compat spin locks enabled means that something is
sleeping or returning without releasing the lock.  The could indicate a
code problem that affects SP machines also.

  Also, in ndis_set_info you can return without releasing NDIS_LOCK.

Matthew Orgass
darkstar@city-net.com