svn commit: r331369 - head/sys/vm

[Cy Schubert]

In message <20180323150709.H968 at besplex.bde.org>, Bruce Evans writes:
> On Thu, 22 Mar 2018, Jeff Roberson wrote:
>
> > On Thu, 22 Mar 2018, Cy Schubert wrote:
> >
> >> It broke i386 too.
> >
> > I just did
> > TARGET_ARCH=i386 make buildworld
> > TARGET_ARCH=i386 make buildkernel
> >
> > This worked for me?
> >> 
> >> Index: sys/vm/vm_reserv.c
> >> ===================================================================
> >> --- sys/vm/vm_reserv.c	(revision 331399)
> >> +++ sys/vm/vm_reserv.c	(working copy)
> >> @@ -45,8 +45,6 @@
> >> 
> >> #include <sys/param.h>
> >> #include <sys/kernel.h>
> >> -#include <sys/counter.h>
> >> -#include <sys/ktr.h>
> >> #include <sys/lock.h>
> >> #include <sys/malloc.h>
> >> #include <sys/mutex.h>
> >> @@ -55,6 +53,8 @@
> >> #include <sys/sbuf.h>
> >> #include <sys/sysctl.h>
> >> #include <sys/systm.h>
> >> +#include <sys/counter.h>
> >> +#include <sys/ktr.h>
> >> #include <sys/vmmeter.h>
> >> #include <sys/smp.h>
> >> 
> >> This is because sys/i386/include/machine.h uses critical_enter() and
> >> critical_exit() which are defined in sys/systm.h.
>
> Wrong fix.  I see you committed this.  Now there are more bugs to fix.
>
> <sys/systm.h> is a prerequisite for all kernel headers except
> <sys/param.h>, since it defines and declares things like KASSERT() and
> critical_enter() which might be used in other headers (except
> sys/param.h and its standard pollution).  Sometimes sys/systm.h is
> included as undocumented namespace pollution in headers that are
> accidentally included before the (other) ones that use KASSERT(), etc.
> The headers that have this bug have it to work around bugs in .c files
> like the one above.  It is more usual to have this bug by not including
> sys/systm.h at all than to have it by including it in a wrong order.
> Sorting it alphabetically almost always gives a wrong order.  It must
> be included after sys/param.h and that must be included first.

Agreed on alphabetic sorting.

>
> It is a related bug to include only sys/types.h and not sys/param.h.
> This requires chumminess with the current implementation and all
> future implementations.  sys/param.h provides certain undocumented
> but standard namespace pollution which might vary with the implementation,
> as necessary to satisfy some of the pollution requirements of all current
> and future implementations of other headers.  (The pollution should be
> monotonically decreasing but it was only that for a few years about 20
> years ago when I worked on fixing it.)  .c files that include sys/types.h
> instead of sys/param.h have do some subset of the includes in sys/param.h.
> Since nothing is documented and the subset might depend on the arch and
> user options, it is hard to know the minimal subset.

That's not the case here. sys/types.h is not included in this file but 
point taken.

>
> .c files that include sys/types.h tend to have lots of other #include
> bugs like not including sys/systm.h.  Again it is hard to know the
> minimal replacement for sys/systm.h and its undocumented but standard
> pollution.  It is a style bug to include both sys/types.h and sys/param.h.
> style(9) even explicitly forbids including both.  It is a larger style
> bug to include the standard pollution in sys/systm.h direction.  This
> includes especially <machine/atomic.h> and <machine/cpufunc.h>.  These
> should be considered as being implemented in sys/systm.h, with the
> <machine> headers for them only and implementation detail.  Similarly
> for <sys/libkern.h>.
>
> >> It built nicely on my amd64's though.
>
> amd64 apparently has more namespace pollution which breaks detection
> of the bug.  But I couldn't find where it is.  sys/systm.h isn't included
> nested in any amd64 or x86 headers.  Apparently some amd64 option gives
> it.

The reason is amd64 doesn't use critical_enter() and critical_exit() 
because counter_enter() and counter_exit() are NOPs. The reason they 
are NOPs in amd64 and not in i386 is not all i386 processors support 
cmpxchg8b. It is only then that the critical_*() functions are called.

>
> Bruce

I can create a phabricator revision to clean this instance up and move 
sys/systm.h just after sys/param.h. I'm just about to head out of town 
so I'll create it after I get back, after April 4.

Thank you for your input Bruce.


-- 
Cheers,
Cy Schubert <Cy.Schubert at cschubert.com>
FreeBSD UNIX:  <cy at FreeBSD.org>   Web:  http://www.FreeBSD.org

	The need of the many outweighs the greed of the few.