svn commit: r321335 - in head/sys: amd64/amd64 x86/x86

[Bruce Evans]

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