style(9) rules for nested includes

Bruce Evans brde at optusnet.com.au
Fri Mar 11 06:38:31 UTC 2011


On Thu, 10 Mar 2011, Warner Losh wrote:

> On 03/10/2011 10:17, mdf at freebsd.org wrote:
>> I recall a recent discussion/PR about nested includes in the context
>> of<sys/linker_set.h>  and<sys/queue.h>  being a few of the only ones
>> allowed.  However, I don't see anything in style(9) about this.

This is not really a style matter, but a policy for avoiding namespace
pollution.

>> One rule for nested includes I've heard (that FreeBSD doesn't use, but
>> just mentioning it for variety) is that every header file should
>> compile on its own; that is, a .c file with nothing but #include
>> <header.h>  should compile without errors.  This rule also makes some
>> sense with the FreeBSD style convention of alphabetizing includes;
>> otherwise it's kinda just happenstance that e.g.<sys/lock.h>  comes
>> alphabetically before<sys/sx.h>,<sys/mutex.h>,<sys/rwlock.h>  and
>> <sys/rmlock.h>, and barely before<sys/lockmgr.h>.
>
> We've explicitly not done that to date.  In the past, this would cause extra 
> I/O and processing that would slow down compile times by a measurable amount. 
> These days, I think you'd be hard pressed to even measure the effect.

Actually, the effect is relatively larger, since i/o is relatively slower.
It is easiest to see on nfs file systems since close-to-open consistency
makes open() a very slow operation even if the file is cached, since it
requires an RPC which takes many tens or hundreds or thousands of
microseconds.  FreeBSD GENERIC kernels now have over 100 thousands includes
(up from 10-20 thousand in ~1998 when I stopped trying to keep up with header
pollution after having reduced the number of includes by several thousands).
On ref9-i386 now, the ping latency is ~130 usec (not bad), and this results
in "make depend" on a kernel with 93128 words in .depend (I use this word
count as an approximation to the number of includes) taking
"45.91 real 19.33 user 8.31 sys" from cold and
"37.46 real 18.95 user 8.91 sys" from warm.  My reference FreeBSD-4 kernel
with a similar config on a slightly slower CPU but not on nfs takes
" 5.65 real  4.88 user 0.75 sys" from warm.  It is helped by having only
24319 words in .depend and other missing bloat.  The nfs latency accounts
for about half of the extra real time in -current.  It can be ameliorated
by building in parallel with many more threads than CPUS so that not all
threads stall on the same RPCs at the same time (usually), but a single
"make depend" is mostly serial.

> However, if we adopted this rule, you tend to get n^2 behavior because each 
> layer of the onion redundantly would be including the inner onion...

This can be reduced by flattening the include hierarchy, but that is harder.

> A survey of the include files shows uneven application of rules.  Some 
> require additional includes before them (the most common being sys/param.h or 
> sys/types.h), some include some prereques, etc.  There's no hard and fast 
> rule that will describe the current state of affairs.

sys/param.h is almost required in the kernel.  The other examples, even
ones that carefully arrange to use only sys/types.h, can be considered as
style bugs.  Using only sys/types.h is actually not very careful, since
all kernel headers are permitted to depend on sys/param.h (and maybe
sys/systm.h, for KASSERT()); whether they actually do is a function of
time which cannot reasonably be tracked in .c files wanting to include
only <sys/types.h>.

>> Now we come to the reason I ask.  I'm working on a patch to change the
>> static sysctl code to use the standard SYSININT/SYSUNINIT code rather
>> than have special treatment in kern_linker.c, but to do this I need to
>> either change quite a few places that include<sys/sysctl.h>, or
>> include<sys/kernel.h>  instead of<sys/linker_set.h>  in sysctl.h, as
>> the SI_SUB_SYSCTLS value isn't visible otherwise.
>> 
>> This, though, shows a bug in<sys/kernel.h>, where it uses the
>> constant MAXPATHLEN in the extern declaration of kernelname[] (which
>> itself I think is not standard C, IIRC, but there are several uses of
>> sizeof(kernelname) that don't work otherwise).  So then<sys/kernel.h>
>> needs<sys/param.h>  to build.

It just depends on the standard pollution from <sys/param.h>.  (MAXPATHLEN
is acually a system parameter, so it is not even pollution in <sys/param.h>
itself).  This allows <sys/kernel.h> to provide a complete declaration
for `kernelname', so that kernelname can be used without worrying about
what to include to what it is.

> It is cascade problems like this which both militate for the change and 
> against it.  For the change because it causes less churn (in theory). 
> Against the change because you tend to accumulate lots of things that used to 
> be required, but are no longer, but which consumers come to bogusly depend 
> on, making it hard to detangle the thicket.

Wanting to detangle this is the main reason that I don't like nested includes.
There should be no new ones.  No one would repeat the mistakes (pollution
and nested includes) made in old headers :-).

> An interesting number of include files assume that param.h has already been 
> included, and avoids the extra parsing overhead that skipping param.h N times 
> would cause.

The parsing overhead seems to be small relative to i/o.  I just checked
that gcc doesn't repeat the opens for nested includes.  (I actually
checked 2 non-nested includes of <stdio.h> using ktrace.  There was only
1 NAMI with "stdio.h".)  I knew that gcc is smart about this.  Even with
a multiply-included header "foo.h" that acts differently for each include,
there is only 1 NAMI.  gcc apparently caches the whole header.

> It likely is better to fix the 6 places in the kernel where sizeof is used 
> than to redundantly include param.h.  Also, while you're there, fix the 
> strncpy to a strlcpy:

I don't like this much (see another reply).

NGROUPS in ucred.h and user.h used to have a similar but more interesting
(large) problem because the APIs using it are exported to userland.
This went away when NGROUPS was replaced by the old NGROUPS in these
old APIs.  It is now hard-coded as 16 in both, but spelled XU_NGROUPS
and KINGROUPS.  But the existence of these macros is a related mistake --
someone might use them instead of sizeof() on the array.

Bruce


More information about the freebsd-arch mailing list