From owner-freebsd-dtrace@FreeBSD.ORG Wed Jul 3 04:10:31 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 96CA435C for ; Wed, 3 Jul 2013 04:10:31 +0000 (UTC) (envelope-from markjdb@gmail.com) Received: from mail-ie0-x234.google.com (mail-ie0-x234.google.com [IPv6:2607:f8b0:4001:c03::234]) by mx1.freebsd.org (Postfix) with ESMTP id 6E9B11A6E for ; Wed, 3 Jul 2013 04:10:31 +0000 (UTC) Received: by mail-ie0-f180.google.com with SMTP id f4so13176096iea.25 for ; Tue, 02 Jul 2013 21:10:31 -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:mime-version:content-type :content-disposition:user-agent; bh=8io6A8aydk8/QaDfk+osFEq5sUjOJI6foM28/DX9uLA=; b=FT0fdH+Eq+hQLVxqiOgrQX7gJAUmb5+jhTW3xzZCgxoyXfq2s+wUug06HwJ2NV0aow WcYb21lq0BDqGHeJdK1ZlQB2f00eYMlni+zd/Uzj/zepi6wldEyc/I1aWqzbJty4a011 EZUNhJt63E2QBt8G4XhtRvqXKjMltI/7k+3LO/LZD6WbewngcsSrF1tkRCeNBs84Sxij xCuurmWpoL+Y5Sxdme2kby7R4pRL8tuaY8LhO9CVtSPWPhHcq80/NeyHCPpZEV9xqpAq 3uOxOBUwFNAKmEK2nDy5aK6JLsXor9DLCNtDrUa6PgIClqZnEoxZRAyNPEpTAO0FsDlm qHeg== X-Received: by 10.42.52.6 with SMTP id h6mr993346icg.5.1372824630926; Tue, 02 Jul 2013 21:10:30 -0700 (PDT) Received: from raichu (24-212-218-13.cable.teksavvy.com. [24.212.218.13]) by mx.google.com with ESMTPSA id ri10sm22221607igc.1.2013.07.02.21.10.28 for (version=TLSv1 cipher=ECDHE-RSA-RC4-SHA bits=128/128); Tue, 02 Jul 2013 21:10:29 -0700 (PDT) Sender: Mark Johnston Date: Wed, 3 Jul 2013 00:10:23 -0400 From: Mark Johnston To: freebsd-dtrace@freebsd.org Subject: [RFC] reworking FreeBSD's SDT implementation Message-ID: <20130703041023.GA82673@raichu> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline 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: Wed, 03 Jul 2013 04:10:31 -0000 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. I'm also writing a man page for the SDT macros so that there's some guidance on how to create SDT probes and providers. There are also some providers (io and nfscl in particular) that are created manually using the code in kern/dtio_kdtrace.c and nfsclient/nfs_kdtrace.c; if no one has any objections, I'd like to convert those providers to use the SDT macros. This will let us delete a bunch of code and would help reduce confusion, since at the moment it looks like there are multiple ways to create new providers, and it's not really clear which way is "correct." I don't yet have a patch for this though. Does anyone have any comments on or objections to my approach? I don't think the patch is ready to commit yet, but it's working well for me as far as actual functionality is concerned. I'm going to be working on this a bit more over the next several days, and any testing or review would be highly appreciated. :) Thanks! -Mark [1] http://people.freebsd.org/~markj/patches/20130702-sdt-module-info.diff