[ltt-dev] Ftrace code in the 2.6.29 kernel

[ltt-dev] Ftrace code in the 2.6.29 kernel

[Mathieu Desnoyers]

Hi Steven,

I am giving a look at the ftrace code, and I am a bit confused by the
way you handle reentrancy in ring_buffer.c. (this is the code in 2.6.29)
Please tell me if I missed important details :

1) you seem to have removed any sort of "nesting" check to allow NMI
handlers to run. Previously, I remember that you simply discarded the
event if a NMI handler appeared to run over the ring buffer code.

2) Assuming 1) is true, then __rb_reserve_next() called from
ring_buffer_lock_reserve() is protected by :

               local_irq_save(flags);
                __raw_spin_lock(&cpu_buffer->lock);

Which I think is the last thing you want to see in a NMI handler. It
sounds like this code is begging for a deadlock to occur if run in NMI
context. Or maybe you don't claim that this code supports NMI, but then
you should remove the following comment from ring_buffer.c :

rb_set_commit_to_write(struct ring_buffer_per_cpu *cpu_buffer)
{
        /*
         * We only race with interrupts and NMIs on this CPU.

So basically, if an NMI nests over that code, or if an instrumented
fault happens within the ring_buffer code, this would generate an
infinite recursive call chain of trap/tracing/trap/tracing...

So this is why I think I might have missed a sanity check somewhere.

Mathieu

-- 
Mathieu Desnoyers
OpenPGP key fingerprint: 8CD5 52C3 8E3C 4140 715F  BA06 3F25 A8FE 3BAE 9A68

[ltt-dev] Ftrace code in the 2.6.29 kernel

[Steven Rostedt]

On Thu, 2 Apr 2009, Mathieu Desnoyers wrote:

> Hi Steven,
> 
> I am giving a look at the ftrace code, and I am a bit confused by the
> way you handle reentrancy in ring_buffer.c. (this is the code in 2.6.29)
> Please tell me if I missed important details :
> 
> 1) you seem to have removed any sort of "nesting" check to allow NMI
> handlers to run. Previously, I remember that you simply discarded the
> event if a NMI handler appeared to run over the ring buffer code.

I did not remove anything. The code you refer to is queued up for 2.6.30.
When that code gets into mainline, we may be able to get it to stable if 
needed.

> 
> 2) Assuming 1) is true, then __rb_reserve_next() called from
> ring_buffer_lock_reserve() is protected by :
> 
>                local_irq_save(flags);
>                 __raw_spin_lock(&cpu_buffer->lock);
> 
> Which I think is the last thing you want to see in a NMI handler. It
> sounds like this code is begging for a deadlock to occur if run in NMI
> context. Or maybe you don't claim that this code supports NMI, but then
> you should remove the following comment from ring_buffer.c :
> 
> rb_set_commit_to_write(struct ring_buffer_per_cpu *cpu_buffer)
> {
>         /*
>          * We only race with interrupts and NMIs on this CPU.
> 
> So basically, if an NMI nests over that code, or if an instrumented
> fault happens within the ring_buffer code, this would generate an
> infinite recursive call chain of trap/tracing/trap/tracing...
> 
> So this is why I think I might have missed a sanity check somewhere.

Nope, you just saw patches that I sent to fix this issue, but those were 
not accepted into mainline. Luckily, 29 does not have many NMI users 
(function tracer is one, but that has its own nested protections).

2.6.30 will have the NMI protection.

2.6.31 will have a completely writer lockless ring buffer solution.

-- Steve

[ltt-dev] Ftrace code in the 2.6.29 kernel

[Lai Jiangshan]

Steven Rostedt wrote:
> On Thu, 2 Apr 2009, Mathieu Desnoyers wrote:
> 
> 
> 2.6.31 will have a completely writer lockless ring buffer solution.

It's wonderful!

I'm very interesting to known your idea before you implement it.

Lai

> 
> -- Steve
>