On Wed, Nov 24, 2010 at 02:40:45AM +0000, Mindaugas Rasiukevicius wrote: > Hello, > > "Juergen Hannken-Illjes" <hann...@netbsd.org> wrote: > > Module Name: src > > Committed By: hannken > > Date: Tue Nov 23 09:30:43 UTC 2010 > > > > Modified Files: > > src/sys/dev: md.c > > > > Log Message: > > Make md(4) mp-safe. > > > > > > To generate a diff of this commit: > > cvs rdiff -u -r1.64 -r1.65 src/sys/dev/md.c > > Few comments: > > > @@ -597,15 +626,18 @@ md_server_loop(struct md_softc *sc) > > int error; > > bool is_read; > > > > + KASSERT(mutex_owned(&sc->sc_lock)); > > + > > for (;;) { > > /* Wait for some work to arrive. */ > > while ((bp = bufq_get(sc->sc_buflist)) == NULL) { > > - error = tsleep((void *)sc, md_sleep_pri, "md_idle", 0); > > + error = cv_wait_sig(&sc->sc_cv, &sc->sc_lock); > > <...> > > biodone(bp); > > + mutex_enter(&sc->sc_lock); > > } > > Is this (as well as other parts of code) are safe in respect of mdclose()? > For example, what happens if other thread executes close(2) while the lock > is dropped here?
The last close will detach (and drain the queue). In the UMEM_SERVER case the umem server (the thread running the ioctl) has to close before we detach on last close. > > @@ -383,7 +396,8 @@ mdstrategy(struct buf *bp) > > case MD_UMEM_SERVER: > > /* Just add this job to the server's queue. */ > > bufq_put(sc->sc_buflist, bp); > > - wakeup((void *)sc); > > + cv_signal(&sc->sc_cv); > > + mutex_exit(&sc->sc_lock); > > It should be cv_broadcast(9). No. There is only one possible waiter (the umem server thread). > > @@ -421,6 +435,8 @@ mdstrategy(struct buf *bp) > > } > > done: > > biodone(bp); > > + > > + mutex_exit(&sc->sc_lock); > > Any reason why biodone() is under lock? No. Will fix. See diff attached. > > @@ -534,6 +561,8 @@ md_ioctl_kalloc(struct md_softc *sc, str > > vaddr_t addr; > > vsize_t size; > > > > + KASSERT(mutex_owned(&sc->sc_lock)); > > Ideally, allocations should be outside the locks (just FYI). Ok. Will fix. See diff attached. > > + kmutex_t sc_lock; /* Protect self. */ > > + kcondvar_t sc_cv; /* Signal work. */ > > I think "Signal work" is missleading. :) No. It DOES signal work to the umem server. > -- > Mindaugas Btw.: The KMEM server was and is fishy. The memory will never be freed. -- Juergen Hannken-Illjes - hann...@eis.cs.tu-bs.de - TU Braunschweig (Germany) Index: md.c =================================================================== RCS file: /cvsroot/src/sys/dev/md.c,v retrieving revision 1.65 diff -p -u -2 -r1.65 md.c --- md.c 23 Nov 2010 09:30:43 -0000 1.65 +++ md.c 24 Nov 2010 10:15:48 -0000 @@ -434,8 +434,9 @@ mdstrategy(struct buf *bp) break; } - done: - biodone(bp); + done: mutex_exit(&sc->sc_lock); + + biodone(bp); } @@ -562,12 +563,21 @@ md_ioctl_kalloc(struct md_softc *sc, str vsize_t size; - KASSERT(mutex_owned(&sc->sc_lock)); + mutex_exit(&sc->sc_lock); /* Sanity check the size. */ size = umd->md_size; addr = uvm_km_alloc(kernel_map, size, 0, UVM_KMF_WIRED|UVM_KMF_ZERO); + + mutex_enter(&sc->sc_lock); + if (!addr) return ENOMEM; + /* If another thread beat us to configure this unit: fail. */ + if (sc->sc_type != MD_UNCONFIGURED) { + uvm_km_free(kernel_map, addr, size, UVM_KMF_WIRED); + return EINVAL; + } + /* This unit is now configured. */ sc->sc_addr = (void *)addr; /* kernel space */