> Date: Mon, 11 May 2015 16:54:54 +0200
> From: Mike Belopuhov <m...@belopuhov.com>
>
> On Fri, May 08, 2015 at 20:28 +0200, Mark Kettenis wrote:
> > > Date: Fri, 8 May 2015 20:15:58 +0200
> > > From: Mike Belopuhov <m...@belopuhov.com>
> > >
> > > On Fri, May 08, 2015 at 12:34 +0200, Mike Belopuhov wrote:
> > > > > I think tsleep(9) and msleep(9) need to release and re-acquire the
> > > > > kernel lock in the "cold || panicstr" case.
> > > >
> > > > Well, it's not hard to do really, but...
> > > >
> > > > > We might need this for
> > > > > handling interrupts during autoconf as soon as we start distributing
> > > > > interrupts over CPUs.
> > > > >
> > > >
> > > > ...cold might mean that interrupts are not ready yet. So then we might
> > > > need another flag for shutdown?
> > >
> > > This is what I have come up with. Chunks were taken directly from
> > > mi_switch and it seems to do the job just fine. Right now I'm not
> > > using any additional flags and it seems to work here. I'll resume
> > > testing on Monday, but it looks fairly complete. Any comments?
> >
> > Makes sense to me. The msleep/tsleep code could be simplified to:
> >
> > if (__mp_lock_held(&kernel_lock)) {
> > hold_count = __mp_release_all(&kernel_lock);
> > __mp_acquire_count(&kernel_lock, hold_count);
> > }
> >
>
> Indeed.
>
> > I'm also wondering whether we should change __mp_release_all() to
> > simple return 0 if the current CPU does not hold the lock, such that
> > the __mp_lock_held() check isn't needed anymore. But that's a
> > separate issue.
> >
>
> It would have made the __mp_release_all safer as well since it
> wouldn't require an external check.
>
> I don't have any firther input on this, diff works fine here.
>
> OK?
ok kettenis@
> diff --git sys/kern/kern_synch.c sys/kern/kern_synch.c
> index 03308b4..6f573fc 100644
> --- sys/kern/kern_synch.c
> +++ sys/kern/kern_synch.c
> @@ -103,10 +103,13 @@ extern int safepri;
> int
> tsleep(const volatile void *ident, int priority, const char *wmesg, int timo)
> {
> struct sleep_state sls;
> int error, error1;
> +#ifdef MULTIPROCESSOR
> + int hold_count;
> +#endif
>
> KASSERT((priority & ~(PRIMASK | PCATCH)) == 0);
>
> #ifdef MULTIPROCESSOR
> KASSERT(timo || __mp_lock_held(&kernel_lock));
> @@ -120,10 +123,16 @@ tsleep(const volatile void *ident, int priority, const
> char *wmesg, int timo)
> * don't run any other procs or panic below,
> * in case this is the idle process and already asleep.
> */
> s = splhigh();
> splx(safepri);
> +#ifdef MULTIPROCESSOR
> + if (__mp_lock_held(&kernel_lock)) {
> + hold_count = __mp_release_all(&kernel_lock);
> + __mp_acquire_count(&kernel_lock, hold_count);
> + }
> +#endif
> splx(s);
> return (0);
> }
>
> sleep_setup(&sls, ident, priority, wmesg);
> @@ -149,10 +158,13 @@ int
> msleep(const volatile void *ident, struct mutex *mtx, int priority,
> const char *wmesg, int timo)
> {
> struct sleep_state sls;
> int error, error1, spl;
> +#ifdef MULTIPROCESSOR
> + int hold_count;
> +#endif
>
> KASSERT((priority & ~(PRIMASK | PCATCH | PNORELOCK)) == 0);
> KASSERT(mtx != NULL);
>
> if (cold || panicstr) {
> @@ -163,10 +175,16 @@ msleep(const volatile void *ident, struct mutex *mtx,
> int priority,
> * in case this is the idle process and already asleep.
> */
> spl = MUTEX_OLDIPL(mtx);
> MUTEX_OLDIPL(mtx) = safepri;
> mtx_leave(mtx);
> +#ifdef MULTIPROCESSOR
> + if (__mp_lock_held(&kernel_lock)) {
> + hold_count = __mp_release_all(&kernel_lock);
> + __mp_acquire_count(&kernel_lock, hold_count);
> + }
> +#endif
> if ((priority & PNORELOCK) == 0) {
> mtx_enter(mtx);
> MUTEX_OLDIPL(mtx) = spl;
> } else
> splx(spl);
> diff --git sys/kern/vfs_subr.c sys/kern/vfs_subr.c
> index a26fbe2..a373789 100644
> --- sys/kern/vfs_subr.c
> +++ sys/kern/vfs_subr.c
> @@ -1664,10 +1664,13 @@ int
> vfs_syncwait(int verbose)
> {
> struct buf *bp;
> int iter, nbusy, dcount, s;
> struct proc *p;
> +#ifdef MULTIPROCESSOR
> + int hold_count;
> +#endif
>
> p = curproc? curproc : &proc0;
> sys_sync(p, (void *)0, (register_t *)0);
>
> /* Wait for sync to finish. */
> @@ -1698,11 +1701,21 @@ vfs_syncwait(int verbose)
> }
> if (nbusy == 0)
> break;
> if (verbose)
> printf("%d ", nbusy);
> +#ifdef MULTIPROCESSOR
> + if (__mp_lock_held(&kernel_lock))
> + hold_count = __mp_release_all(&kernel_lock);
> + else
> + hold_count = 0;
> +#endif
> DELAY(40000 * iter);
> +#ifdef MULTIPROCESSOR
> + if (hold_count)
> + __mp_acquire_count(&kernel_lock, hold_count);
> +#endif
> }
>
> return nbusy;
> }
>
>
