Re: [PATCH] tracing/markers: make markers select tracepoints

[Ingo Molnar <mingo@elte.hu>]

* Frederic Weisbecker <fweisbec@gmail.com> wrote:

> > > >  config MARKERS
> > > >  	bool "Activate markers"
> > > > -	depends on TRACEPOINTS
> > > > +	select TRACEPOINTS
> > > >  	help
> > > >  	  Place an empty function call at each marker site. Can be
> > > >  	  dynamically changed for a probe function.
> > > 
> > > but using "select" instead of "depends on" just causes the 
> > > kind of problem that you described, whereas using "depends on" 
> > > does follow dependency chains.
> > 
> > Well, as long as the secondary selects are 'expanded' along the 
> > line of dependencies, it should still be fine. With an 
> > increasing number of dependencies it quickly becomes ugly 
> > though. This might be one of the cases where it works.
> > 
> > Eventually we should eliminate markers, their uses can either be 
> > converted to new-style tracepoints, or to ftrace_printk().
> > 
> > 	Ingo
> 
> ftrace_printk adds more overhead if not used since it 
> inconditionally send the trace, unless the related 
> TRACE_ITER_PRINTK flag on ftrace is unset.
> 
> I don't know how markers work, but the documentation describes 
> that a single branch check is done in case the probe is 
> disabled.
> 
> With ftrace_printk, even if TRACE_ITER_PRINTK is unset, this 
> is still one call and one branch check. So for hot callsite 
> it's unappropriate.
> 
> IMHO, tracepoints are more suited to replace markers if they 
> have to.

there's i think the KVM usecase where markers are used 
essentially a printk()-alike flexible tracing facility.

This is how KVMTRACE looks like at the moment:

./vmx.c:	KVMTRACE_3D(MSR_READ, vcpu, ecx, (u32)data, (u32)(data >> 32),
./vmx.c:	KVMTRACE_3D(MSR_WRITE, vcpu, ecx, (u32)data, (u32)(data >> 32),
./vmx.c:	KVMTRACE_0D(PEND_INTR, vcpu, handler);
./vmx.c:	KVMTRACE_3D(VMEXIT, vcpu, exit_reason, (u32)kvm_rip_read(vcpu),
./vmx.c:		KVMTRACE_0D(NMI, vcpu, handler);
./lapic.c:	KVMTRACE_1D(APIC_ACCESS, apic->vcpu, (u32)offset, handler);
./lapic.c:	KVMTRACE_1D(APIC_ACCESS, apic->vcpu, (u32)offset, handler);
./svm.c:	KVMTRACE_2D(DR_READ, vcpu, (u32)dr, (u32)val, handler);
./svm.c:		KVMTRACE_3D(PAGE_FAULT, &svm->vcpu, error_code,
./svm.c:		KVMTRACE_3D(TDP_FAULT, &svm->vcpu, error_code,
./svm.c:	KVMTRACE_0D(NMI, &svm->vcpu, handler);

I think this could easily be converted to a wrapper around 
ftrace_printk() plus a "kvmtrace" ftrace plugin which activates 
those wrapped ftrace_printk() sites via a single global 
__read_mostly flag.

This means KVM tracing is activated via:

 echo kvmtrace > /debug/tracing/current_tracer

that's all.

Basically the conversion would look like this:

before:

        KVMTRACE_3D(MSR_READ, &svm->vcpu, ecx, (u32)data,
                    (u32)(data >> 32), handler);

after:

	kvm_trace("MSR_READ: %p, %08lx, %016Lx\n", &svm->vcpu, ecx, data);

As a result all these traces would become a lot more readable 
(and a lot more flexible) both in the source code, and in the 
trace output stage.

And any ad-hoc tracepoint can be added, without worrying about 
the name of the macro or the number of type of arguments. Note 
that in this specific example we didnt need to split up the u64 
'data' into two 32-bit values, nor do we have to pass in the 
'handler' name, nor do we have to provide a MSR_READ 
enumeration.

The tracing-disabled case would still be as fast - a single 
branch check.

Avi, what do you think, any objections against an RFC patchset 
that shows this off?

	Ingo