Print this page
OS-2366 ddi_periodic_add(9F) is entirely rubbish (MORE)

@@ -1,36 +1,21 @@
 /*
- * CDDL HEADER START
+ * This file and its contents are supplied under the terms of the
+ * Common Development and Distribution License ("CDDL"), version 1.0.
+ * You may only use this file in accordance with the terms of version
+ * 1.0 of the CDDL.
  *
- * The contents of this file are subject to the terms of the
- * Common Development and Distribution License (the "License").
- * You may not use this file except in compliance with the License.
- *
- * You can obtain a copy of the license at usr/src/OPENSOLARIS.LICENSE
- * or http://www.opensolaris.org/os/licensing.
- * See the License for the specific language governing permissions
- * and limitations under the License.
- *
- * When distributing Covered Code, include this CDDL HEADER in each
- * file and include the License file at usr/src/OPENSOLARIS.LICENSE.
- * If applicable, add the following below this CDDL HEADER, with the
- * fields enclosed by brackets "[]" replaced with your own identifying
- * information: Portions Copyright [yyyy] [name of copyright owner]
- *
- * CDDL HEADER END
+ * A full copy of the text of the CDDL should have accompanied this
+ * source.  A copy of the CDDL is also available via the Internet at
+ * http://www.illumos.org/license/CDDL.
  */
-
 /*
- * Copyright 2009 Sun Microsystems, Inc.  All rights reserved.
- * Use is subject to license terms.
- */
-/*
  * Copyright (c) 2013, Joyent, Inc. All rights reserved.
  */
 
 #include <sys/cmn_err.h>
-#include <sys/ddi_timer.h>
+#include <sys/ddi_periodic.h>
 #include <sys/id_space.h>
 #include <sys/kobj.h>
 #include <sys/sysmacros.h>
 #include <sys/systm.h>
 #include <sys/taskq.h>

@@ -126,13 +111,21 @@
         PERI_IPL_5,
         PERI_IPL_6,
         PERI_IPL_7,
         PERI_IPL_8,
         PERI_IPL_9,
-        PERI_IPL_10,
+        PERI_IPL_10
 } periodic_ipl_t;
 
+static char *
+periodic_handler_symbol(ddi_periodic_impl_t *dpr)
+{
+        ulong_t off;
+
+        return (kobj_getsymname((uintptr_t)dpr->dpr_handler, &off));
+}
+
 /*
  * This function may be called either from a soft interrupt handler
  * (ddi_periodic_softintr), or as a taskq worker function.
  */
 static void

@@ -144,19 +137,21 @@
         /*
          * We must be DISPATCHED, but not yet EXECUTING:
          */
         VERIFY((dpr->dpr_flags & (DPF_DISPATCHED | DPF_EXECUTING)) ==
             DPF_DISPATCHED);
+        VERIFY(dpr->dpr_thread == NULL);
 
         if (!(dpr->dpr_flags & DPF_CANCELLED)) {
                 int level = dpr->dpr_level;
                 uint64_t count = dpr->dpr_fire_count;
                 /*
                  * If we have not yet been cancelled, then
                  * mark us executing:
                  */
                 dpr->dpr_flags |= DPF_EXECUTING;
+                dpr->dpr_thread = curthread;
                 mutex_exit(&dpr->dpr_lock);
 
                 /*
                  * Execute the handler, without holding locks:
                  */

@@ -165,10 +160,11 @@
                 (*dpr->dpr_handler)(dpr->dpr_arg);
                 DTRACE_PROBE4(ddi__periodic__done, void *, dpr->dpr_handler,
                     void *, dpr->dpr_arg, int, level, uint64_t, count);
 
                 mutex_enter(&dpr->dpr_lock);
+                dpr->dpr_thread = NULL;
                 dpr->dpr_fire_count++;
         }
 
         /*
          * We're done with this periodic for now, so release it and

@@ -188,12 +184,12 @@
         mutex_enter(&periodics_lock);
         /*
          * Pull the first scheduled periodic off the queue for this priority
          * level:
          */
-        while ((dpr = list_remove_head(&periodic_softint_queue[level - 1]))
-            != NULL) {
+        while ((dpr = list_remove_head(&periodic_softint_queue[level - 1])) !=
+            NULL) {
                 mutex_exit(&periodics_lock);
                 /*
                  * And execute it:
                  */
                 periodic_execute(dpr);

@@ -244,10 +240,35 @@
          * queues.
          */
         mutex_init(&periodics_lock, NULL, MUTEX_ADAPTIVE, NULL);
 }
 
+void
+ddi_periodic_fini(void)
+{
+        ddi_periodic_impl_t *dpr;
+        /*
+         * Find all periodics that have not yet been unregistered and,
+         * on DEBUG bits, print a warning about this resource leak.
+         */
+        mutex_enter(&periodics_lock);
+        while ((dpr = list_head(&periodics)) != NULL) {
+#ifdef  DEBUG
+                printf("DDI periodic handler not deleted (id=%p, handler=%s)\n",
+                    dpr->dpr_id, periodic_handler_symbol(dpr));
+#endif
+
+                mutex_exit(&periodics_lock);
+                /*
+                 * Delete the periodic ourselves:
+                 */
+                i_untimeout(dpr->dpr_id);
+                mutex_enter(&periodics_lock);
+        }
+        mutex_exit(&periodics_lock);
+}
+
 static void
 periodic_cyclic_handler(void *arg)
 {
         extern void sir_on(int);
         ddi_periodic_impl_t *dpr = arg;

@@ -302,12 +323,15 @@
         /*
          * By now, we should have a periodic that is not busy, and has been
          * cancelled:
          */
         VERIFY(dpr->dpr_flags == DPF_CANCELLED);
+        VERIFY(dpr->dpr_thread == NULL);
 
         id_free(periodic_id_space, dpr->dpr_id);
+        cv_destroy(dpr->dpr_cv);
+        mutex_destroy(dpr->dpr_lock);
         kmem_cache_free(periodic_cache, dpr);
 }
 
 static ddi_periodic_impl_t *
 periodic_create(void)

@@ -321,10 +345,17 @@
         cv_init(&dpr->dpr_cv, NULL, CV_DEFAULT, NULL);
 
         return (dpr);
 }
 
