Am Samstag, 24. September 2016 schrieb David Gwynne :
> On Fri, Sep 23, 2016 at 10:16:34PM -0700, Philip Guenther wrote:
> > On Fri, 23 Sep 2016, Christiano F. Haesbaert wrote:
> > ...
> > > The diff as it is will deadlock against SCHED_LOCK.
> > > tsleep() calls sleep_setup(), which grabs SCHED_LOCK,
> > > Then sleep_setup_timeout() will grab timeout_mutex in timeout_add()
> > >
> > > On softclock() you have the opposite:
> > > Grabs timeout_mutex then does a wakeup, which grabs SCHED_LOCK.
>
> nice.
>
> >
> > Hmm, yes. And softclock_thread() has the same problem: msleep() needs to
> > grab SCHED_LOCK while the passed in mutex is held, so it'll hold
> > timeout_mutex while grabbing SCHED_LOCK too.
> >
> > I just played around with using a separate mutex for protecting
> > timeout_proc, a mutex which would be 'outside' SCHED_LOCK, unlike
> > timeout_mutex which is 'inside' SCHED_LOCK. The catch is that supporting
> > all the functionality of timeout_add() and timeout_del() becomes ugly and
> > fragile: they would need to check whether the mutex has
> > TIMEOUT_NEEDPROCCTX set and, if so, grab the new mutex before grabbing
> > timeout_mutex. ??That's "safe" from being a lock loop from tsleep()
> because
> > the thread's p_sleep_to *can't* have that flag set, so the 'outside'
> mutex
> > wouldn't be needed....but geez is this ugly. I'm also unsure what
> defined
> > semantics, if any, timeout_triggered() should have for NEEDPROCCTX
> > timeouts. Should it return true once the timeout has been dequeued from
> > the timeout wheel, or should it only be set once softclock_thread is
> > actually going to run it?
> >
> >
> > ...or maybe this makes people think we should toss this out and go
> > directly to dlg@'s proposal...
>
> let's not go crazy.
>
> i believe the deadlock can be fixed by moving the wakeup outside
> the hold of timeout_mutex in timeout_add. you only need to wake up
> the thread once even if you queued multiple timeouts, cos the thread
> will loop until it completes all pending work. think of this as
> interrupt mitigation.
Yes, that was my solution as well, i don't have access to the code right
now, but I think this won't fix the msleep case guenther pointed out.
>
> it is worth noting that sleep_setup_timeout doesnt timeout_add if
> the wait is 0, so the thread doesnt recurse.
>
> Index: kern/kern_timeout.c
> ===================================================================
> RCS file: /cvs/src/sys/kern/kern_timeout.c,v
> retrieving revision 1.49
> diff -u -p -r1.49 kern_timeout.c
> --- kern/kern_timeout.c 22 Sep 2016 12:55:24 -0000 1.49
> +++ kern/kern_timeout.c 24 Sep 2016 09:45:09 -0000
> @@ -375,6 +375,7 @@ softclock(void *arg)
> int delta;
> struct circq *bucket;
> struct timeout *to;
> + int needsproc = 0;
>
> mtx_enter(&timeout_mutex);
> while (!CIRCQ_EMPTY(&timeout_todo)) {
> @@ -391,7 +392,7 @@ softclock(void *arg)
> CIRCQ_INSERT(&to->to_list, bucket);
> } else if (to->to_flags & TIMEOUT_NEEDPROCCTX) {
> CIRCQ_INSERT(&to->to_list, &timeout_proc);
> - wakeup(&timeout_proc);
> + needsproc = 1;
> } else {
> #ifdef DEBUG
> if (delta < 0)
> @@ -401,6 +402,9 @@ softclock(void *arg)
> }
> }
> mtx_leave(&timeout_mutex);
> +
> + if (needsproc)
> + wakeup(&timeout_proc);
> }
>
> void
>
