Date: Wed, 3 Jul 2013 11:28:24 -0600 From: Alan Somers <asomers@freebsd.org> To: Mark Johnston <markj@freebsd.org>, Rui Paulo <rpaulo@freebsd.org> Cc: svn-src-head@freebsd.org, svn-src-all@freebsd.org, src-committers@freebsd.org Subject: Re: svn commit: r250953 - head/sys/cddl/contrib/opensolaris/uts/common/dtrace Message-ID: <CAOtMX2i2iUoyC92_Xn6jQbrm05=H8AJAvMpgsXGVQOGqhXH4RA@mail.gmail.com> In-Reply-To: <201305240329.r4O3TWnU016249@svn.freebsd.org> References: <201305240329.r4O3TWnU016249@svn.freebsd.org>
next in thread | previous in thread | raw e-mail | index | archive | help
This creates another panic on module unload when WITNESS is enabled,
because the module exits while holding the fasttrap_cleanup_mtx. This
patch fixes the problem. I'm not sure if the mtx_destroy() is
necessary, but I would feel dirty to leave it out. Does this patch
look good to you?
Index: sys/cddl/contrib/opensolaris/uts/common/dtrace/fasttrap.c
===================================================================
--- sys/cddl/contrib/opensolaris/uts/common/dtrace/fasttrap.c (revision 252490)
+++ sys/cddl/contrib/opensolaris/uts/common/dtrace/fasttrap.c (working copy)
@@ -2434,6 +2434,7 @@
wakeup(&fasttrap_cleanup_cv);
mtx_sleep(&fasttrap_cleanup_drain, &fasttrap_cleanup_mtx, 0, "ftcld",
0);
+ mtx_unlock(&fasttrap_cleanup_mtx);
fasttrap_cleanup_proc = NULL;
#ifdef DEBUG
@@ -2473,6 +2474,7 @@
#if !defined(sun)
destroy_dev(fasttrap_cdev);
mutex_destroy(&fasttrap_count_mtx);
+ mtx_destroy(&fasttrap_cleanup_mtx);
CPU_FOREACH(i) {
mutex_destroy(&fasttrap_cpuc_pid_lock[i]);
}
On Thu, May 23, 2013 at 9:29 PM, Mark Johnston <markj@freebsd.org> wrote:
> Author: markj
> Date: Fri May 24 03:29:32 2013
> New Revision: 250953
> URL: http://svnweb.freebsd.org/changeset/base/250953
>
> Log:
> The fasttrap provider cleans up probes asynchronously when a process with
> USDT probes exits. This was previously done with a callout; however, it is
> possible to sleep while holding the DTrace mutexes, so a panic will occur
> on INVARIANTS kernels if the callout handler can't immediately acquire one
> of these mutexes. This panic will be frequently triggered on systems where
> a USDT-enabled program (perl, for instance) is often run.
>
> This revision changes the fasttrap cleanup mechanism so that a dedicated
> thread is used instead of a callout. The old behaviour is otherwise
> preserved.
>
> Reviewed by: rpaulo
> MFC after: 1 month
>
> Modified:
> head/sys/cddl/contrib/opensolaris/uts/common/dtrace/fasttrap.c
>
> Modified: head/sys/cddl/contrib/opensolaris/uts/common/dtrace/fasttrap.c
> ==============================================================================
> --- head/sys/cddl/contrib/opensolaris/uts/common/dtrace/fasttrap.c Fri May 24 02:18:37 2013 (r250952)
> +++ head/sys/cddl/contrib/opensolaris/uts/common/dtrace/fasttrap.c Fri May 24 03:29:32 2013 (r250953)
> @@ -155,9 +155,9 @@ static struct cdevsw fasttrap_cdevsw = {
> static struct cdev *fasttrap_cdev;
> static dtrace_meta_provider_id_t fasttrap_meta_id;
>
> -static struct callout fasttrap_timeout;
> +static struct proc *fasttrap_cleanup_proc;
> static struct mtx fasttrap_cleanup_mtx;
> -static uint_t fasttrap_cleanup_work;
> +static uint_t fasttrap_cleanup_work, fasttrap_cleanup_drain, fasttrap_cleanup_cv;
>
> /*
> * Generation count on modifications to the global tracepoint lookup table.
> @@ -310,8 +310,11 @@ fasttrap_mod_barrier(uint64_t gen)
> }
>
> /*
> - * This is the timeout's callback for cleaning up the providers and their
> - * probes.
> + * This function performs asynchronous cleanup of fasttrap providers. The
> + * Solaris implementation of this mechanism use a timeout that's activated in
> + * fasttrap_pid_cleanup(), but this doesn't work in FreeBSD: one may sleep while
> + * holding the DTrace mutexes, but it is unsafe to sleep in a callout handler.
> + * Thus we use a dedicated process to perform the cleanup when requested.
> */
> /*ARGSUSED*/
> static void
> @@ -322,11 +325,8 @@ fasttrap_pid_cleanup_cb(void *data)
> dtrace_provider_id_t provid;
> int i, later = 0, rval;
>
> - static volatile int in = 0;
> - ASSERT(in == 0);
> - in = 1;
> -
> - while (fasttrap_cleanup_work) {
> + mtx_lock(&fasttrap_cleanup_mtx);
> + while (!fasttrap_cleanup_drain || later > 0) {
> fasttrap_cleanup_work = 0;
> mtx_unlock(&fasttrap_cleanup_mtx);
>
> @@ -397,39 +397,32 @@ fasttrap_pid_cleanup_cb(void *data)
> }
> mutex_exit(&bucket->ftb_mtx);
> }
> -
> mtx_lock(&fasttrap_cleanup_mtx);
> - }
>
> -#if 0
> - ASSERT(fasttrap_timeout != 0);
> -#endif
> + /*
> + * If we were unable to retire a provider, try again after a
> + * second. This situation can occur in certain circumstances
> + * where providers cannot be unregistered even though they have
> + * no probes enabled because of an execution of dtrace -l or
> + * something similar.
> + */
> + if (later > 0 || fasttrap_cleanup_work ||
> + fasttrap_cleanup_drain) {
> + mtx_unlock(&fasttrap_cleanup_mtx);
> + pause("ftclean", hz);
> + mtx_lock(&fasttrap_cleanup_mtx);
> + } else
> + mtx_sleep(&fasttrap_cleanup_cv, &fasttrap_cleanup_mtx,
> + 0, "ftcl", 0);
> + }
>
> /*
> - * If we were unable to remove a retired provider, try again after
> - * a second. This situation can occur in certain circumstances where
> - * providers cannot be unregistered even though they have no probes
> - * enabled because of an execution of dtrace -l or something similar.
> - * If the timeout has been disabled (set to 1 because we're trying
> - * to detach), we set fasttrap_cleanup_work to ensure that we'll
> - * get a chance to do that work if and when the timeout is reenabled
> - * (if detach fails).
> - */
> - if (later > 0) {
> - if (callout_active(&fasttrap_timeout)) {
> - callout_reset(&fasttrap_timeout, hz,
> - &fasttrap_pid_cleanup_cb, NULL);
> - }
> -
> - else if (later > 0)
> - fasttrap_cleanup_work = 1;
> - } else {
> -#if !defined(sun)
> - /* Nothing to be done for FreeBSD */
> -#endif
> - }
> + * Wake up the thread in fasttrap_unload() now that we're done.
> + */
> + wakeup(&fasttrap_cleanup_drain);
> + mtx_unlock(&fasttrap_cleanup_mtx);
>
> - in = 0;
> + kthread_exit();
> }
>
> /*
> @@ -440,8 +433,10 @@ fasttrap_pid_cleanup(void)
> {
>
> mtx_lock(&fasttrap_cleanup_mtx);
> - fasttrap_cleanup_work = 1;
> - callout_reset(&fasttrap_timeout, 1, &fasttrap_pid_cleanup_cb, NULL);
> + if (!fasttrap_cleanup_work) {
> + fasttrap_cleanup_work = 1;
> + wakeup(&fasttrap_cleanup_cv);
> + }
> mtx_unlock(&fasttrap_cleanup_mtx);
> }
>
> @@ -991,7 +986,6 @@ fasttrap_pid_enable(void *arg, dtrace_id
> proc_t *p = NULL;
> int i, rc;
>
> -
> ASSERT(probe != NULL);
> ASSERT(!probe->ftp_enabled);
> ASSERT(id == probe->ftp_id);
> @@ -2272,17 +2266,23 @@ static int
> fasttrap_load(void)
> {
> ulong_t nent;
> - int i;
> + int i, ret;
>
> /* Create the /dev/dtrace/fasttrap entry. */
> fasttrap_cdev = make_dev(&fasttrap_cdevsw, 0, UID_ROOT, GID_WHEEL, 0600,
> "dtrace/fasttrap");
>
> mtx_init(&fasttrap_cleanup_mtx, "fasttrap clean", "dtrace", MTX_DEF);
> - callout_init_mtx(&fasttrap_timeout, &fasttrap_cleanup_mtx, 0);
> mutex_init(&fasttrap_count_mtx, "fasttrap count mtx", MUTEX_DEFAULT,
> NULL);
>
> + ret = kproc_create(fasttrap_pid_cleanup_cb, NULL,
> + &fasttrap_cleanup_proc, 0, 0, "ftcleanup");
> + if (ret != 0) {
> + destroy_dev(fasttrap_cdev);
> + return (ret);
> + }
> +
> /*
> * Install our hooks into fork(2), exec(2), and exit(2).
> */
> @@ -2389,15 +2389,6 @@ fasttrap_unload(void)
> return (-1);
>
> /*
> - * Prevent any new timeouts from running by setting fasttrap_timeout
> - * to a non-zero value, and wait for the current timeout to complete.
> - */
> - mtx_lock(&fasttrap_cleanup_mtx);
> - fasttrap_cleanup_work = 0;
> - callout_drain(&fasttrap_timeout);
> - mtx_unlock(&fasttrap_cleanup_mtx);
> -
> - /*
> * Iterate over all of our providers. If there's still a process
> * that corresponds to that pid, fail to detach.
> */
> @@ -2431,26 +2422,20 @@ fasttrap_unload(void)
> }
>
> if (fail) {
> - uint_t work;
> - /*
> - * If we're failing to detach, we need to unblock timeouts
> - * and start a new timeout if any work has accumulated while
> - * we've been unsuccessfully trying to detach.
> - */
> - mtx_lock(&fasttrap_cleanup_mtx);
> - work = fasttrap_cleanup_work;
> - callout_drain(&fasttrap_timeout);
> - mtx_unlock(&fasttrap_cleanup_mtx);
> -
> - if (work)
> - fasttrap_pid_cleanup();
> -
> (void) dtrace_meta_register("fasttrap", &fasttrap_mops, NULL,
> &fasttrap_meta_id);
>
> return (-1);
> }
>
> + mtx_lock(&fasttrap_cleanup_mtx);
> + fasttrap_cleanup_drain = 1;
> + /* Wait for the cleanup thread to finish up and signal us. */
> + wakeup(&fasttrap_cleanup_cv);
> + mtx_sleep(&fasttrap_cleanup_drain, &fasttrap_cleanup_mtx, 0, "ftcld",
> + 0);
> + fasttrap_cleanup_proc = NULL;
> +
> #ifdef DEBUG
> mutex_enter(&fasttrap_count_mtx);
> ASSERT(fasttrap_pid_count == 0);
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?CAOtMX2i2iUoyC92_Xn6jQbrm05=H8AJAvMpgsXGVQOGqhXH4RA>