lkml.org 
[lkml]   [2012]   [Feb]   [14]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    Patch in this message
    /
    Date
    From
    SubjectRe: [PATCH] percpu: use raw_local_irq_* in _this_cpu op
    On Tue, Feb 14, 2012 at 11:30:06AM +0800, Ming Lei wrote:
    > Hi,
    >
    > On Tue, Feb 14, 2012 at 1:23 AM, Tejun Heo <tj@kernel.org> wrote:
    > > On Mon, Feb 13, 2012 at 07:03:38PM +0800, Ming Lei wrote:
    > >> It doesn't make sense to trace irq off or do irq flags
    > >> lock proving inside 'this_cpu' operations, so replace local_irq_*
    > >> with raw_local_irq_* in 'this_cpu' op.
    > >>
    > >> Also the patch fixes one lockdep warning[1], which is caused
    > >> by the added local_irq_save/restore(flags) in this_cpu_inc
    > >> called by __debug_atomic_inc: kernel/lockdep.c
    > >
    > > I think this isn't gonna hurt anything but I don't understand why the
    > > lockdep warning is triggering when using traced version. ?Can you
    > > please explain that in a bit more detail in the patch description?
    >
    > In trace_hardirqs_on_caller:kernel/lockdep.c, __debug_atomic_inc
    > will be called to add on 'this_cpu' variable, so may introduce recursive
    > trace_hardirqs_on|off_caller called.

    Don't we need to prevent this kind of recursion first?

    UNTESTED patch, I guess it'll smooth your concern.
    ---
    diff --git a/kernel/lockdep.c b/kernel/lockdep.c
    index 8889f7d..028b4c5 100644
    --- a/kernel/lockdep.c
    +++ b/kernel/lockdep.c
    @@ -2561,6 +2561,8 @@ void trace_hardirqs_on_caller(unsigned long ip)
    if (unlikely(!debug_locks || current->lockdep_recursion))
    return;

    + current->lockdep_recursion = 1;
    +
    if (unlikely(current->hardirqs_enabled)) {
    /*
    * Neither irq nor preemption are disabled here
    @@ -2568,7 +2570,7 @@ void trace_hardirqs_on_caller(unsigned long ip)
    * in a stat is not a big deal.
    */
    __debug_atomic_inc(redundant_hardirqs_on);
    - return;
    + goto out;
    }

    /*
    @@ -2577,23 +2579,24 @@ void trace_hardirqs_on_caller(unsigned long ip)
    * enabled.. someone messed up their IRQ state tracing.
    */
    if (DEBUG_LOCKS_WARN_ON(!irqs_disabled()))
    - return;
    + goto out;

    /*
    * See the fine text that goes along with this variable definition.
    */
    if (DEBUG_LOCKS_WARN_ON(unlikely(early_boot_irqs_disabled)))
    - return;
    + goto out;

    /*
    * Can't allow enabling interrupts while in an interrupt handler,
    * that's general bad form and such. Recursion, limited stack etc..
    */
    if (DEBUG_LOCKS_WARN_ON(current->hardirq_context))
    - return;
    + goto out;

    - current->lockdep_recursion = 1;
    __trace_hardirqs_on_caller(ip);
    +
    +out:
    current->lockdep_recursion = 0;
    }
    EXPORT_SYMBOL(trace_hardirqs_on_caller);

    \
     
     \ /
      Last update: 2012-02-14 08:43    [W:0.023 / U:122.816 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site