Date: Sat, 07 Apr 2018 15:23:11 -0700 From: Cy Schubert <Cy.Schubert@cschubert.com> To: Cy Schubert <Cy.Schubert@cschubert.com> Cc: Bruce Evans <brde@optusnet.com.au>, Jeff Roberson <jroberson@jroberson.net>, Justin Hibbits <jrh29@alumni.cwru.edu>, Jeff Roberson <jeff@freebsd.org>, src-committers <src-committers@freebsd.org>, svn-src-all@freebsd.org, svn-src-head@freebsd.org Subject: Re: svn commit: r331369 - head/sys/vm Message-ID: <201804072223.w37MNBQI009248@slippy.cwsent.com> In-Reply-To: Message from Cy Schubert <Cy.Schubert@cschubert.com> of "Thu, 22 Mar 2018 23:15:29 -0700." <201803230615.w2N6FTMJ040628@slippy.cwsent.com>
next in thread | previous in thread | raw e-mail | index | archive | help
In message <201803230615.w2N6FTMJ040628@slippy.cwsent.com>, Cy Schubert
writes:
> In message <20180323150709.H968@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.
Hi Bruce,
Can you please give this a once over?
diff --git a/sys/vm/vm_reserv.c b/sys/vm/vm_reserv.c
index d8869e3bdbe..6d31d79da39 100644
--- a/sys/vm/vm_reserv.c
+++ b/sys/vm/vm_reserv.c
@@ -44,6 +44,7 @@ __FBSDID("$FreeBSD$");
#include "opt_vm.h"
#include <sys/param.h>
+#include <sys/systm.h>
#include <sys/kernel.h>
#include <sys/lock.h>
#include <sys/malloc.h>
@@ -52,7 +53,6 @@ __FBSDID("$FreeBSD$");
#include <sys/rwlock.h>
#include <sys/sbuf.h>
#include <sys/sysctl.h>
-#include <sys/systm.h>
#include <sys/counter.h>
#include <sys/ktr.h>
#include <sys/vmmeter.h>
--
Cheers,
Cy Schubert <Cy.Schubert@cschubert.com>
FreeBSD UNIX: <cy@FreeBSD.org> Web: http://www.FreeBSD.org
The need of the many outweighs the greed of the few.
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?201804072223.w37MNBQI009248>