Why have another variable deciding a tracepoint?

Why have another variable deciding a tracepoint?

[Steven Rostedt]

I've been reviewing different users of tracepoints and I stumbled
across this:

drivers/gpu/host1x/cdma.c: host1x_cdma_push()

	if (host1x_debug_trace_cmdbuf)
		trace_host1x_cdma_push(dev_name(cdma_to_channel(cdma)->dev),
				       op1, op2);

That host1x_debug_trace_cmdbuf is a variable that gets set by another
debugfs file "trace_cmdbuf" that is custom to this driver.

Why?

The tracepoint host1x_cdma_push is already controlled by either ftrace
or perf. If it gets enabled by perf or ftrace, it still wont be traced
unless we also enable this trace_cmdbuf. Is there some reason for this?
I can't figure it out from the change log: 6236451d83a720 ("gpu:
host1x: Add debug support").

As tracepoints uses jump labels, there is no branch cost associated
with them. That is, they are either a direct jump, or a nop (in most
cases a nop). But here you added the overhead of a conditional branch
depending on this variable.

If this is truly needed, then use TRACE_EVENT_CONDITION() for that
tracepoint.


/me is baffled

-- Steve

Re: Why have another variable deciding a tracepoint?

[Terje Bergström]

On 15.11.2013 06:48, Steven Rostedt wrote:
> I've been reviewing different users of tracepoints and I stumbled
> across this:
> 
> drivers/gpu/host1x/cdma.c: host1x_cdma_push()
> 
> 	if (host1x_debug_trace_cmdbuf)
> 		trace_host1x_cdma_push(dev_name(cdma_to_channel(cdma)->dev),
> 				       op1, op2);
> 
> That host1x_debug_trace_cmdbuf is a variable that gets set by another
> debugfs file "trace_cmdbuf" that is custom to this driver.
> 
> Why?

This is because it takes a lot of time to prepare for dumping the cmdbuf
data, like mapping buffer and unmapping after tracing. We want to avoid
all that preparation time.

> The tracepoint host1x_cdma_push is already controlled by either ftrace
> or perf. If it gets enabled by perf or ftrace, it still wont be traced
> unless we also enable this trace_cmdbuf. Is there some reason for this?
> I can't figure it out from the change log: 6236451d83a720 ("gpu:
> host1x: Add debug support").
> 
> As tracepoints uses jump labels, there is no branch cost associated
> with them. That is, they are either a direct jump, or a nop (in most
> cases a nop). But here you added the overhead of a conditional branch
> depending on this variable.
> 
> If this is truly needed, then use TRACE_EVENT_CONDITION() for that
> tracepoint.

TRACE_EVENT_CONDITION() isn't documented, so I don't know how that would
help.

Terje

Re: Why have another variable deciding a tracepoint?

[Steven Rostedt]

On Fri, 15 Nov 2013 10:17:41 +0200
Terje Bergström <tbergstrom@nvidia.com> wrote:

> On 15.11.2013 06:48, Steven Rostedt wrote:
> > I've been reviewing different users of tracepoints and I stumbled
> > across this:
> > 
> > drivers/gpu/host1x/cdma.c: host1x_cdma_push()
> > 
> > 	if (host1x_debug_trace_cmdbuf)
> > 		trace_host1x_cdma_push(dev_name(cdma_to_channel(cdma)->dev),
> > 				       op1, op2);
> > 
> > That host1x_debug_trace_cmdbuf is a variable that gets set by another
> > debugfs file "trace_cmdbuf" that is custom to this driver.
> > 
> > Why?
> 
> This is because it takes a lot of time to prepare for dumping the cmdbuf
> data, like mapping buffer and unmapping after tracing. We want to avoid
> all that preparation time.
> 

So let me get this straight. The recording of the cdma_push() data is
slow, correct? What mapping are you talking about?

	trace_host1x_cdma_push(dev_name(cdma_to_channel(cdma)->dev),
				       op1, op2);

#define cdma_to_channel(cdma) container_of(cdma, struct host1x_channel, cdma)

That just references the cdma field of struct host1x_channel, to get
the dev field, which does dev_name() that just gets the dev->init_name
(if defined, or kobject_name() if not). And the two u32 fields that are
passed in.

The tracepoint assigns with:

	TP_fast_assign(
		__entry->name = name;
		__entry->op1 = op1;
		__entry->op2 = op2;
	),

So I still have to ask; what do you have to set up and take down here?



> > The tracepoint host1x_cdma_push is already controlled by either ftrace
> > or perf. If it gets enabled by perf or ftrace, it still wont be traced
> > unless we also enable this trace_cmdbuf. Is there some reason for this?
> > I can't figure it out from the change log: 6236451d83a720 ("gpu:
> > host1x: Add debug support").
> > 
> > As tracepoints uses jump labels, there is no branch cost associated
> > with them. That is, they are either a direct jump, or a nop (in most
> > cases a nop). But here you added the overhead of a conditional branch
> > depending on this variable.
> > 
> > If this is truly needed, then use TRACE_EVENT_CONDITION() for that
> > tracepoint.
> 
> TRACE_EVENT_CONDITION() isn't documented, so I don't know how that would
> help.
> 

It's documented in the code ;-)  (kidding)

Yeah, I need to get that out a bit more. That's actually how I found
this code, I'm doing searches for all the locations that can benefit
from TRACE_EVENT_CONDITION(), and I'm trying to fix them up. I'm still
not convinced that this extra variable checking is required for this
tracepoint.

As you state though, I'll have to get time to document CONDITION() and
how and when to use it.

Thanks,

-- Steve

Re: Why have another variable deciding a tracepoint?

[Terje Bergström]

On 15.11.2013 16:52, Steven Rostedt wrote:
> So let me get this straight. The recording of the cdma_push() data is
> slow, correct? What mapping are you talking about?
> 
> 	trace_host1x_cdma_push(dev_name(cdma_to_channel(cdma)->dev),
> 				       op1, op2);
> 
> #define cdma_to_channel(cdma) container_of(cdma, struct host1x_channel, cdma)
> 
> That just references the cdma field of struct host1x_channel, to get
> the dev field, which does dev_name() that just gets the dev->init_name
> (if defined, or kobject_name() if not). And the two u32 fields that are
> passed in.
> 
> The tracepoint assigns with:
> 
> 	TP_fast_assign(
> 		__entry->name = name;
> 		__entry->op1 = op1;
> 		__entry->op2 = op2;
> 	),
> 
> So I still have to ask; what do you have to set up and take down here?

Oops, there's a piece missing that I didn't mention. What we want is a
full trace of what we're sending to the push buffer. See
hw/channel_hw.c/trace_write_gather(). It maps the buffer handed from
user space, dumps it to ftrace and unmaps it. That costs a lot of time
that we don't want to spend in normal operation and hence the special
condition.

trace_host1x_cdma_push() in the code you refer to just makes the dump
complete by adding some overhead opcodes that kernel needs to add. It
doesn't make sense to output that if we don't get the bulk from
trace_write_gather() and that's why it's also checking the same
conditional. It'd just make the trace look corrupted.

> It's documented in the code ;-)  (kidding)
> 
> Yeah, I need to get that out a bit more. That's actually how I found
> this code, I'm doing searches for all the locations that can benefit
> from TRACE_EVENT_CONDITION(), and I'm trying to fix them up. I'm still
> not convinced that this extra variable checking is required for this
> tracepoint.
> 
> As you state though, I'll have to get time to document CONDITION() and
> how and when to use it.

Sounds good. :-)

Terje