Re: [gcv v3 27/35] arm: Replace __get_cpu_var uses

[Will Deacon <will.deacon@arm.com>]

Hi Christoph,

Sorry for the delay in looking at this, I've been on holiday for a week.
Comments inline.

On Wed, Aug 28, 2013 at 08:48:23PM +0100, Christoph Lameter wrote:
> Transformations done to __get_cpu_var()
> 
> 
> 1. Determine the address of the percpu instance of the current processor.
> 
>         DEFINE_PER_CPU(int, y);
>         int *x = &__get_cpu_var(y);
> 
>     Converts to
> 
>         int *x = this_cpu_ptr(&y);
> 
> 
> 2. Same as #1 but this time an array structure is involved.
> 
>         DEFINE_PER_CPU(int, y[20]);
>         int *x = __get_cpu_var(y);
> 
>     Converts to
> 
>         int *x = this_cpu_ptr(y);

This is the flavour we have for ARM's hw_breakpoint code, where we have an
array of perf_event * instead of int...

> Index: linux/arch/arm/kernel/hw_breakpoint.c
> ===================================================================
> --- linux.orig/arch/arm/kernel/hw_breakpoint.c  2013-08-26 13:48:40.956794980 -0500
> +++ linux/arch/arm/kernel/hw_breakpoint.c       2013-08-26 13:48:40.952795024 -0500
> @@ -344,13 +344,13 @@ int arch_install_hw_breakpoint(struct pe
>                 /* Breakpoint */
>                 ctrl_base = ARM_BASE_BCR;
>                 val_base = ARM_BASE_BVR;
> -               slots = (struct perf_event **)__get_cpu_var(bp_on_reg);
> +               slots = (struct perf_event **)__this_cpu_read(bp_on_reg);

...so I don't think this is quite right, and indeed, we get a bunch of errors
from GCC:

arch/arm/kernel/hw_breakpoint.c: In function ‘arch_install_hw_breakpoint’:
arch/arm/kernel/hw_breakpoint.c:347:33: error: incompatible types when assigning to type ‘struct perf_event *[16]’ from type ‘struct perf_event **’
arch/arm/kernel/hw_breakpoint.c:347:1: error: incompatible types when assigning to type ‘struct perf_event *[16]’ from type ‘struct perf_event **’
arch/arm/kernel/hw_breakpoint.c:347:1: error: incompatible types when assigning to type ‘struct perf_event *[16]’ from type ‘struct perf_event **’
arch/arm/kernel/hw_breakpoint.c:347:1: error: incompatible types when assigning to type ‘struct perf_event *[16]’ from type ‘struct perf_event **’

changing to match your recipe still doesn't work, however:

arch/arm/kernel/hw_breakpoint.c: In function ‘arch_install_hw_breakpoint’:
arch/arm/kernel/hw_breakpoint.c:347:33: error: cast specifies array type

> Index: linux/arch/arm64/kernel/debug-monitors.c
> ===================================================================
> --- linux.orig/arch/arm64/kernel/debug-monitors.c       2013-08-26 13:48:40.956794980 -0500
> +++ linux/arch/arm64/kernel/debug-monitors.c    2013-08-26 13:48:40.952795024 -0500
> @@ -98,11 +98,11 @@ void enable_debug_monitors(enum debug_el
> 
>         WARN_ON(preemptible());
> 
> -       if (local_inc_return(&__get_cpu_var(mde_ref_count)) == 1)
> +       if (this_cpu_inc_return(mde_ref_count) == 1)
>                 enable = DBG_MDSCR_MDE;

I'm not sure that this is safe. We rely on local_inc_return to be atomic
with respect to the current CPU, which will end up being a wrapper around
atomic64_inc_return. However, this_cpu_inc_return simply uses a lock, so
other people accessing the count in a different manner (local_dec_and_test
below) may break local atomicity unless we start disabling interrupts or
something horrible like that.
 
>         if (el == DBG_ACTIVE_EL1 &&
> -           local_inc_return(&__get_cpu_var(kde_ref_count)) == 1)
> +           this_cpu_inc_return(kde_ref_count) == 1)
>                 enable |= DBG_MDSCR_KDE;
> 
>         if (enable && debug_enabled) {
> @@ -118,11 +118,11 @@ void disable_debug_monitors(enum debug_e
> 
>         WARN_ON(preemptible());
> 
> -       if (local_dec_and_test(&__get_cpu_var(mde_ref_count)))
> +       if (local_dec_and_test(this_cpu_ptr(&mde_ref_count)))
>                 disable = ~DBG_MDSCR_MDE;
> 
>         if (el == DBG_ACTIVE_EL1 &&
> -           local_dec_and_test(&__get_cpu_var(kde_ref_count)))
> +           local_dec_and_test(this_cpu_ptr(&kde_ref_count)))
>                 disable &= ~DBG_MDSCR_KDE;
> 
>         if (disable) {

Will