Date: Fri, 17 Feb 2017 17:05:54 +1100 (EST) From: Bruce Evans <brde@optusnet.com.au> To: Mark Johnston <markj@freebsd.org> 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 Message-ID: <20170217160016.D1386@besplex.bde.org> In-Reply-To: <201702170327.v1H3RKK4078742@repo.freebsd.org> References: <201702170327.v1H3RKK4078742@repo.freebsd.org>
next in thread | previous in thread | raw e-mail | index | archive | help
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 <sys/dtrace_impl.h> > #include <sys/dtrace_bsd.h> > #include <machine/clock.h> > +#include <machine/cpufunc.h> This was correct. <machine/cpufunc.h> is standard pollution in <sys/systm.h>, and it is a style bug to include it directly, and a bug to not include <sys/systm.h> after <sys/param.h> and before all other headers in kernel .c files. It is pollution to include <machine/cpufunc.h> in any header except <sys/systm.h>. 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 <machine/frame.h> > +#include <machine/psl.h> 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 <vm/pmap.h> > > 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
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20170217160016.D1386>