From owner-svn-src-all@freebsd.org Fri Mar 23 06:15:36 2018 Return-Path: Delivered-To: svn-src-all@mailman.ysv.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2610:1c1:1:606c::19:1]) by mailman.ysv.freebsd.org (Postfix) with ESMTP id C4FACF4A49E; Fri, 23 Mar 2018 06:15:36 +0000 (UTC) (envelope-from cy.schubert@cschubert.com) Received: from smtp-out-no.shaw.ca (smtp-out-no.shaw.ca [64.59.134.9]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (Client CN "Client", Issuer "CA" (not verified)) by mx1.freebsd.org (Postfix) with ESMTPS id 1793E8292E; Fri, 23 Mar 2018 06:15:35 +0000 (UTC) (envelope-from cy.schubert@cschubert.com) Received: from spqr.komquats.com ([96.50.22.10]) by shaw.ca with ESMTPA id zFz9ehJb4U5pnzFzAemn2j; Fri, 23 Mar 2018 00:15:34 -0600 X-Authority-Analysis: v=2.3 cv=Tai4SyYh c=1 sm=1 tr=0 a=jvE2nwUzI0ECrNeyr98KWA==:117 a=jvE2nwUzI0ECrNeyr98KWA==:17 a=kj9zAlcOel0A:10 a=v2DPQv5-lfwA:10 a=JzwRw_2MAAAA:8 a=YxBL1-UpAAAA:8 a=6I5d2MoRAAAA:8 a=KOz5PoXyROM3pLaX7PQA:9 a=CjuIK1q_8ugA:10 a=_bBvcJC8wCc67rcU61zu:22 a=Ia-lj3WSrqcvXOmTRaiG:22 a=IjZwj45LgO3ly-622nXo:22 Received: from slippy.cwsent.com (slippy [10.1.1.91]) by spqr.komquats.com (Postfix) with ESMTPS id 5682298; Thu, 22 Mar 2018 23:15:31 -0700 (PDT) Received: from slippy.cwsent.com (localhost [127.0.0.1]) by slippy.cwsent.com (8.15.2/8.15.2) with ESMTP id w2N6FUOW040644; Thu, 22 Mar 2018 23:15:30 -0700 (PDT) (envelope-from Cy.Schubert@cschubert.com) Received: from slippy (cy@localhost) by slippy.cwsent.com (8.15.2/8.15.2/Submit) with ESMTP id w2N6FTMJ040628; Thu, 22 Mar 2018 23:15:29 -0700 (PDT) (envelope-from Cy.Schubert@cschubert.com) Message-Id: <201803230615.w2N6FTMJ040628@slippy.cwsent.com> X-Authentication-Warning: slippy.cwsent.com: cy owned process doing -bs X-Mailer: exmh version 2.8.0 04/21/2012 with nmh-1.7.1 Reply-to: Cy Schubert From: Cy Schubert X-os: FreeBSD X-Sender: cy@cwsent.com X-URL: http://www.cschubert.com/ To: Bruce Evans cc: Jeff Roberson , Cy Schubert , Justin Hibbits , Jeff Roberson , src-committers , svn-src-all@freebsd.org, svn-src-head@freebsd.org Subject: Re: svn commit: r331369 - head/sys/vm In-Reply-To: Message from Bruce Evans of "Fri, 23 Mar 2018 15:38:08 +1100." <20180323150709.H968@besplex.bde.org> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Date: Thu, 22 Mar 2018 23:15:29 -0700 X-CMAE-Envelope: MS4wfJs+r7omdWf+Otrml2Q8nLd1Np0GcJ6obUh1atf5U4xHOZ4O1fxijDwi5YJHtTtHfKdaT1R1dg7YwTOCpGLVw49kjLRNw3uI3p37w7QVWi/j1qWszdHN x/MqikPHVA19WLRXfMi/pFY3nVkq/z00F7oc6WS2VjALHYIv20+3MEkxBIuljnooc8+8wsu+GetHnDHbLsTSdJo88o9DfMV7RfyvbvoCxKXZ9F6uDKZZGs98 6pEyS5hGyDEa+/JxKhDRhiqK+qEgrINjoujrzoVplowq0z1kgQ5J5VZmKkmtsLKKVLa8JdkfyDu4SkeOkS1r62LShygpDPDpKFlGui0871ffBmKM4i3mDaU7 d8S21n7oBg4IezwbxLQatJxwoG2v1w== X-BeenThere: svn-src-all@freebsd.org X-Mailman-Version: 2.1.25 Precedence: list List-Id: "SVN commit messages for the entire src tree \(except for " user" and " projects" \)" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 23 Mar 2018 06:15:37 -0000 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 > >> #include > >> -#include > >> -#include > >> #include > >> #include > >> #include > >> @@ -55,6 +53,8 @@ > >> #include > >> #include > >> #include > >> +#include > >> +#include > >> #include > >> #include > >> > >> 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. > > is a prerequisite for all kernel headers except > , 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 and . These > should be considered as being implemented in sys/systm.h, with the > headers for them only and implementation detail. Similarly > for . > > >> 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 FreeBSD UNIX: Web: http://www.FreeBSD.org The need of the many outweighs the greed of the few.