Re: CVS commit: src/sys/dev

[Mindaugas Rasiukevicius <rmind%netbsd.org@localhost>]

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?

> @@ -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).

> @@ -421,6 +435,8 @@ mdstrategy(struct buf *bp)
>       }
>   done:
>       biodone(bp);
> +
> +     mutex_exit(&sc->sc_lock);

Any reason why biodone() is under lock?

> @@ -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).

> +     kmutex_t sc_lock;       /* Protect self. */
> +     kcondvar_t sc_cv;       /* Signal work. */

I think "Signal work" is missleading. :)

-- 
Mindaugas