From owner-freebsd-dtrace@FreeBSD.ORG Thu Jul 11 02:45:16 2013 Return-Path: Delivered-To: freebsd-dtrace@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [8.8.178.115]) by hub.freebsd.org (Postfix) with ESMTP id 5D867DB9 for ; Thu, 11 Jul 2013 02:45:16 +0000 (UTC) (envelope-from markjdb@gmail.com) Received: from mail-oa0-x234.google.com (mail-oa0-x234.google.com [IPv6:2607:f8b0:4003:c02::234]) by mx1.freebsd.org (Postfix) with ESMTP id 2C12D1AF6 for ; Thu, 11 Jul 2013 02:45:16 +0000 (UTC) Received: by mail-oa0-f52.google.com with SMTP id g12so10439779oah.39 for ; Wed, 10 Jul 2013 19:45:15 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20120113; h=sender:date:from:to:subject:message-id:references:mime-version :content-type:content-disposition:in-reply-to:user-agent; bh=xQEWcsX+fXqCel9qTHMioInziQh8K0mdqqvLjGfFMF8=; b=x/RXtvnRvzRS0IXb+r4gwSyzTiHufPKzdxP3+9gsHdQ+0lSWWWYXQcq/zEqRHz6bae qTAGQXAuEmXbe1ZW+OUGDUxSdxdiQq4mI56XF9bBxRQY4WcZN8MtUbGfggB35pNQYWn2 Z41Q8iHTaNkzgAbrI/mH9MDIIa/ZDhTR9xH9Y/oi900f3XvkEVXZ6PZm1SGJyzJ2vyKp ct3IGitum5JTIYmRd4E1i79qmhfY7lQ8z0wrZYWqCO3wVz150JFng8VfP009fJEv6ECQ 0qyE0MyvoPQcJsx2i/mB27iOdedfs0monz+K2QStNG7iJUvsFK+3kTOFgwsAqksGc+K4 e7PA== X-Received: by 10.182.19.168 with SMTP id g8mr30481873obe.21.1373510715555; Wed, 10 Jul 2013 19:45:15 -0700 (PDT) Received: from raichu (24-212-218-13.cable.teksavvy.com. [24.212.218.13]) by mx.google.com with ESMTPSA id m11sm48047949oer.4.2013.07.10.19.45.14 for (version=TLSv1 cipher=ECDHE-RSA-RC4-SHA bits=128/128); Wed, 10 Jul 2013 19:45:14 -0700 (PDT) Sender: Mark Johnston Date: Wed, 10 Jul 2013 22:45:08 -0400 From: Mark Johnston To: freebsd-dtrace@freebsd.org Subject: Re: [RFC] reworking FreeBSD's SDT implementation Message-ID: <20130711024500.GA67976@raichu> References: <20130703041023.GA82673@raichu> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20130703041023.GA82673@raichu> User-Agent: Mutt/1.5.21 (2010-09-15) X-BeenThere: freebsd-dtrace@freebsd.org X-Mailman-Version: 2.1.14 Precedence: list List-Id: "A discussion list for developers working on DTrace in FreeBSD." List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 11 Jul 2013 02:45:16 -0000 On Wed, Jul 03, 2013 at 12:10:23AM -0400, Mark Johnston wrote: > Hello, > > There are a few problems with the way SDT is currently implemented in > FreeBSD. First, the DTrace framework isn't notified when modules are > unloaded, so any probes created by these modules are never destroyed > (this problem isn't specific to SDT though, FBT probes have the same > problem). Second, there is currently nothing preventing one from > unloading a module while some of its SDT probes are enabled; doing this > will generally cause a panic. Finally, providers are "tied" to modules > in the sense that dtrace_unregister() is called on each provider > declared in a module when that module is unloaded. This is inflexible - > probes already have a "module" field to indicate which module they're > defined in, and it would restrict the implementation of, say, a > hypothetical GEOM or netgraph provider, which would probably contain > some common probes for each GEOM or netgraph module. Plus a panic will > occur if a probe from one module is enabled and a second module > declaring the provider of the probe is unloaded. > > I have a patch at [1] which tries to solve all of these problems. It > more or less completely reworks FreeBSD's SDT implementation (currently > contained in kern/kern_sdt.c and cddl/dev/sdt/sdt.c) and changes a > number of things: > > * It adds a pair of hooks into the kernel linker so that > dtrace_module_{loaded,unloaded}() are called when modules are loaded > and unloaded. This is what's currently done in Solaris/illumos and it > makes it possible to ensure that probes are destroyed when a module is > unloaded. This is done by going over all the probes whose module name > is that of the module being unloaded and destroying them. > Unfortunately there are several SDT providers in FreeBSD that actually > (incorrectly) set the module name of their probes; the sctp, nfscl, > vfs and linuxlator providers do this. In practice this will turn out > to be ok since these providers aren't declared across multiple > modules - with my patch, an SDT provider is unregistered when the last > module that refers to it is unloaded. > > * Instead of using SYSINIT/SYSUNINIT to register probes and providers as > modules are loaded/unloaded, the SDT macros will append provider/probe > data to dedicated linker sets in the module (or the kernel). This > allows everything to be done lazily: when sdt.ko is loaded, it can > just iterate over all the probe linker sets and created probes as > needed. This allows SDT to tie a given probe to a module and makes it > possible to solve the second problem described above, since the probe > can now keep a pointer to its linker_file_t. > > * Moves the entire SDT implementation (modulo a stub) into sdt.ko, > instead of having sdt.ko interacting with some in-kernel SDT code via > a set of callbacks. The new SDT implementation is pretty simple; it > maintains a global list of SDT providers, each of which keeps a list > of its probes (which are all just elements of some module's SDT probe > linker set). It has separate hooks for the kernel linker since it > needs to be able to call dtrace_register() when a module is loaded; > the DTrace framework doesn't allow provider methods to register new > providers. I have some ideas on how to simplify the new SDT > implementation even further, but what I have now is working fine for > me. > > * Removes references to mod_lock; our DTrace port defines its own > mod_lock which has nothing to do with the kernel linker lock, so it's > not doing anything useful. Moreover, by having the kernel linker hook > call into DTrace with the linker lock held, I've changed the lock > ordering rule: in illumos, code is supposed to acquire the provider > lock before mod_lock. Hello, The final draft of my patch is here: http://people.freebsd.org/~markj/patches/sdt-module-info/20130710-sdt-module-info.diff I've committed the mod_lock stuff and man page separately. If no one has any objections, I'll commit the patch sometime this weekend. Review and comments are still very welcome! Thanks, -Mark From owner-freebsd-dtrace@FreeBSD.ORG Thu Jul 11 21:02:21 2013 Return-Path: Delivered-To: freebsd-dtrace@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 84F4870B; Thu, 11 Jul 2013 21:02:21 +0000 (UTC) (envelope-from to.my.trociny@gmail.com) Received: from mail-ee0-x235.google.com (mail-ee0-x235.google.com [IPv6:2a00:1450:4013:c00::235]) by mx1.freebsd.org (Postfix) with ESMTP id D5A8B15EC; Thu, 11 Jul 2013 21:02:20 +0000 (UTC) Received: by mail-ee0-f53.google.com with SMTP id c41so5858433eek.40 for ; Thu, 11 Jul 2013 14:02:19 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20120113; h=sender:date:from:to:cc:subject:message-id:references:mime-version :content-type:content-disposition:in-reply-to:user-agent; bh=jV7IwiR+GrfGLCAUNGj/V6RI/MP1wM7RUtoPMc918Ik=; b=L6WX8JztxxLfR0ZTdhPDbjaKKYeRbcFavkGf7CcNXtXzyvyf44kzovQVl+x3ihtCBJ MDxBbo9pWcHtR9uE0seJ8A2O/kx3iV85lKZFt1cZKr7SxBOkmAsQ4Gj2Ds6k9pGUTLbJ W0rKCFxiv0lTyeSvwoJ25RCcU6wFRR5/iCrFcehvCgAAphfvlvX7jfu1clZQiECI7KPN Lb3s1nM10tENhjiLnt+7FqO7b3MHajhfhmi3xQMmqTIf22T0uGDZYtsAHgt29xH8u1NY f2ni3ISsFRL/YMzRdnOTlKPRETLGzmz4LbnBM3ld4Y2VlhHkdXzltcULgKKaQltTpTJn 4cCA== X-Received: by 10.14.126.72 with SMTP id a48mr43189454eei.34.1373576539657; Thu, 11 Jul 2013 14:02:19 -0700 (PDT) Received: from localhost ([178.150.115.244]) by mx.google.com with ESMTPSA id a4sm72357715eez.0.2013.07.11.14.02.17 for (version=TLSv1.2 cipher=RC4-SHA bits=128/128); Thu, 11 Jul 2013 14:02:18 -0700 (PDT) Sender: Mikolaj Golub Date: Fri, 12 Jul 2013 00:02:16 +0300 From: Mikolaj Golub To: Mark Johnston Subject: Re: [RFC] reworking FreeBSD's SDT implementation Message-ID: <20130711210215.GB7506@gmail.com> References: <20130703041023.GA82673@raichu> <20130711024500.GA67976@raichu> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20130711024500.GA67976@raichu> User-Agent: Mutt/1.5.21 (2010-09-15) Cc: freebsd-dtrace@freebsd.org X-BeenThere: freebsd-dtrace@freebsd.org X-Mailman-Version: 2.1.14 Precedence: list List-Id: "A discussion list for developers working on DTrace in FreeBSD." List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 11 Jul 2013 21:02:21 -0000 Hi Mark, On Wed, Jul 10, 2013 at 10:45:08PM -0400, Mark Johnston wrote: > The final draft of my patch is here: > http://people.freebsd.org/~markj/patches/sdt-module-info/20130710-sdt-module-info.diff > > I've committed the mod_lock stuff and man page separately. If no one has > any objections, I'll commit the patch sometime this weekend. > Review and comments are still very welcome! One thing noticed. I don't think '#if defined(sun)' directives in dtrace.c are correct now: > -#if defined(sun) > /* > * DTrace Hook Functions > */ > static void > -dtrace_module_loaded(modctl_t *ctl) > +dtrace_module_loaded(struct linker_file *lf) ctl is unconditionally removed > { > dtrace_provider_t *prv; > > @@ -15166,14 +15150,16 @@ dtrace_module_loaded(modctl_t *ctl) > mutex_enter(&mod_lock); > #endif > > +#if defined(sun) > ASSERT(ctl->mod_busy); > +#endif while here it is used if sun is defined. > > /* > * We're going to call each providers per-module provide operation > * specifying only this module. > */ > for (prv = dtrace_provider; prv != NULL; prv = prv->dtpv_next) > - prv->dtpv_pops.dtps_provide_module(prv->dtpv_arg, ctl); > + prv->dtpv_pops.dtps_provide_module(prv->dtpv_arg, lf); And this looks wrong for sun case. The same issues are in dtrace_module_unloaded(). -- Mikolaj Golub From owner-freebsd-dtrace@FreeBSD.ORG Sat Jul 13 23:42:07 2013 Return-Path: Delivered-To: freebsd-dtrace@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [8.8.178.115]) by hub.freebsd.org (Postfix) with ESMTP id 47C97774; Sat, 13 Jul 2013 23:42:07 +0000 (UTC) (envelope-from markjdb@gmail.com) Received: from mail-ie0-x22a.google.com (mail-ie0-x22a.google.com [IPv6:2607:f8b0:4001:c03::22a]) by mx1.freebsd.org (Postfix) with ESMTP id 0C8E21363; Sat, 13 Jul 2013 23:42:07 +0000 (UTC) Received: by mail-ie0-f170.google.com with SMTP id e11so23858944iej.29 for ; Sat, 13 Jul 2013 16:42:06 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20120113; h=sender:date:from:to:cc:subject:message-id:references:mime-version :content-type:content-disposition:in-reply-to:user-agent; bh=b8RF7NxAVZWNFeTv8ndEKGmNLaaKmffQ2JSvhdWVDWA=; b=X9W1ehyp87pSxrZpt0Y7TeYEuyuQ/25moeL5jlxV8jbh3xyylYm5PqtXo2LQ00n0Xh Ggrj1xkFo4h21xhY/NpyUKTCBvf3IJhksze1SGEDiKvDY810YwkpT55HugSajJrHQW4J g/EoDjphMn51A1Q2b9cLvVbRnDSLDeJDNUxsli/1YNpN6GKbNqkYlbU54MzvOO7tWWAi eDCNjt9exQYZPhH8ovrtsAeezsOUdAX1N6rQRoDGhdoxnTVYrYXa5NVlNAwSC+AiSWwe 8+8JB9qC5B9poIo0vwkwa3Xi7k8jUe/7Vw94MP60UtOjJ50QWqXn9rhMckSBm+QUCeMn WlxA== X-Received: by 10.50.134.101 with SMTP id pj5mr3694001igb.31.1373758926801; Sat, 13 Jul 2013 16:42:06 -0700 (PDT) Received: from raichu (24-212-218-13.cable.teksavvy.com. [24.212.218.13]) by mx.google.com with ESMTPSA id z15sm8460996igp.0.2013.07.13.16.42.05 for (version=TLSv1 cipher=ECDHE-RSA-RC4-SHA bits=128/128); Sat, 13 Jul 2013 16:42:05 -0700 (PDT) Sender: Mark Johnston Date: Sat, 13 Jul 2013 19:42:00 -0400 From: Mark Johnston To: Mikolaj Golub Subject: Re: [RFC] reworking FreeBSD's SDT implementation Message-ID: <20130713234200.GA40803@raichu> References: <20130703041023.GA82673@raichu> <20130711024500.GA67976@raichu> <20130711210215.GB7506@gmail.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20130711210215.GB7506@gmail.com> User-Agent: Mutt/1.5.21 (2010-09-15) Cc: freebsd-dtrace@freebsd.org X-BeenThere: freebsd-dtrace@freebsd.org X-Mailman-Version: 2.1.14 Precedence: list List-Id: "A discussion list for developers working on DTrace in FreeBSD." List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Sat, 13 Jul 2013 23:42:07 -0000 On Fri, Jul 12, 2013 at 12:02:16AM +0300, Mikolaj Golub wrote: > Hi Mark, > > On Wed, Jul 10, 2013 at 10:45:08PM -0400, Mark Johnston wrote: > > > The final draft of my patch is here: > > http://people.freebsd.org/~markj/patches/sdt-module-info/20130710-sdt-module-info.diff > > > > I've committed the mod_lock stuff and man page separately. If no one has > > any objections, I'll commit the patch sometime this weekend. > > Review and comments are still very welcome! > > One thing noticed. I don't think '#if defined(sun)' directives in > dtrace.c are correct now: > > > -#if defined(sun) > > /* > > * DTrace Hook Functions > > */ > > static void > > -dtrace_module_loaded(modctl_t *ctl) > > +dtrace_module_loaded(struct linker_file *lf) > > ctl is unconditionally removed > > > { > > dtrace_provider_t *prv; > > > > @@ -15166,14 +15150,16 @@ dtrace_module_loaded(modctl_t *ctl) > > mutex_enter(&mod_lock); > > #endif > > > > +#if defined(sun) > > ASSERT(ctl->mod_busy); > > +#endif > > while here it is used if sun is defined. > > > > > /* > > * We're going to call each providers per-module provide operation > > * specifying only this module. > > */ > > for (prv = dtrace_provider; prv != NULL; prv = prv->dtpv_next) > > - prv->dtpv_pops.dtps_provide_module(prv->dtpv_arg, ctl); > > + prv->dtpv_pops.dtps_provide_module(prv->dtpv_arg, lf); > > And this looks wrong for sun case. > > The same issues are in dtrace_module_unloaded(). Thanks for pointing this out - I've fixed these problems in the following diff: http://people.freebsd.org/~markj/patches/sdt-module-info/20130713-sdt-module-info.diff It turns out that we typedef modctl_t to struct linker_file, so I don't need to have separate cases for defined(sun) and !defined(sun) in a few places. -Mark