lkml.org 
[lkml]   [2013]   [Aug]   [30]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [gcv v3 27/35] arm: Replace __get_cpu_var uses
    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
    --
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

    \
     
     \ /
      Last update: 2013-08-30 13:01    [W:2.619 / U:0.152 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site