From owner-svn-src-head@freebsd.org Sun Nov 20 08:05:20 2016 Return-Path: Delivered-To: svn-src-head@mailman.ysv.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:1900:2254:206a::19:1]) by mailman.ysv.freebsd.org (Postfix) with ESMTP id 3AAF5C4BD64 for ; Sun, 20 Nov 2016 08:05:20 +0000 (UTC) (envelope-from markmi@dsl-only.net) Received: from asp.reflexion.net (outbound-mail-210-31.reflexion.net [208.70.210.31]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (Client did not present a certificate) by mx1.freebsd.org (Postfix) with ESMTPS id EFF741932 for ; Sun, 20 Nov 2016 08:05:19 +0000 (UTC) (envelope-from markmi@dsl-only.net) Received: (qmail 15082 invoked from network); 20 Nov 2016 08:04:59 -0000 Received: from unknown (HELO rtc-sm-01.app.dca.reflexion.local) (10.81.150.1) by 0 (rfx-qmail) with SMTP; 20 Nov 2016 08:04:59 -0000 Received: by rtc-sm-01.app.dca.reflexion.local (Reflexion email security v8.20.0) with SMTP; Sun, 20 Nov 2016 03:04:58 -0500 (EST) Received: (qmail 23055 invoked from network); 20 Nov 2016 08:04:58 -0000 Received: from unknown (HELO iron2.pdx.net) (69.64.224.71) by 0 (rfx-qmail) with (AES256-SHA encrypted) SMTP; 20 Nov 2016 08:04:58 -0000 Received: from [192.168.1.106] (c-76-115-7-162.hsd1.or.comcast.net [76.115.7.162]) by iron2.pdx.net (Postfix) with ESMTPSA id 87588EC8FDE; Sun, 20 Nov 2016 00:05:12 -0800 (PST) Content-Type: text/plain; charset=us-ascii Mime-Version: 1.0 (Mac OS X Mail 10.1 \(3251\)) Subject: Re: svn commit: r308817 - head/sys/powerpc/include [Still have pmap_t and struct pmap ppowerpc64 problems as of -r308860] From: Mark Millard In-Reply-To: Date: Sun, 20 Nov 2016 00:05:11 -0800 Cc: svn-src-head@freebsd.org, FreeBSD Current Content-Transfer-Encoding: quoted-printable Message-Id: <469C6A73-67F3-4CB5-90BD-C8092210CDDD@dsl-only.net> References: <39962D4C-29BA-4AA4-B77D-2344A68FDB54@dsl-only.net> <53258F35-C86E-4DE0-BDF0-5C139E68356D@dsl-only.net> <20161119204715.79632a66@zhabar.knownspace> <3D338DB4-9FAF-46A8-96FF-4F77B01871E2@dsl-only.net> <1EC92166-7DF3-42DC-9AAD-AED1BA9D5CC1@dsl-only.net> <59613CEF-BA2D-4010-996C-6FB8F9D126C6@dsl-only.net> <9CDAF14D-2953-4914-BD3D-C91F591DE7DD@dsl-only.net> To: Justin Hibbits X-Mailer: Apple Mail (2.3251) X-BeenThere: svn-src-head@freebsd.org X-Mailman-Version: 2.1.23 Precedence: list List-Id: SVN commit messages for the src tree for head/-current List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Sun, 20 Nov 2016 08:05:20 -0000 Glad to help. Going in another direction: What of stable/11 ? (And possibly stable/10 = .) Changing the BROOKE kernel ABI in stable/11 is a no-no. Similarly for = AIM. Is there a reason to prefer which context actually works correctly for 11.0 default builds for accessing pm_stats in pmap? My guess is that GENERIC64 and GENERIC for powerpc64 and powerpc should by default work. Needing to have something special for 11.x BOOKE to build correctly in seems less of a problem overall: BOOKE likely has a more tailored 11.x build context anyway and there are no ftp 11.0 snapshots or releases for KERNCONF=3DMPC85XX . TARGET_ARCH=3Dpowerpcspe (KERNCONF=3DMPC85XXSPE) = does not exist until head (12.0-CURRENT) from what I can tell. This might suggest for stable/11 something like: (notation picked for emphasis) #if defined(AIM) || !defined(BOOKE) . . . struct pmap { struct mtx pm_mtx; =20 #ifdef __powerpc64__ struct slbtnode *pm_slb_tree_root; struct slb **pm_slb; int pm_slb_len; #else register_t pm_sr[16]; #endif cpuset_t pm_active; struct pmap *pmap_phys; struct pmap_statistics pm_stats;. . . . . . (the unchanged order above) . . . #elif defined(BOOKE) struct pmap { struct mtx pm_mtx; /* pmap mutex */ tlbtid_t pm_tid[MAXCPU]; /* TID to identify this = pmap entries in TLB */ cpuset_t pm_active; /* active on cpus */ struct pmap_statistics pm_stats; /* pmap statistics */ . . . (the unchanged order above) . . . #endif with some means used for defining BOOKE outside buildkernel for the BOOKE builds, such as for KERNCONF=3DMPC85XX . Then at least GENERIC and GENERIC64 would build correctly for this subject area by default. In particular GENERIC64 actually has no MPC85XX (BOOKE) alternative so far as I know. That makes it too bad that it is always broken for the issue in stable/11 (and in stable/10 ). I choose the above to also cover GENERIC. If only GENERIC64 was to be made to work right by default there are alternatives that involve __powerpc64__ in at least one more place, such as: #if defined(AIM) || defined(__powerpc64__) =3D=3D=3D Mark Millard markmi at dsl-only.net On 2016-Nov-19, at 10:12 PM, Justin Hibbits = wrote: > My buildworld completed successfully, so it's been fixed in = r308873/r308874. >=20 > Thanks for your testing. I often build just kernel, so wouldn't have = seen the fallout until it was far too late. >=20 > - Justin On Nov 19, 2016, at 10:22 PM, Mark Millard wrote: > On 2016-Nov-16, at 8:33 PM, Justin Hibbits = wrote: >=20 >> *sigh* okay, thanks. I just tested, and vm/vm_page.h, and vm/vm.h = can both be removed from memstat_uma.c for it to compile. I'm kicking = off a buildworld myself now, too, and hope to have it ready to commit = tomorrow (takes a couple hours to buildworld on my G5). >>=20 >> - Justin >=20 > That will not be the only potential place: umastat.c in = tools/tools/umastat/ > also has a include of vm/vm_page.h: >=20 >> # find /usr/src/ -name .svn -prune -o -name sys -prune -o -name man = -prune -o -exec grep "vm_page[.]h" {} \; -print | more >> #include >> /usr/src/lib/libmemstat/memstat_uma.c >> #define LIBMEMSTAT /* Cause vm_page.h not to include = opt_vmpage.h */ >> #include >> /usr/src/tools/tools/umastat/umastat.c >=20 >=20 > =3D=3D=3D > Mark Millard > markmi at dsl-only.net >=20 > On Nov 19, 2016, at 9:47 PM, Mark Millard wrote: >=20 >> [Top post of bad news.] >>=20 >> With the patch I get a different incomplete type used in libmemstat: >>=20 >> struct md_page >>=20 >> --- all_subdir_lib/libmemstat --- >> In file included from /usr/src/lib/libmemstat/memstat_uma.c:34:0: >> /usr/src/sys/vm/vm_page.h:144:17: error: field 'md' has incomplete = type >> struct md_page md; /* machine dependent stuff */ >> ^ >> *** [memstat_uma.o] Error code 1 >>=20 >> make[5]: stopped in /usr/src/lib/libmemstat >>=20 >>=20 >> =3D=3D=3D >> Mark Millard >> markmi at dsl-only.net >>=20 >> On 2016-Nov-19, at 7:42 PM, Mark Millard = wrote: >>=20 >>> On 2016-Nov-19, at 7:36 PM, Mark Millard = wrote: >>>=20 >>>> On 2016-Nov-19, at 7:32 PM, Justin Hibbits wrote: >>>>=20 >>>>> Sorry, I generated the diff from a different tree that wasn't = synced to head (had the same change in both trees originally). If that = is the only problem, you can ignore it and try the rest. I can generate = another diff later too. >>>>> - Justin >>>>=20 >>>> Yep: I manually did the move of the pm_stats line and am building. >>>=20 >>> If it builds and I install it on a PowerMac G5 and it boots, what do = I >>> do to test if pm_stats and pm_mtx seems to be working well/right for >>> the out of kernel code? Do you know of a reasonable test? >>>=20 >>> =3D=3D=3D >>> Mark Millard >>> markmi at dsl-only.net >>>=20 >> On Nov 19, 2016 21:27, "Mark Millard" wrote: >>> [Top post about patch issues.] >>>=20 >>> Looking at the patch it seems to be designed for when #else was in = use: >>>=20 >>>> -#else >>>> +#elif defined(BOOKE) >>>=20 >>> but -r308817 already has the 2nd line (BOOKE). Your patch shows: >>>=20 >>>> Index: sys/powerpc/include/pmap.h >>>> =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D >>>> --- sys/powerpc/include/pmap.h (revision 308718) >>>> +++ sys/powerpc/include/pmap.h (working copy) >>>=20 >>> So it looks like you started from before -r308817 . >>>=20 >>> Trying it (I'm at -r308860): >>>=20 >>>> Patching file sys/powerpc/include/pmap.h using Plan A... >>>> Hunk #1 succeeded at 74. >>>> Hunk #2 succeeded at 84. >>>> Hunk #3 succeeded at 132. >>>> Hunk #4 succeeded at 145. >>>> Hunk #5 failed at 180. >>>> Hunk #6 succeeded at 194. >>>> Hunk #7 succeeded at 210. >>>> 1 out of 7 hunks failed--saving rejects to = sys/powerpc/include/pmap.h.rej >>>=20 >>>> # more sys/powerpc/include/pmap.h.rej >>>> @@ -179,13 +180,13 @@ >>>> struct slb **slb_alloc_user_cache(void); >>>> void slb_free_user_cache(struct slb **); >>>>=20 >>>> -#else >>>> +#elif defined(BOOKE) >>>>=20 >>>> struct pmap { >>>> + struct pmap_statistics pm_stats; /* pmap statistics = */ >>>> struct mtx pm_mtx; /* pmap mutex */ >>>> tlbtid_t pm_tid[MAXCPU]; /* TID to identify this = pmap entries in TLB */ >>>> cpuset_t pm_active; /* active on cpus */ >>>> - struct pmap_statistics pm_stats; /* pmap statistics = */ >>>>=20 >>>> /* Page table directory, array of pointers to page tables. */ >>>> pte_t *pm_pdir[PDIR_NENTRIES]; >>>=20 >>>=20 >>> =3D=3D=3D >>> Mark Millard >>> markmi at dsl-only.net >>>=20 >>> On 2016-Nov-19, at 7:00 PM, Mark Millard = wrote: >>>=20 >>> It may take a little bit but I'll try the patch. >>>=20 >>> It looks like sys/powerpc/include/pmap.h from -r176700 from = 2088-Mar-3 >>> is when the BOOKE/E500 split started with the preprocessor use of = AIM >>> and #else . This predates PowerMac G5 support. >>>=20 >>> This is definitely not new for the general structure on the powerpc >>> side of things. Any place that did not have the AIM vs. not status >>> available was subject to problems of possibly mismatched = definitions. >>>=20 >>> =3D=3D=3D >>> Mark Millard >>> markmi at dsl-only.net >>>=20 >>> On 2016-Nov-19, at 6:47 PM, Justin Hibbits = wrote: >>>=20 >>> On Sat, 19 Nov 2016 18:36:39 -0800 >>> Mark Millard wrote: >>>=20 >>>> [Quick top post I'm afraid.] >>>>=20 >>>> I think that I figured out why there is a problem even earlier >>>> --that just did not stop the compiles. >>>>=20 >>>> lib/libutil/kinfo_getallproc.c is built here as part of buildworld >>>> (stage 4.2 "building libraries" instead of buildkernel. It does not >>>> have the KERNCONF's AIM vs. BOOKE vs. . . . definitions vs. lack of >>>> them). >>>>=20 >>>> So if it includes machine/pmap.h that binds to >>>> sys/powerpc/include/pmap.h which has the structure. . . >>>>=20 >>>> . . . >>>> #if defined(AIM) >>>> . . . (definitions here) >>>> #elif defined(BOOKE) >>>> . . . (definitions here) >>>> #endif >>>> . . . >>>>=20 >>>> it gets no definition now. >>>>=20 >>>> With the older: >>>>=20 >>>> . . . >>>> #if defined(AIM) >>>> . . . (definitions here) >>>> #else >>>> . . . (definitions here) >>>> #endif >>>> . . . >>>>=20 >>>> It got a definition, just not necessarily the right one. >>>>=20 >>>>=20 >>>> =3D=3D=3D >>>> Mark Millard >>>> markmi at dsl-only.net >>>=20 >>> Can you try the attached patch? There was a subtle ABI issue that >>> r308817 exposed, which is that the pmap structs aren't identical = such >>> that the pm_stats are at different locations, and libkvm ends up >>> reading with the Book-E pmap, getting different stats than expected = for >>> AIM. This patch fixes that, bumping version to account for this ABI >>> change. >>>=20 >>> - Justin >>=20 >>=20 >>=20 >=20 >=20