From owner-svn-src-head@freebsd.org Fri Feb 17 06:34:49 2017 Return-Path: Delivered-To: svn-src-head@mailman.ysv.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:1900:2254:206a::19:1]) by mailman.ysv.freebsd.org (Postfix) with ESMTP id A78B9CE1CE1; Fri, 17 Feb 2017 06:34:49 +0000 (UTC) (envelope-from brde@optusnet.com.au) Received: from mail105.syd.optusnet.com.au (mail105.syd.optusnet.com.au [211.29.132.249]) by mx1.freebsd.org (Postfix) with ESMTP id 63CC312ED; Fri, 17 Feb 2017 06:34:48 +0000 (UTC) (envelope-from brde@optusnet.com.au) Received: from besplex.bde.org (c122-106-153-191.carlnfd1.nsw.optusnet.com.au [122.106.153.191]) by mail105.syd.optusnet.com.au (Postfix) with ESMTPS id 8D78010452F1; Fri, 17 Feb 2017 17:05:55 +1100 (AEDT) Date: Fri, 17 Feb 2017 17:05:54 +1100 (EST) From: Bruce Evans X-X-Sender: bde@besplex.bde.org To: Mark Johnston cc: src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-head@freebsd.org Subject: Re: svn commit: r313850 - in head/sys/cddl/dev/dtrace: amd64 i386 In-Reply-To: <201702170327.v1H3RKK4078742@repo.freebsd.org> Message-ID: <20170217160016.D1386@besplex.bde.org> References: <201702170327.v1H3RKK4078742@repo.freebsd.org> MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII; format=flowed X-Optus-CM-Score: 0 X-Optus-CM-Analysis: v=2.2 cv=Ca543Pjl c=1 sm=1 tr=0 a=Tj3pCpwHnMupdyZSltBt7Q==:117 a=Tj3pCpwHnMupdyZSltBt7Q==:17 a=kj9zAlcOel0A:10 a=ZDuaA4-kceXO56LHDsIA:9 a=CjuIK1q_8ugA:10 X-BeenThere: svn-src-head@freebsd.org X-Mailman-Version: 2.1.23 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: Fri, 17 Feb 2017 06:34:49 -0000 On Fri, 17 Feb 2017, Mark Johnston wrote: > Log: > Directly include needed headers rather than relying on pollution. > > We get machine/cpu.h via kmem.h -> proc.h -> _vm_domain.h -> seq.h. machine/cpu.h is not added here. Do you mean machine/cpufnc.h? > Modified: head/sys/cddl/dev/dtrace/amd64/dtrace_subr.c > ============================================================================== > --- head/sys/cddl/dev/dtrace/amd64/dtrace_subr.c Fri Feb 17 00:50:00 2017 (r313849) > +++ head/sys/cddl/dev/dtrace/amd64/dtrace_subr.c Fri Feb 17 03:27:20 2017 (r313850) > @@ -41,7 +41,9 @@ > #include > #include > #include > +#include This was correct. is standard pollution in , and it is a style bug to include it directly, and a bug to not include after and before all other headers in kernel .c files. It is pollution to include in any header except . kmem.h has grosser pollution (param, proc, malloc, vmem, vm/uma, vm/vm, vm/vm_extern, but not systm which is a prerequisite for most of the other headers that kmem.h includes). Most of the other headers also have gross pollution, ending with seq.h which includes systm.h and its standard pollution. seq.h also includes machine/cpu.h. Apparently kmem.h does depend on this, and this commit doesn't fix it. seq.h also has a nonsense include of sys/types.h after sys/systm.h. sys/types.h is a prerequisite for sys/systm.h, and is apparently usually suppied for seq.h in the usual way by including sys/param.h before all other headers in kernel files. seq.h is also polluted with sys/lock.h. It apparently only does this to get MPASS() defined. MPASS() has nothing to do with the locking in lock.h, and isn't even used there. It is just a trivial wrapper for KASSERT and probably belongs next to the definition of KASSERT() in systm.h. This wrapper mostly subtracts value by adding "Assertion... failed" and __FILE__ and __LINE__ to KASSERT(). That is, it makes the output format more like user assert(). But KASSERT() intentionally doesn't use this format, since is too generic. MPASS() is easier to use, and I suspect most uses of it are because of this and not because it is any good. In kern, there are 1214 KASSERT()s and 346 MPASS(), with MPASS(). > #include > +#include This include is not mentioned in the log message. machine/psl.h is stamdard pollution in machine/cpu.h (on at least amd64 and i386), so would not be needed here if machine/cpu.h had been spelled correctly. > #include > > extern void dtrace_getnanotime(struct timespec *tsp); The timestamping seems over-engineered, but it has a chance of working, unlike KTR timestamps which just used unscaled get_cyclecount() whatever that is (it is raw TSC on most x86, and KTR doesn't even record its current frequency to give the user a chance of scaling). Bruce