Re: [PATCH] x86: Use named address spaces in asm/percpu.h

[Ingo Molnar <mingo@kernel.org>]

* Richard Henderson <rth@twiddle.net> wrote:

> GCC 6 adds support for __seg_fs and __seg_gs as named address spaces,
> producing the obvious segment overrides for objects so marked.
> 
> Exposing the memory reference allows slightly better code generation
> in some cases (and in others, merely affects the scheduling).  E.g.:
> 
> [1]
> - mov    %gs:0x0(%rip),%eax
> -        R_X86_64_PC32      context_tracking+0x4
> - cmp    $0x1,%eax
> + cmpl   $0x1,%gs:0x0(%rip)
> +        R_X86_64_PC32      context_tracking+0x3
> 
> [2]
> - mov    %gs:0x0(%rip),%ebx
> -        R_X86_64_PC32      cpu_number-0x4
> - movslq %ebx,%rax
> + movslq %gs:0x0(%rip),%rax
> +        R_X86_64_PC32      cpu_number-0x4
> 
> [3]
> - mov    %gs:0x0(%rip),%rdx
> -        R_X86_64_PC32      cpu_info+0x20
> - test   $0x1000000,%edx
> + testb  $0x1,%gs:0x0(%rip)
> +        R_X86_64_PC32      cpu_info+0x22
> 
> [4]
> - mov    $0x0,%rax
> -        R_X86_64_32S       __uv_hub_info
> - mov    %rax,%rcx
> - add    %gs:0x0(%rip),%rcx
> -        R_X86_64_PC32      this_cpu_off-0x4
> - movzbl 0x15(%rcx),%ecx
> ...
> - mov    %rax,%rdx
> - add    %gs:0x0(%rip),%rdx
> -        R_X86_64_PC32      this_cpu_off-0x4
> - or     (%rdx),%rcx
> + mov    %gs:0x0(%rip),%r9
> +        R_X86_64_PC32      this_cpu_off-0x4
> + mov    $0x0,%rax
> +        R_X86_64_32S       __uv_hub_info
> ...
> + movzbl 0x15(%rax,%r9,1),%ecx
> ...
> + or     (%rax,%r9,1),%rdx
> 
> The final vmlinux text size is reduced by about 5k for a standard
> Fedora configure.

Very nice!

> Cc: Ingo Molnar <mingo@redhat.com>
> Cc: "H. Peter Anvin" <hpa@zytor.com>
> Signed-off-by: Richard Henderson <rth@twiddle.net>
> ---
>  arch/x86/include/asm/percpu.h | 145 +++++++++++++++++++++++++-----------------
>  1 file changed, 86 insertions(+), 59 deletions(-)

I have a small request: since this depends on a very new compiler feature, and to 
make this easier to revert (even just for testing), could you split it into two 
patches: the first patch introduces the __percpu_addrspace machinery, the second 
one activates it by defining it?

The second patch will be easy to bisect to and will be easy to revert even many 
releases down the road, as GCC6 is tested more widely. We could also add a CONFIG_ 
switch for this - but that might be overkill.

> +#ifdef __percpu_addrspace
> +/* Produce an address-space lvalue for VAR.  */
> +#define __percpu_as(VAR) \
> +  (*(typeof(VAR) __kernel __force __percpu_addrspace *)(uintptr_t)&(VAR))
> +
> +/* We cannot allow __my_cpu_offset to recurse through this_cpu_read, as
> +   this will change based on CONFIG_X86_64, with which games are played
> +   in 32-bit compatibility files.  */

Small nit: please use the customary (multi-line) comment style:

  /*
   * Comment .....
   * ...... goes here.
   */

Thanks,

	Ingo