Date: Tue, 13 Dec 2011 00:38:46 +1100 (EST) From: Bruce Evans <brde@optusnet.com.au> To: Andriy Gapon <avg@freebsd.org> Cc: Kostik Belousov <kostikbel@gmail.com>, svn-src-head@freebsd.org, svn-src-all@freebsd.org, src-committers@freebsd.org Subject: Re: svn commit: r228433 - in head/sys: kern security/mac Message-ID: <20111212225057.E2739@besplex.bde.org> In-Reply-To: <4EE5D574.9080303@FreeBSD.org> References: <201112121005.pBCA5Dar093711@svn.freebsd.org> <20111212101558.GK50300@deviant.kiev.zoral.com.ua> <4EE5D574.9080303@FreeBSD.org>
next in thread | previous in thread | raw e-mail | index | archive | help
On Mon, 12 Dec 2011, Andriy Gapon wrote: > on 12/12/2011 12:15 Kostik Belousov said the following: >> On Mon, Dec 12, 2011 at 10:05:13AM +0000, Andriy Gapon wrote: >>> Author: avg >>> Date: Mon Dec 12 10:05:13 2011 >>> New Revision: 228433 >>> URL: http://svn.freebsd.org/changeset/base/228433 >>> >>> Log: >>> put sys/systm.h at its proper place or add it if missing >>> >>> Reported by: lstewart, tinderbox >>> Pointyhat to: avg, attilio >>> MFC after: 1 week >>> MFC with: r228430 >>> >>> Modified: >>> head/sys/kern/kern_sx.c >>> head/sys/kern/vfs_cache.c >>> head/sys/security/mac/mac_framework.c >>> head/sys/security/mac/mac_priv.c >> It means that previously sx.h did not required systm.h and now it does ? >> Might be, you should move SCHEDULER_STOPPED and stop_scheduler declarations >> into sys/lock.h ? > > Strictly speaking it's sys/lockstat.h that now requires systm.h. > I am not an expert in FreeBSD header file organization, so I will just follow > whatever the experts advise. <sys/systm.h> should always be included second (after <sys/param.h>) since it defines interfaces like KASSERT() and now SCHEDULER_STOPPED() that _might_ be used in other headers. KASSERT() was already used in a few critical headers. However, this shouldn't necessary in practice since KASSERT() and SCHEDULER_STOPPED() are implemented as macros. The other headers should also use macros and not use inline functions, so that they don't depend on the include order. The <sys> headers that use SCHEDULER_STOPPED(), namely <sys/lockstat.h> and <sys/mutex.h> already don't use any inline functions, although they use rather bloated macros that give too much inlining in a different way to inline functions. The compilation failures occurred because other headers use inline functions that use the macros that use SCHEDULER_STOPPED(). <sys/sx.h> seems to be the main offender. It uses LOCKSTAT_PROFILE_OBTAIN_LOCK_SUCCESS(). This horribly named interface now uses SCHEDULER_STOPPED(). When lockstat profiling is configured, the expansion in nested macros becomes even larger. I think the SCHEDULER_STOPPED() call should be in lock_profile_obtain_lock_success(), not in the macro that invokes it. The usual excuse for bloated macros and inlines -- that they execute only a small amount of code in the usual case -- applies inversely here, since it is the unusual case of SCHEDULER_STOPPED() that is optimized by testing in the macro to avoid the function call. <sys/sx.h>'s excessive dependencies on other headers can also be seen in its namespace pollution. It includes nested all of the following: - <sys/pcpu.h>: Needed for some reason(?) - <sys/lock_profile.h> <sys/lockstat.h>: These are more reasonable. sx.h uses lock profiling, etc., so it needs the lock profiling interfaces defined somewhere in cases where they are actually referenced, and since sx.h uses inline functions the lock profiling interfaces are _always_ referenced if sx.h is included, even if they are not used because the sx.h inlines that reference them are not referenced. If sx.h used macros instead of inlines, then the lock profiling headers would only be used sometimes, and then including them nested would save having to include them in .c files that don't really know about them. But style rules forbid such nested includes, since they result in almost every header including every other header via transitive closure of creeping pollution, and make it very hard to see the actual dependencies. davidg cleaned up the vm header pollution in 1995. Despite attempts to keep it under control, pollution in other headers is probably 10 times larger than it was then :-(. - <machine/atomic.h>: bogus. This is standard namespace pollution in <sys/systm.h>, and it is a style bug not to depend on this, since it is usually a not just a style bug to not include <sys/systm.h>. This style bug used to be only in mutex.h among <sys> headers. I fixed it there long ago in my version. Now it is also in refcount.h, rwlock.h and sx.h. Some of the other bugs in this area are including <sys/types.h> instead of <sys/param.h> and not including <sys/systm.h> at all. This discussion shows that <sys/systm.h> _may_ be a prerequisite for almost any system header in the kernel (_KERNEL defined). Even if not including <sys/systm.h> at all (or sorted alphabetically) works when it is tested and committed, another header may grow a dependency on <sys/systm.h>, though it shouldn't. Similarly for <sys/types.h>. <sys/param.h> provides lots of standard namespace pollution that _may_ be depended on. It is also hard to test whether minimal headers work in all cases even before the future, due to some things depending on options and the labyrinth include dependencies sometimes being satisifed accidentally by pollution in unrelated headers. <sys/systm.h> may be the wrong place for critical macros like KASSERT(). I've thought of putting it in <sys/param.h> instead, but there is already too much non-parameter and other pollution there. If I knew what "systm" stood for, then I might know what should be put in systm.h. In 4.4BSD, it only declares a few variables and prototypes, and includes <sys/libkern.h> (which declares a few inline functions and prototypes), and defines 3 bogus macros which FreeBSD removed. An implementation from scratch should have only types in <sys/types.h>, only parameters in <sys/param.h>, extern declarations prototypes in somewhere like <sys/systm.h>, and general macros and inlines in 1 or 2 other headers. 4.4BSD and FreeBSD also have kernel.h a only small subset of kernel stuff in it. In 4.4BSD. this has a prominent comment saying that it is for global kernel variables. In FreeBSD, it still has this comment, but most of the file is now for SYSINITs and TUNABLEs. It also has intrhook stuff, and namespace pollution for SYSINITs and intrhooks. In FreeBSD-1, all except 1 of the data declarations in systm.h was moved to kernel.h, apparently to give a more logical split of data declarations and prototypes. Most kernel files won' need the data declarations, since globals were uncommon and are less common with SMP. But most kernel files need some general prototypes, for printf() at least. Bruce
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20111212225057.E2739>