Date: Tue, 01 Nov 2005 11:02:51 -0800 From: Julian Elischer <julian@elischer.org> To: Dinesh Nair <dinesh@alphaque.com> Cc: freebsd-hackers@freebsd.org, Scott Long <scottl@samsco.org> Subject: Re: locking in a device driver Message-ID: <4367BBDB.7020005@elischer.org> In-Reply-To: <4367AA8D.3060506@alphaque.com> References: <4360B8EE.4070605@alphaque.com> <4360DD7B.20900@samsco.org> <4361044B.50807@alphaque.com> <20051027.205250.55834228.imp@bsdimp.com> <4361E3E0.4090409@alphaque.com> <43676121.4030801@alphaque.com> <436791ED.8010808@samsco.org> <4367AA8D.3060506@alphaque.com>
next in thread | previous in thread | raw e-mail | index | archive | help
Dinesh Nair wrote: > > On 11/02/05 00:03 Scott Long said the following: > >> I think this thread has gone too far into hyperbole and conjecture. >> What is your code trying to do, and what problems are you seeing? > > > apologies, scott. i'm actually trying to get a driver written for > freebsd 5.x backported to 4.x. the driver works like a charm on 5.x. > > under 4.x, i seem to be getting problems with synchronization/locking. > > the driver set consists of two drivers, a pseudo driver which the > userland reads/writes/ioctls to, and the actual device driver which > reads/writes from the device thru DMA and places the data into a ring > buffer. > > this is the sequence of events for data from the device: > > 1. interrupt handler of device driver called handler called at splxxx() level (see later) > 2. device driver reads data from DMA memory space still at splxxx level > 3. device driver writes to a shared buffer still at splxxx level > > 4. device driver calls a function inside pseudo driver still at splxxx level > 5. pseudo driver copies data from shared buffer to another buffer still at splxx level > 6. wakeup() is called still at splxxx level > 7. device driver waits for next interrupt drops to splzero or similar,.. woken process called, starts manipulating "another buffer" collides with next interrupt. what ever is woken up needs to call splxxx() to block the interupt routine while manipulating "another buffer" for as long as it takes to get the data out or swap the b uffer pointers or whatever it does. > > the interrupt handler uses splhigh()/splx() to mask out interrupts > during the time it's executing. interrupts happen 1000 times a second > consistently. When you register an interrupt handler, you specify which of several groups you are a part of.. Network, disk io, etc. once you specify you are part of a particular set, then you should only block interrupts from that set.. e.g, a netowrk driver would use s = splimp(); ...; splx(s) Try not to use splhigh().. it is ok for getting your driver going but may block too much. > > when a read on the pseudo device is called from a -lc_r threaded > userland process, the following happens in pseudo device driver: > > 7. tsleep() is called, (with the same ident as the wakeup() in #6) > 8. pseudo device reads from buffer in #5 and uses uiomove to return > data to calling process it needs to call splxxx() while it is doing it.. I would suggest having two buffers and swapping them under splxxx() so that the one that the driver is accessing is not the one you are draining. that way teh splxxx() levle needs to only be held for the small time you are doing the swap. > > exactly the reverse happens for a write. > > i believe that steps 3,5,8 need to be protected/synchronized with locks. not locks, but spl, and only step 8 needs to be changed because all teh rest are already done at high spl. > > the code uses mtx_lock/mtx_unlock in the 5.x version, and uses > simple_lock in the 4.x version. digging thru the include files, i've > noticed that simple_lock is a NOOP in 4.x if you're on a single > processor. > > could i replace the mtx_lock/mtx_unlock with > lockmgr(...,LK_EXCLUSIVE/LK_RELEASE) instead ? the earlier notion of > using > splhigh()/splx() to protect the common areas didnt seem like the right > way to do it since the pseudo device driver is not interrupt driven, > but rather is driven by open/read/write/ioctl from the userland. > > also, when the threaded userland reads, it's first put to a tsleep > with PZERO|PCATCH and this tsleep is surrounded by a mtx_lock(&Giant) > and mtx_unlock(&Giant). what should this be replaced with in 4.x ? > > when the real device driver has data from the device, it uses wakeup() > to wake the tsleep'ed bits in the pseudo device driver. is this the > correct way to do this ? > > also, is there any equivalent for PROC_LOCK()/PROC_UNLOCK() in 4.x or > is it unnecessary ? > use spl???()/splx uinstead. your top end needs to raise to the same level as the interrupt you have registered for as long as it is manupulating data that the bottom end might manipulate, (e.g. buffer pointers or linked lists, etc.)
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?4367BBDB.7020005>