On Sat, Sep 24, 2016 at 12:20:02PM +0200, Christiano F. Haesbaert wrote:
> 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.
yeah. that one took me a bit longer to understand.
the below includes my first diff, but also gets rid of the hold of
the timeout mutex while waiting for entries in the timeout_proc
list. it basically sleeps on CIRCQ_EMPTY outside the lock.
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 26 Sep 2016 02:56:21 -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
@@ -415,6 +419,7 @@ softclock_thread(void *arg)
{
CPU_INFO_ITERATOR cii;
struct cpu_info *ci;
+ struct sleep_state sls;
struct timeout *to;
KERNEL_ASSERT_LOCKED();
@@ -427,16 +432,18 @@ softclock_thread(void *arg)
KASSERT(ci != NULL);
sched_peg_curproc(ci);
- mtx_enter(&timeout_mutex);
for (;;) {
- while (CIRCQ_EMPTY(&timeout_proc))
- msleep(&timeout_proc, &timeout_mutex, PSWP, "bored", 0);
+ sleep_setup(&sls, &timeout_proc, PSWP, "bored");
+ sleep_finish(&sls, CIRCQ_EMPTY(&timeout_proc));
- to = timeout_from_circq(CIRCQ_FIRST(&timeout_proc));
- CIRCQ_REMOVE(&to->to_list);
- timeout_run(to);
+ mtx_enter(&timeout_mutex);
+ while (!CIRCQ_EMPTY(&timeout_proc)) {
+ to = timeout_from_circq(CIRCQ_FIRST(&timeout_proc));
+ CIRCQ_REMOVE(&to->to_list);
+ timeout_run(to);
+ }
+ mtx_leave(&timeout_mutex);
}
- mtx_leave(&timeout_mutex);
}
#ifndef SMALL_KERNEL
