From owner-svn-src-head@FreeBSD.ORG Wed Jul 3 17:28:25 2013 Return-Path: Delivered-To: svn-src-head@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:1900:2254:206a::19:1]) by hub.freebsd.org (Postfix) with ESMTP id 758F0F03; Wed, 3 Jul 2013 17:28:25 +0000 (UTC) (envelope-from asomers@gmail.com) Received: from mail-qc0-x22d.google.com (mail-qc0-x22d.google.com [IPv6:2607:f8b0:400d:c01::22d]) by mx1.freebsd.org (Postfix) with ESMTP id EA95B1A2D; Wed, 3 Jul 2013 17:28:24 +0000 (UTC) Received: by mail-qc0-f173.google.com with SMTP id l10so258832qcy.4 for ; Wed, 03 Jul 2013 10:28:24 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20120113; h=mime-version:sender:in-reply-to:references:date :x-google-sender-auth:message-id:subject:from:to:cc:content-type; bh=ZnbpXzS1bnwhbOvXWq+O/LqEVMOQILV8yng0GzexH9g=; b=RubOx8MGo2TJ21yTDI+d86n/Hx4wRo7lzZV7POIqgTE6v06OiMrEkS4Xhx3Yg3Whc7 Rjx99NDX6EUunHI9/iAtfyqMS05NleaiAxKz9TbsFLDx+pqnEDGj+t/RVjbWN+3qYY6B gvTMVob0WmXM/VEtbn8J/ZlzvCRS3XvH5Rhit4UV32dyB/+MlAHc7U0ucpKEmd8tjtU/ 21Fbomka/d4muImW7DYPJz3ekuH12hzoOHLLy0KSL6ggf96WNrUCz3pB3HwSPl69wLDU 06vQHKwyFF+SnqvD0tJ48oekpEM8HsZxm9cR+gZpXyzRiDhmqSbO1FTnlqXmB4Lt5I6R aD+A== MIME-Version: 1.0 X-Received: by 10.224.8.130 with SMTP id h2mr4419754qah.9.1372872504437; Wed, 03 Jul 2013 10:28:24 -0700 (PDT) Sender: asomers@gmail.com Received: by 10.49.37.226 with HTTP; Wed, 3 Jul 2013 10:28:24 -0700 (PDT) In-Reply-To: <201305240329.r4O3TWnU016249@svn.freebsd.org> References: <201305240329.r4O3TWnU016249@svn.freebsd.org> Date: Wed, 3 Jul 2013 11:28:24 -0600 X-Google-Sender-Auth: 2th67sNXrlPVScZx1Wb1lQ5r_J0 Message-ID: Subject: Re: svn commit: r250953 - head/sys/cddl/contrib/opensolaris/uts/common/dtrace From: Alan Somers To: Mark Johnston , Rui Paulo Content-Type: text/plain; charset=ISO-8859-1 Cc: svn-src-head@freebsd.org, svn-src-all@freebsd.org, src-committers@freebsd.org X-BeenThere: svn-src-head@freebsd.org X-Mailman-Version: 2.1.14 Precedence: list List-Id: SVN commit messages for the src tree for head/-current List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 03 Jul 2013 17:28:25 -0000 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 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);