svn commit: r228433 - in head/sys: kern security/mac

[Bruce Evans]

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