linux-kernel - Re: Why have another variable deciding a tracepoint?

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20131115095240.334c9f6b@gandalf.local.home>
Date:	Fri, 15 Nov 2013 09:52:40 -0500
From:	Steven Rostedt <rostedt@...dmis.org>
To:	Terje Bergström <tbergstrom@...dia.com>
Cc:	LKML <linux-kernel@...r.kernel.org>,
	Arto Merilainen <amerilainen@...dia.com>,
	Thierry Reding <thierry.reding@...onic-design.de>,
	Erik Faye-Lund <kusmabite@...il.com>
Subject: Re: Why have another variable deciding a tracepoint?

On Fri, 15 Nov 2013 10:17:41 +0200
Terje Bergström <tbergstrom@...dia.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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/