Date: Tue, 01 Nov 2005 11:07:17 -0800 From: Julian Elischer <julian@elischer.org> To: Julian Elischer <julian@elischer.org> Cc: freebsd-hackers@freebsd.org, Scott Long <scottl@samsco.org> Subject: Re: locking in a device driver Message-ID: <4367BCE5.2070900@elischer.org> In-Reply-To: <4367BBDB.7020005@elischer.org> 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> <4367BBDB.7020005@elischer.org>
next in thread | previous in thread | raw e-mail | index | archive | help
Julian Elischer wrote: > 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. > the interrupt handler is ALREADY blocking it's own interupt. you don't need to do anything. You need to block interrupts for a momen tin #8, not in the interrupt handler. > > > 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.) > _______________________________________________ > freebsd-hackers@freebsd.org mailing list > http://lists.freebsd.org/mailman/listinfo/freebsd-hackers > To unsubscribe, send any mail to > "freebsd-hackers-unsubscribe@freebsd.org"
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?4367BCE5.2070900>