Re: [PATCH 2/2] Markers Implementation for Preempt RCU Boost Tracing

[Ingo Molnar <mingo@elte.hu>]

* K. Prasad <prasad@linux.vnet.ibm.com> wrote:

> @@ -486,12 +309,12 @@ void rcu_unboost_readers(void)
>  
>  	spin_lock_irqsave(&rcu_boost_wake_lock, flags);
>  
> -	rcu_trace_boost_try_unboost_readers(RCU_BOOST_ME);
> +	trace_mark(try_unboost_readers, "%u", smp_processor_id());

this isnt directed at you or your patch, it's more directed at Mathieu, 
but looking at this actual markers patch submitted to me, i'm still 
fundamentally worried about the whole marker approach.

Firstly, why on earth does a full format string have to be passed in for 
something as simple as a CPU id? This way we basically codify it forever 
that tracing _has_ to be expensive when enabled. The latency-tracer 
(which i'd love to convert to markers, if only markers were capable 
enough) has shown it that tracing _can_ be used to capture performance 
data without disturbing the measured system, even at hundreds of 
thousands context switches a second per CPU.

Secondly, the inlined overhead of trace_mark() is still WAY too large:

                if (unlikely(__mark_##name.state)) {                    \
                        preempt_disable();                              \
                        (*__mark_##name.call)                           \
                                (&__mark_##name, call_data,             \
                                format, ## args);                       \
                        preempt_enable();                               \
                }                                                       \

Whatever became of the obvious suggestion that i outlined years ago, to 
have a _single_ trace call instruction and to _patch out_ the damn 
marker calls by default? No .state flag checking inlined. No 
preempt_disable()/enable() pair. Patching static tracepoints OUT of the 
kernel, only leaving a ~5-byte NOP sequence behind them (and some 
minimal disturbance to the variables the tracepoint accesses). We've got 
all the alternatives.h code patching infrastructure available for such 
purposes. Why are we 2 years down the line and _STILL_ arguing about 
this?

Thirdly, the patch selects CONFIG_MARKERS:

>  config RCU_TRACE
> -       bool "Enable tracing for RCU - currently stats in debugfs"
> +       tristate "Enable tracing for RCU - currently stats in debugfs"
>         select DEBUG_FS
> -       default y
> +       select MARKERS

Which adds overhead (inlined checks for markers) all around the kernel, 
even if all markers are deactivated! Imagine a thousand of them and the 
kernel blows up measurably.

Sadly, this whole trace_mark() API seems to have gotten much worse since 
i last saw it. It's sub-par when it's turned on and it's sub-par when 
it's turned off. It gets us the worst of both worlds.

If it continues like this then i'd much rather see people add printks as 
tracing, because there you _know_ that it's high-overhead and people 
wont start arguing about trace compatibility either, etc.

	Ingo