Re: Fix quilt merge error in acpi-cpufreq.c

[Rusty Russell]

On Mon, 20 Apr 2009 08:08:05 pm Ingo Molnar wrote:
> Can you integrate these two into a single summary line? The obvious 
> solution would be to append them:
> 
>      tracing/ring-buffer: add unlock recursion protection on discard to fix spurious warning triggering tracing shutdown
> 
> but 121 characters width is _way_ too long for a summary.

No, if I'm reading this commit correctly, the commit message is well written,
just really bad.

    tracing/ring-buffer: Add unlock recursion protection on discard

-- This helps the patch reviewer, but noone else.  And the patch reviewer
   should be reading the entire thing anyway.  This is a fix, but you have
   to read to the bottom to know that.

    The pair of helpers trace_recursive_lock() and trace_recursive_unlock()
    have been introduced recently to provide generic tracing recursion
    protection.

    They are used in a symetric way:

     - trace_recursive_lock() on buffer reserve
     - trace_recursive_unlock() on buffer commit

-- Err, why is this verbiage in this patch at all?

    However sometimes, we don't commit but discard on entry
    to the buffer, ie: in case of filter checking.

    Then we must also unlock the recursion protection on discard time,
    otherwise the tracing gets definitely deactivated and a warning
    is raised spuriously, such as:

-- So, the problem is that tracing gets deactivated permanently?  Also a
   warning is raised (in which case is it really spurious?).   Since I have
   no idea what this code does, is it common?  When was this problem
   introduced?  

    111.119821] ------------[ cut here ]------------
    [  111.119829] WARNING: at kernel/trace/ring_buffer.c:1498 ring_buffer_lock_reserve+0x1b7/0x1d0()
    [  111.119835] Hardware name: AMILO Li 2727
    [  111.119839] Modules linked in:
    [  111.119846] Pid: 5731, comm: Xorg Tainted: G        W  2.6.30-rc1 #69
    [  111.119851] Call Trace:
    [  111.119863]  [<ffffffff8025ce68>] warn_slowpath+0xd8/0x130
    [  111.119873]  [<ffffffff8028a30f>] ? __lock_acquire+0x19f/0x1ae0
    [  111.119882]  [<ffffffff8028a30f>] ? __lock_acquire+0x19f/0x1ae0
    ...

-- Good, a backtrace is nice.

   [ Impact: fix spurious warning triggering tracing shutdown ]

-- Hidden away here, I don't think I like this.  Not an improvement.

    Signed-off-by: Frederic Weisbecker <fweisbec@xxxxxxxxx>

The patch itself basically adds trace_recursive_unlock() to event discard.
Seems like it should always have done that, and it's a simple bug that it
didn't.  But I had to work hard to figure that out.

Rusty.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/