Skip site navigation (1)Skip section navigation (2)
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>