Re: [PATCH 1/5] ftrace: Synchronize variable setting with breakpoints

[Steven Rostedt <rostedt@goodmis.org>]

On Thu, 2012-05-31 at 19:28 +0200, Peter Zijlstra wrote:

> I don't know, nor did you explain the implicit ordering there. Also in
> such diagrams you need the other side as well.

Right, I forgot to add that.

> 
> > If another breakpoint is hit (one other than one put in by ftrace) then
> > we don't care. It wont crash the system whether or not A is 1 or 0. We
> > just need to make sure that a ftrace breakpoint that is hit knows that
> > it was a ftrace breakpoint (calls the ftrace handler). No other
> > breakpoint should be on a ftrace nop anyway.
> 
> So the ordering is like:
> 
> ---
> 
>  CPU-0				CPU-1
> 
> 
>  lock inc mod-count /* implicit (w)mb */
>  write int3
> 				<trap-int3> /* implicit (r)mb */
> 				load mod-count
> 
>  sync-ipi-broadcast
>  write rest-of-instruction
>  sync-ipi-broadcast
>  write head-of-instruction
>  sync-ipi-broadcast
>  lock dec mod-count /* implicit (w)mb */
> 
> 
> Such that when we observe the int3 on CPU-1 we also must see the
> increment on mod-count.
> 
> ---
> 
> A simple something like the above makes it very clear what we're doing
> and what we're expecting.

You are obviously better at drawing such diagrams than I am. Which also
explains why I needed to do the lockdep annotations to understand them
better. Because I don't have your ability to visualize it in such a nice
simple diagram. My vision is more of a bayesian tree than an oak. I
guess that explains why I like complex code so much :-)


>  I think a (local) trap should imply a barrier
> of sorts but will have to defer to others (hpa?) to confirm.

I would think it does, but lets have hpa or others confirm.


>  But at the
> very least write it down someplace that you are assuming that.

I'll cut and paste this into the comments.

> 
> 
> 
> fwiw run_sync() could do with a much bigger comment on why its sane to
> enable interrupts.. That simply reeks, enabling interrupts too early can
> wreck stuff properly.

Hmm, actually, I think these patches avoid the need for run_sync() at
boot up now anyway. Thus it shouldn't need to enable interrupts.

The ftrace.c code was what disabled interrupts on start up (and
re-enables them) before calling into this, this is because the old way
needed interrupts disabled, and other archs still use the old way. But,
your point stands, it should be commented better.

-- Steve