Date: Thu, 20 Mar 2008 21:32:25 -0700 From: Alfred Perlstein <alfred@freebsd.org> To: smp@freebsd.org Subject: request for review, callout fix. Message-ID: <20080321043225.GZ67856@elvis.mu.org>
next in thread | raw e-mail | index | archive | help
--6lXr1rPCNTf1w0X8
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
Hi guys, attached is a fix for the timeout/untimeout race with
Giant locked code.
Basically the old code would make the callout inaccessable
right before calling it inside of softclock.
However only the callout lock is held, so when switching to
the callout's associated mutex (in this case Giant) there's
a race where a "local" (timeout/untimeout) callout would be
fired even if stopped.
This patch fixes that. We've run several hours of regression
testing on a version of this for 6.x.
People internal to Juniper and iedowse@ helped with this.
Please review/comment.
thank you,
--
- Alfred Perlstein
--6lXr1rPCNTf1w0X8
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment; filename="kern_timeout.diff"
cvs diff: Diffing .
Index: kern_timeout.c
===================================================================
RCS file: /home/ncvs/src/sys/kern/kern_timeout.c,v
retrieving revision 1.110
diff -u -r1.110 kern_timeout.c
--- kern_timeout.c 12 Mar 2008 06:31:06 -0000 1.110
+++ kern_timeout.c 21 Mar 2008 04:28:35 -0000
@@ -230,11 +230,8 @@
c_arg = c->c_arg;
c_flags = c->c_flags;
if (c->c_flags & CALLOUT_LOCAL_ALLOC) {
- c->c_func = NULL;
c->c_flags = CALLOUT_LOCAL_ALLOC;
- SLIST_INSERT_HEAD(&callfree, c,
- c_links.sle);
- curr_callout = NULL;
+ curr_callout = c;
} else {
c->c_flags =
(c->c_flags & ~CALLOUT_PENDING);
@@ -299,6 +296,24 @@
class->lc_unlock(c_lock);
skip:
mtx_lock_spin(&callout_lock);
+ /*
+ * If the current callout is locally
+ * allocated (timeout(9)) and if it has not
+ * been killed by untimeout(9)
+ * then put it on the freelist.
+ *
+ * Note: we need to check the cached
+ * copy of c_flags because if it was not
+ * local, then it's not safe to deref the
+ * callout pointer.
+ */
+ if (c_flags & CALLOUT_LOCAL_ALLOC &&
+ !(c->c_flags &
+ (CALLOUT_PENDING | CALLOUT_ACTIVE))) {
+ c->c_func = NULL;
+ SLIST_INSERT_HEAD(&callfree, c,
+ c_links.sle);
+ }
curr_callout = NULL;
if (callout_wait) {
/*
--6lXr1rPCNTf1w0X8--
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20080321043225.GZ67856>