Date: Sun, 23 Jul 2017 06:55:51 +1000 (EST) From: Bruce Evans <brde@optusnet.com.au> To: Ryan Libby <rlibby@freebsd.org> Cc: src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-head@freebsd.org Subject: Re: svn commit: r321335 - in head/sys: amd64/amd64 x86/x86 Message-ID: <20170723054939.K1150@besplex.bde.org> In-Reply-To: <201707211711.v6LHBaZc026103@repo.freebsd.org> References: <201707211711.v6LHBaZc026103@repo.freebsd.org>
next in thread | previous in thread | raw e-mail | index | archive | help
On Fri, 21 Jul 2017, Ryan Libby wrote: > Log: > __pcpu: gcc -Wredundant-decls > > Pollution from counter.h made __pcpu visible in amd64/pmap.c. Delete > the existing extern decl of __pcpu in amd64/pmap.c and avoid referring > to that symbol, instead accessing the pcpu region via PCPU_SET macros. > Also delete an unused extern decl of __pcpu from mp_x86.c. Thanks. I use a similar quick fix. Unfortunately, it is not complete. clang is still broken. It doesn't support -Wredundant-decls even to the point of generating a warning when it is requested. There is still a lot of pollution: - the pollution is different on i386, so the problem doesn't occur for some reason, but the pollution is so convoluted that the reasons why it affects amd64 and doesn't affect i386 are both hard to see - pmap.c (or any file in the same directory) doesn't include counter.h on either amd64 or i386. The non-inclusion seems to be correct since no API in counter.h is used. It gets this include from other pollution. The pollution chain is very long. On i386, machine/counter.h is included by sys/counter.h (not polution); sys/counter.h is included by sys/sf_buf.h (polution); sys/sf_buf.h is included by pmap.c (not pollution). But there is no problem on i386/pmap.c because it doesn't declare or use __pcpu. I didn't find the exact pollution chain on amd64, but guess it is the same. There is was a problem on amd64/pmap.c since it did define and use __pcpu. > Modified: head/sys/amd64/amd64/pmap.c > ============================================================================== > --- head/sys/amd64/amd64/pmap.c Fri Jul 21 16:14:35 2017 (r321334) > +++ head/sys/amd64/amd64/pmap.c Fri Jul 21 17:11:36 2017 (r321335) > @@ -274,8 +274,6 @@ pmap_modified_bit(pmap_t pmap) > return (mask); > } > > -extern struct pcpu __pcpu[]; > - My quick fix was just to remove this. I didn't understand how this works. Now I see that it is because counter.h is polluted by much more than the declaration. It includes <sys/pcpu.h>. That gives the complete complete struct definition that is needed to use __pcpu. But it is weird that the extern variable is not declared in <sys/pcpu.h>. This seems to be the result of hiding the variable too well to prevent using it in places like counter.h, but then using it anyway. I hoped to declare it in md_var.h, but wasn't sure that an incomplete declaration would work there. Now I think it does. Any use requires completing the direction. In counter.h, this is done by including <sys/pcpu.h>. amd64/pmap.c should have done the same. The pollution makes the explicit include unecessary. The intended hiding seems to be: - __pcpu[] is an implementation detail that should only be used for initialization. Otherwise, it should be accessed using the PCPU access macros. It is intentionally not declared in any header file. Only the initialization code should refer to it. - the initialization code is machdep.c, where it is declared non-extern. So it shouldn't be declared in md_var.h either. > #if !defined(DIAGNOSTIC) > #ifdef __GNUC_GNU_INLINE__ > #define PMAP_INLINE __attribute__((__gnu_inline__)) inline > @@ -1063,8 +1061,8 @@ pmap_bootstrap(vm_paddr_t *firstaddr) > kernel_pmap->pm_pcids[i].pm_pcid = PMAP_PCID_KERN; > kernel_pmap->pm_pcids[i].pm_gen = 1; > } > - __pcpu[0].pc_pcid_next = PMAP_PCID_KERN + 1; > - __pcpu[0].pc_pcid_gen = 1; > + PCPU_SET(pcid_next, PMAP_PCID_KERN + 1); > + PCPU_SET(pcid_gen, 1); > /* > * pcpu area for APs is zeroed during AP startup. > * pc_pcid_next and pc_pcid_gen are initialized by AP > This seems right, but I thought it was wrong at first since it is initialization code. On x86, the PCPU macros depend on the pcpu segment register being initialized. I had problems getting the order right for implementing early use of the message buffer and printf(). Early use of the PCPU macros finds garbage in the segment register and loads futher garbage indirectly. I fixed this by initializing the segment register as the first thing done after leaving locore. The above is in pmap_bootstrap() which is quite late so it is clear that the segment register is initialized early enough even without my changes. > Modified: head/sys/x86/x86/mp_x86.c > ============================================================================== > --- head/sys/x86/x86/mp_x86.c Fri Jul 21 16:14:35 2017 (r321334) > +++ head/sys/x86/x86/mp_x86.c Fri Jul 21 17:11:36 2017 (r321335) > @@ -90,8 +90,6 @@ int mcount_lock; > int mp_naps; /* # of Applications processors */ > int boot_cpu_id = -1; /* designated BSP */ > > -extern struct pcpu __pcpu[]; > - > /* AP uses this during bootstrap. Do not staticize. */ > char *bootSTK; > int bootAP; Perhaps the variable can be made static. In FreeBSD-10, there is no counter.h to abuse it, and it was only extern since it was used here as well as in machdep.c (when this was named mp_machdep.c). Bruce
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20170723054939.K1150>