+/*
+ * This function provides the implementation for the ddi_periodic_add(9F)
+ * interface.  It registers a periodic handler and returns an opaque identifier
+ * that can be unregistered via ddi_periodic_delete(9F)/i_untimeout().
+ *
+ * It may be called in user or kernel context, provided cpu_lock is not held.
+ */
 timeout_t
 i_timeout(void (*func)(void *), void *arg, hrtime_t interval, int level)
 {
         cyc_handler_t cyh;
         cyc_time_t cyt;

@@ -340,32 +371,26 @@
         dpr->dpr_level = level;
         dpr->dpr_handler = func;
         dpr->dpr_arg = arg;
 
         /*
-         * The resolution must be finer than or equal to
-         * the requested interval. If it's not, set the resolution
-         * to the interval.
-         * Note. There is a restriction currently. Regardless of the
-         * clock resolution used here, 10ms is set as the timer resolution.
-         * Even on the 1ms resolution timer, the minimum interval is 10ms.
+         * The minimum supported interval between firings of the periodic
+         * handler is 10ms; see ddi_periodic_add(9F) for more details.  If a
+         * shorter interval is requested, round up.
          */
         if (ddi_periodic_resolution > interval) {
-                uintptr_t pc = (uintptr_t)dpr->dpr_handler;
-                ulong_t off;
                 cmn_err(CE_WARN,
                     "The periodic timeout (handler=%s, interval=%lld) "
                     "requests a finer interval than the supported resolution. "
-                    "It rounds up to %lld\n", kobj_getsymname(pc, &off),
+                    "It rounds up to %lld\n", periodic_handler_symbol(dpr),
                     interval, ddi_periodic_resolution);
                 interval = ddi_periodic_resolution;
         }
 
         /*
-         * If the specified interval is already multiples of
-         * the resolution, use it as is. Otherwise, it rounds
-         * up to multiples of the timer resolution.
+         * Ensure that the interval is an even multiple of the base resolution
+         * that is at least as long as the requested interval.
          */
         dpr->dpr_interval = roundup(interval, ddi_periodic_resolution);
 
         /*
          * Create the underlying cyclic:

@@ -372,12 +397,11 @@
          */
         cyh.cyh_func = periodic_cyclic_handler;
         cyh.cyh_arg = dpr;
         cyh.cyh_level = CY_LOCK_LEVEL;
 
-        cyt.cyt_when = roundup(gethrtime() + dpr->dpr_interval,
-            ddi_periodic_resolution);
+        cyt.cyt_when = 0;
         cyt.cyt_interval = dpr->dpr_interval;
 
         mutex_enter(&cpu_lock);
         dpr->dpr_cyclic_id = cyclic_add(&cyh, &cyt);
         mutex_exit(&cpu_lock);

@@ -392,31 +416,16 @@
 
         return ((timeout_t)(uintptr_t)dpr->dpr_id);
 }
 
 /*
- *  void
- *  i_untimeout(timeout_t req)
+ * This function provides the implementation for the ddi_periodic_delete(9F)
+ * interface.  It cancels a periodic handler previously registered through
+ * ddi_periodic_add(9F)/i_timeout().
  *
- *  Overview
- *    i_untimeout() is an internal function canceling the i_timeout()
- *    request previously issued.
- *    This function is used for ddi_periodic_delete(9F).
- *
- *  Argument
- *      req: timeout_t opaque value i_timeout() returned previously.
- *
- *  Return value
- *      Nothing.
- *
- *  Caller's context
- *    i_untimeout() can be called in user, kernel or interrupt context.
- *    It cannot be called in high interrupt context.
- *
- *  Note. This function is used by ddi_periodic_delete(), which cannot
- *  be called in interrupt context. As a result, this function is called
- *  in user or kernel context only in practice.
+ * It may be called in user or kernel context, provided cpu_lock is not held.
+ * It may NOT be called from within a periodic handler.
  */
 void
 i_untimeout(timeout_t id)
 {
         ddi_periodic_impl_t *dpr;

@@ -446,10 +455,18 @@
         /*
          * We should be the only one trying to cancel this periodic:
          */
         VERIFY(!(dpr->dpr_flags & DPF_CANCELLED));
         /*
+         * Removing a periodic from within its own handler function will
+         * cause a deadlock, so panic explicitly.
+         */
+        if (dpr->dpr_thread == curthread) {
+                panic("ddi_periodic_delete(%p) called from its own handler\n",
+                    dpr->dpr_id);
+        }
+        /*
          * Mark the periodic as cancelled:
          */
         dpr->dpr_flags |= DPF_CANCELLED;
         mutex_exit(&dpr->dpr_lock);