Re: CVS commit: src/sys/dev

[Juergen Hannken-Illjes <hannken%eis.cs.tu-bs.de@localhost>]

On Wed, Nov 24, 2010 at 02:40:45AM +0000, Mindaugas Rasiukevicius wrote:
> Hello,
> 
> "Juergen Hannken-Illjes" <hannken%netbsd.org@localhost> 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 - hannken%eis.cs.tu-bs.de@localhost - 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 */