markers: example of irq regular kernel markers

Add trace points of irq handle events ported from LTTng's markers.

Signed-off-by Masami Hiramatsu <[email protected]>

---
I just rewrote LTTng's irq event by using DEFINE_TRACE for example.

 include/linux/irq_trace.h |    6 ++++++
 kernel/irq/handle.c       |    6 ++++++
 2 files changed, 12 insertions(+)

Index: 2.6.26-rc5-mm3/include/linux/irq_trace.h
===================================================================

Index: 2.6.26-rc5-mm3/kernel/irq/handle.c
===================================================================

kernel/irq/handle.c +6 -0

@@ -15,6 +15,7 @@
 #include <linux/random.h>
 #include <linux/interrupt.h>
 #include <linux/kernel_stat.h>
+#include <linux/irq_trace.h>
 
 #include "internals.h"
 

@@ -130,6 +131,9 @@ irqreturn_t handle_IRQ_event(unsigned in
 {
 	irqreturn_t ret, retval = IRQ_NONE;
 	unsigned int status = 0;
+	struct pt_regs *regs = get_irq_regs();
+
+	trace_irq_entry(irq, (regs)?(!user_mode(regs)):(1));
 
 	handle_dynamic_tick(action);
 

@@ -148,6 +152,8 @@ irqreturn_t handle_IRQ_event(unsigned in
 		add_interrupt_randomness(irq);
 	local_irq_disable();
 
+	trace_irq_exit();
+
 	return retval;
 }
 

Signature · 7 lines

Masami Hiramatsu

Software Engineer
Hitachi Computer Products (America) Inc.
Software Solutions Division

e-mail: [email protected]

Quoted 15 lines · * Masami Hiramatsu ([email protected]) wrote:

Add trace points of irq handle events ported from LTTng's markers.

Signed-off-by Masami Hiramatsu <[email protected]>

---
I just rewrote LTTng's irq event by using DEFINE_TRACE for example.

 include/linux/irq_trace.h |    6 ++++++
 kernel/irq/handle.c       |    6 ++++++
 2 files changed, 12 insertions(+)

Index: 2.6.26-rc5-mm3/include/linux/irq_trace.h
===================================================================

include/linux/irq_trace.h line 6

Quoted 6 lines

include/linux/irq_trace.h new file +6 -0

@@ -0,0 +1,6 @@
+#include <linux/marker.h>
+
+DEFINE_TRACE(irq_entry, (int irq_id, int kernel_mode), irq_id, kernel_mode);
+
+DEFINE_TRACE(irq_exit, (void));
+

All this work look good, thanks Masami! Sorry I did not find time to do it lately, I've been busy on other things. A small question though :
since LTTng is configurable both as an external module or as an in-kernel tracer, I wonder if it would really hurt to add the format strings to DEFINE_TRACE, e.g. :

DEFINE_TRACE(name, prototype, format_string, args...)

which would give :

DEFINE_TRACE(irq_entry, (int irq_id, int kernel_mode), "%d %d",

    irq_id, kernel_mode);

DEFINE_TRACE(irq_exit, (void), MARK_NOARGS);

and calling this in the kernel code :

trace_irq_entry(irq, (regs)?(!user_mode(regs)):(1));
...
trace_irq_exit();

and for quick-and-dirty debug usage, one would add this to kernel code :

trace_mark(subsystem_event, "(int arg, struct task_struct *task)",
"%d %p", arg, current);

By doing so, we could leave a gcc format string check by passing the format string to __mark_check_format(). We could extract the field names from the prototype, so there is no need to duplicate field information in the format string.

Since the format string information is hidden in a header but kept at the same location as the trace point definition, I think it reaches goals of being "neat", efficient for general purpose tracers and to keep the tracepoint information all in one place so we don't end up adding stuff to various information consumers whenever a new trace point is added.

What do you think of these changes ?

Mathieu

Quoted 40 lines

Index: 2.6.26-rc5-mm3/kernel/irq/handle.c
===================================================================

kernel/irq/handle.c +6 -0

@@ -15,6 +15,7 @@
 #include <linux/random.h>
 #include <linux/interrupt.h>
 #include <linux/kernel_stat.h>
+#include <linux/irq_trace.h>
 
 #include "internals.h"
 

@@ -130,6 +131,9 @@ irqreturn_t handle_IRQ_event(unsigned in
 {
 	irqreturn_t ret, retval = IRQ_NONE;
 	unsigned int status = 0;
+	struct pt_regs *regs = get_irq_regs();
+
+	trace_irq_entry(irq, (regs)?(!user_mode(regs)):(1));
 
 	handle_dynamic_tick(action);
 

@@ -148,6 +152,8 @@ irqreturn_t handle_IRQ_event(unsigned in
 		add_interrupt_randomness(irq);
 	local_irq_disable();
 
+	trace_irq_exit();
+
 	return retval;
 }
 

Signature · 7 lines

Masami Hiramatsu

Software Engineer
Hitachi Computer Products (America) Inc.
Software Solutions Division

e-mail: [email protected]

Quoted 4 lines

By doing so, we could leave a gcc format string check by passing the format string to __mark_check_format(). We could extract the field names from the prototype, so there is no need to duplicate field information in the format string.

I thought that someone complained against those format strings in kernel code. Thus I removed it from DEFINE_TRACE.

even though, I think you can do that by adding below string table to LTTng module.

const char *lookup_table[MAX_MARKERS][2] = {
{"irq_entry", "%d %d"}, // or "(int irq_id, int kernel_mode)", "%d %d"
...
};

Quoted 8 lines

even though, I think you can do that by adding below string table to LTTng module.

const char *lookup_table[MAX_MARKERS][2] = {
{"irq_entry", "%d %d"}, // or "(int irq_id, int kernel_mode)", "%d %d"
...
};

if move string to out of kernel core, compiler may kill some variable. thus, we will get incomplete tracing result.

I think your proposal is very interesting.
but I dont understand why someone dislike format strings. Could you explain this reason?

[...] Format strings only help for printf like operations. [...]

The best example of this is the sched_switch code. LTTng and friends just want a pid and comm to show. But there's tracers that want more info from the task_struct. We also like to see the priority of the task. [...]

Passing in a pointer to the structure being traced should be enough for all tracers.

Now back to your question, why don't we like the printf format. Simply because it does nothing for pointers. It might help you with a %d and number of parameters, but a %p can not tell the difference between a struct tasks_struct *, and a int *, which can have even more devastating results.

It also just looks like a debug session instead of a trace marker.

Quoted 1 line · Steven Rostedt

[...] Format strings only help for printf like operations. [...]

That is not so. They are far from panaceanic, but printf formats are useful for type checked simple scalars, which we can extract and use for purposes other than printf like operations.

Quoted 5 lines

The best example of this is the sched_switch code. LTTng and friends just want a pid and comm to show. But there's tracers that want more info from the task_struct. We also like to see the priority of the task. [...]

This is the sort of information that can help generate a compromise. For this case, pass a few raw pointers that a compiled-in tracing engine can dereference at will, and *also pass* a few user-level scalars that a separately-compiled tracing engine can use.

Quoted 3 lines

Passing in a pointer to the structure being traced should be enough for all tracers.

On the contrary, we have explained why *this is not so*. Using raw general structure pointers in impractical for some tracers.

Quoted 6 lines

Now back to your question, why don't we like the printf format. Simply because it does nothing for pointers. It might help you with a %d and number of parameters, but a %p can not tell the difference between a struct tasks_struct *, and a int *, which can have even more devastating results.

Indeed. Unfortunately, C is not kind to us in the way that perhaps C++ templates could be. We have not yet seen a single mechanism that does all of:

- type-safe passing of arbitrary pointer/etc. types (so compiled-in trace data consumers can go wild with data passing)
- declarative / separately-compiled consumption of types (so lttng/systemtap/userspace can hook in without heroics)
- parsimonious implementation

Maybe a solution could involve some restrictions on the generalities. For example, can we narrow down the number of different scalar + pointer types to a fixed handful? Can we tolerate type-safety being provided by families of function declarations rather than one generic one?

Quoted 3 lines

It also just looks like a debug session instead of a trace marker.

Why do you think the difference between those is profound?

Quoted 5 lines · Frank Ch. Eigler

Quoted 1 line · Steven Rostedt

It also just looks like a debug session instead of a trace marker.

Why do you think the difference between those is profound?

Not that profound but I do find:

  trace_sched_switch(prev, next);

much nicer to look at than

  trace_mark("%p %p", prev, next);

The trace_sched_switch seems a bit more informative with a simple glance than the trace_mark.

[...] I think what Frank refers to here is why not scatter the kernel code with trace_mark()s on every conceivable location like you do with printk-style debugging.

Those trace marks might help out when $customer's kernel goes splat and you don't want to provide him with a custom kernel.

I do think we must make a clear distinction between these cases because:

1) tracers provide a kernel<->user interface - and whilst we don't have a stable in-kernel API/ABI we are anal about the kernel/user boundary. Andrew also greatly worries about this aspect.

2) it highly uglyfies the code, Frank doesn't need to maintain it, so its easy for him to say. But IMHO its much harder to read code that is littered with debug statements that it is to read regular code.

3) it bloats the kernel,.. while it may not be fast path bloat, all
that marker stuff does go somewhere.

So, while I see the value of 'stable' mark sites for 'stable' events, I'm dead-set against littering the kernel with markers just because we can, and hoping they might some day be useful for someone.

Quoted 5 lines

I do think we must make a clear distinction between these cases because:

1) tracers provide a kernel<->user interface - and whilst we don't have a stable in-kernel API/ABI we are anal about the kernel/user boundary. Andrew also greatly worries about this aspect.

Well, how to set Andrew's mind at ease then, beyond what we've already said? Back a few months ago, both systemtap and lttng guys - the primary user-space clients - have said that we are fine with this interface changing. We each have mechanisms to adapt.

Quoted 4 lines

2) it highly uglyfies the code, Frank doesn't need to maintain it, so its easy for him to say. But IMHO its much harder to read code that is littered with debug statements that it is to read regular code.

Then don't put too many in, or hide them with inline functions.

Quoted 2 lines

3) it bloats the kernel,.. while it may not be fast path bloat, all
that marker stuff does go somewhere.

That bloat has been quantified and appears negligible in space and time.

Quoted 4 lines

So, while I see the value of 'stable' mark sites for 'stable' events, I'm dead-set against littering the kernel with markers just because we can, and hoping they might some day be useful for someone.

We're in violent agreement. No one suggested "littering just because we can". The initial lttng suite of markers consisted of about one hundred *in total*. If some other subsystem maintainer runs amok and adds thousands, please take it up with them, not with us.

Peter, I thought, we were discussing what interface we could accept, not how many or where tracepoints we could accept. Or, am I misreading?

I know that if someone pushes markers into kernel in his own sweet way, of course, the kernel code will be bloated endlessly. But I also know why we review patches and send Ack/Nack before merging them to the tree. (If you still worry about it, we might be able to make linux-markers git tree, and review all regular markers on it)

[...]

Quoted 3 lines

That is not so. They are far from panaceanic, but printf formats are useful for type checked simple scalars, which we can extract and use for purposes other than printf like operations.

It doesn't help much if you mix a pid and prio of a task_struct, and then use the prio to find the actual task, or try setting another task priority to the pid.

[...]

Quoted 5 lines

Quoted 2 lines

Passing in a pointer to the structure being traced should be enough for all tracers.

On the contrary, we have explained why *this is not so*. Using raw general structure pointers in impractical for some tracers.

The thing that those tracers need is something that can be stored in the kernel that can easily extract the needed information.

[...]

Quoted 5 lines

Maybe a solution could involve some restrictions on the generalities. For example, can we narrow down the number of different scalar + pointer types to a fixed handful? Can we tolerate type-safety being provided by families of function declarations rather than one generic one?

I'm all for restricting this, I even suggested something similar a while ago (http://lists.openwall.net/linux-kernel/2006/10/07/21). No, I'm not pushing that solution, that solution was only to bring out more ideas.

Quoted 10 lines

Quoted 4 lines

By doing so, we could leave a gcc format string check by passing the format string to __mark_check_format(). We could extract the field names from the prototype, so there is no need to duplicate field information in the format string.

I thought that someone complained against those format strings in kernel code. Thus I removed it from DEFINE_TRACE.

even though, I think you can do that by adding below string table to LTTng module.

Quoted 4 lines

const char *lookup_table[MAX_MARKERS][2] = {
{"irq_entry", "%d %d"}, // or "(int irq_id, int kernel_mode)", "%d %d"
...
};

if move string to out of kernel core, compiler may kill some variable. thus, we will get incomplete tracing result.

All this work look good, thanks Masami! Sorry I did not find time to do it lately, I've been busy on other things. A small question though :
since LTTng is configurable both as an external module or as an in-kernel tracer, I wonder if it would really hurt to add the format strings to DEFINE_TRACE, e.g. :

DEFINE_TRACE(name, prototype, format_string, args...)

which would give :

DEFINE_TRACE(irq_entry, (int irq_id, int kernel_mode), "%d %d",

    irq_id, kernel_mode);

DEFINE_TRACE(irq_exit, (void), MARK_NOARGS);

and calling this in the kernel code :

trace_irq_entry(irq, (regs)?(!user_mode(regs)):(1));
...
trace_irq_exit();

and for quick-and-dirty debug usage, one would add this to kernel code :

trace_mark(subsystem_event, "(int arg, struct task_struct *task)",
"%d %p", arg, current);

Quoted 17 lines · Mathieu Desnoyers

All this work look good, thanks Masami! Sorry I did not find time to do it lately, I've been busy on other things. A small question though :
since LTTng is configurable both as an external module or as an in-kernel tracer, I wonder if it would really hurt to add the format strings to DEFINE_TRACE, e.g. :

DEFINE_TRACE(name, prototype, format_string, args...)

which would give :

DEFINE_TRACE(irq_entry, (int irq_id, int kernel_mode), "%d %d",

    irq_id, kernel_mode);

DEFINE_TRACE(irq_exit, (void), MARK_NOARGS);

and calling this in the kernel code :

Quoted 8 lines

trace_irq_entry(irq, (regs)?(!user_mode(regs)):(1));
...
trace_irq_exit();

and for quick-and-dirty debug usage, one would add this to kernel code :

trace_mark(subsystem_event, "(int arg, struct task_struct *task)",
"%d %p", arg, current);

How would this work for:

DEFINE_TRACE(sched_switch, (struct task_struct *prev, struct task_struct *next), prev, next);

You'd want a string like: "%d %d", prev->pid, next->pid
not: "%p %p", prev, next

perhaps we can do something like:

DEFINE_TRACER(sched_switch, (struct task_struct *prev, struct task_struct *next), prev, next,

		"%d %d", prev->pid, next->pid);

that defines a default tracer function for the previously defined trace point. That way its optional, and allows for generic trace points.

Of course, all this could be ruined by reality - C really sucks wrt forwarding functions.. :-/

Tracepoint proposal

- Tracepoint infrastructure
- In-kernel users
- Complete typing, verified by the compiler
- Dynamically linked and activated

- Marker infrastructure
- Exported API to userland
- Basic types only

- Dynamic vs static
- In-kernel probes are dynamically linked, dynamically activated, connected to tracepoints. Type verification is done at compile-time. Those in-kernel probes can be a probe extracting the information to put in a marker or a specific in-kernel tracer such as ftrace.
- Information sinks (LTTng, SystemTAP) are dynamically connected to the markers inserted in the probes and are dynamically activated.

- Near instrumentation site vs in a separate tracer module

A probe module, only if provided with the kernel tree, could connect to internal tracing sites. This argues for keeping the tracepoing probes near the instrumentation site code. However, if a tracer is general purpose and exports typing information to userspace through some mechanism, it should only export the "basic type" information and could be therefore shipped outside of the kernel tree.

In-kernel probes should be integrated to the kernel tree. They would be close to the instrumented kernel code and would translate between the in-kernel instrumentation and the "basic type" exports. Other in-kernel probes could provide a different output (statistics available through debugfs for instance). ftrace falls into this category.

Generic or specialized information "sinks" (LTTng, systemtap) could be connected to the markers put in tracepoint probes to extract the information to userspace. They would extract both typing information and the per-tracepoint execution information to userspace.

Therefore, the code would look like :

kernel/sched.c:

#include "sched-trace.h"

schedule()
{
...
trace_sched_switch(prev, next);
...
}

Quoted 43 lines · Mathieu Desnoyers

Tracepoint proposal

- Tracepoint infrastructure
- In-kernel users
- Complete typing, verified by the compiler
- Dynamically linked and activated

- Marker infrastructure
- Exported API to userland
- Basic types only

- Dynamic vs static
- In-kernel probes are dynamically linked, dynamically activated, connected to tracepoints. Type verification is done at compile-time. Those in-kernel probes can be a probe extracting the information to put in a marker or a specific in-kernel tracer such as ftrace.
- Information sinks (LTTng, SystemTAP) are dynamically connected to the markers inserted in the probes and are dynamically activated.

- Near instrumentation site vs in a separate tracer module

A probe module, only if provided with the kernel tree, could connect to internal tracing sites. This argues for keeping the tracepoing probes near the instrumentation site code. However, if a tracer is general purpose and exports typing information to userspace through some mechanism, it should only export the "basic type" information and could be therefore shipped outside of the kernel tree.

In-kernel probes should be integrated to the kernel tree. They would be close to the instrumented kernel code and would translate between the in-kernel instrumentation and the "basic type" exports. Other in-kernel probes could provide a different output (statistics available through debugfs for instance). ftrace falls into this category.

Generic or specialized information "sinks" (LTTng, systemtap) could be connected to the markers put in tracepoint probes to extract the information to userspace. They would extract both typing information and the per-tracepoint execution information to userspace.

Therefore, the code would look like :

kernel/sched.c:

Quoted 8 lines

#include "sched-trace.h"

schedule()
{
...
trace_sched_switch(prev, next);
...
}

Once this is accepted you're going to add hundreds of such calls to every core subsystem, right?

Quoted 53 lines · * Alexey Dobriyan ([email protected]) wrote:

Quoted 51 lines · Mathieu Desnoyers

Tracepoint proposal

- Tracepoint infrastructure
- In-kernel users
- Complete typing, verified by the compiler
- Dynamically linked and activated

- Marker infrastructure
- Exported API to userland
- Basic types only

- Dynamic vs static
- In-kernel probes are dynamically linked, dynamically activated, connected to tracepoints. Type verification is done at compile-time. Those in-kernel probes can be a probe extracting the information to put in a marker or a specific in-kernel tracer such as ftrace.
- Information sinks (LTTng, SystemTAP) are dynamically connected to the markers inserted in the probes and are dynamically activated.

- Near instrumentation site vs in a separate tracer module

A probe module, only if provided with the kernel tree, could connect to internal tracing sites. This argues for keeping the tracepoing probes near the instrumentation site code. However, if a tracer is general purpose and exports typing information to userspace through some mechanism, it should only export the "basic type" information and could be therefore shipped outside of the kernel tree.

In-kernel probes should be integrated to the kernel tree. They would be close to the instrumented kernel code and would translate between the in-kernel instrumentation and the "basic type" exports. Other in-kernel probes could provide a different output (statistics available through debugfs for instance). ftrace falls into this category.

Generic or specialized information "sinks" (LTTng, systemtap) could be connected to the markers put in tracepoint probes to extract the information to userspace. They would extract both typing information and the per-tracepoint execution information to userspace.

Therefore, the code would look like :

kernel/sched.c:

#include "sched-trace.h"

schedule()
{
...
trace_sched_switch(prev, next);
...
}

Quoted 3 lines

Once this is accepted you're going to add hundreds of such calls to every core subsystem, right?

The LTTng instrumentation has about 125 of such calls. Tests have revealed that adding such dormant tracepoints to the kernel often increase kernel performances rather than decreasing it (see the ia64 benchmarks posted on lkml a few weeks ago).

The core subsystem maintainers are being involved in the process.

Actually, marking up the source code has the interesting effect of letting knowledgeable people influence the trace point decisions.

Quoted 53 lines · Mathieu Desnoyers

Quoted 52 lines · * Alexey Dobriyan ([email protected]) wrote:

Quoted 51 lines · Mathieu Desnoyers

Tracepoint proposal

- Tracepoint infrastructure
- In-kernel users
- Complete typing, verified by the compiler
- Dynamically linked and activated

- Marker infrastructure
- Exported API to userland
- Basic types only

- Dynamic vs static
- In-kernel probes are dynamically linked, dynamically activated, connected to tracepoints. Type verification is done at compile-time. Those in-kernel probes can be a probe extracting the information to put in a marker or a specific in-kernel tracer such as ftrace.
- Information sinks (LTTng, SystemTAP) are dynamically connected to the markers inserted in the probes and are dynamically activated.

- Near instrumentation site vs in a separate tracer module

A probe module, only if provided with the kernel tree, could connect to internal tracing sites. This argues for keeping the tracepoing probes near the instrumentation site code. However, if a tracer is general purpose and exports typing information to userspace through some mechanism, it should only export the "basic type" information and could be therefore shipped outside of the kernel tree.

In-kernel probes should be integrated to the kernel tree. They would be close to the instrumented kernel code and would translate between the in-kernel instrumentation and the "basic type" exports. Other in-kernel probes could provide a different output (statistics available through debugfs for instance). ftrace falls into this category.

Generic or specialized information "sinks" (LTTng, systemtap) could be connected to the markers put in tracepoint probes to extract the information to userspace. They would extract both typing information and the per-tracepoint execution information to userspace.

Therefore, the code would look like :

kernel/sched.c:

#include "sched-trace.h"

schedule()
{
...
trace_sched_switch(prev, next);
...
}

Quoted 7 lines

Quoted 3 lines

Once this is accepted you're going to add hundreds of such calls to every core subsystem, right?

The LTTng instrumentation has about 125 of such calls. Tests have revealed that adding such dormant tracepoints to the kernel often increase kernel performances rather than decreasing it (see the ia64 benchmarks posted on lkml a few weeks ago).

We're not adding this for performance increase, you do realize this?

Quoted 1 line

The core subsystem maintainers are being involved in the process.

NAK this from proc if you about this.

Quoted 2 lines

Actually, marking up the source code has the interesting effect of letting knowledgeable people influence the trace point decisions.

I'd say that maximum source code overhead any tracing facility should be allowed is "__xxx" annotation at very start of function definition. Anything beyond should be rejected and there are good reasons for that.

One reason why we need markers or other in-the-middle-of-function trace point is that some events happen inside functions, not it's interface.

[...]
Each kernel sub-system seems to have its own way of dealing with debugging statements. Some of these methods include 'dprintk', 'pr_debug', 'dev_debug', 'DEBUGP'. I think that these functions are the tracepoints that has been availably mounted without setting up the tool set of the outside. I think whether mounting that unites these functions can be done if kernel marker and tracepoint are used.

By the way, isn't there problem on security? What kprobe, jprobe, and kernel marker, etc. offer looks like what the framework of Linux Security Module had offered before. Gotten kprobe, jprobe, and kernel marker, etc. should not be exported to the userland for security because it becomes the hotbed of rootkits.

Users such as kprobe, jprobe, and kernel marker should not be Loadable Kernel Module. [...]

Hi

Quoted 3 lines · Hiramatsu

One reason why we need markers or other in-the-middle-of-function trace point is that some events happen inside functions, not it's interface.

Each kernel sub-system seems to have its own way of dealing with
debugging statements. Some of these methods include 'dprintk', 'pr_debug', 'dev_debug', 'DEBUGP'. I think that these functions are the tracepoints that has been availably mounted without setting up the tool set of the outside. I think whether mounting that unites these functions can be done if kernel marker and tracepoint are used.

By the way, isn't there problem on security?
What kprobe, jprobe, and kernel marker, etc. offer looks like what
the framework of Linux Security Module had offered before. Gotten kprobe, jprobe, and kernel marker, etc. should not be exported to the

userland for security because it becomes the hotbed of rootkits. Users such as kprobe, jprobe, and kernel marker should not be Loadable Kernel Module. I think that there are some solutions in LTTng about this security problem. However, will the environment to be able to operate SystemTap be really secure?
　At least, kernel commandline option to invalidate all of kprobe, jprobe, and kernel marker, etc. because of the batch might be necessary.

Tracepoint
- stored in a section
- dynamic activation
- no message
- export kernel structure
- arguments depending on points
- high frequently called
- no module support (kernel use only)

Quoted 8 lines

Tracepoint
- stored in a section
- dynamic activation
- no message
- export kernel structure
- arguments depending on points
- high frequently called
- no module support (kernel use only)

Why Shouldn't kernel module use tracepoint?
technical problem? or any plicy exist?

Quoted 11 lines · KOSAKI Motohiro

Quoted 8 lines

Tracepoint
- stored in a section
- dynamic activation
- no message
- export kernel structure
- arguments depending on points
- high frequently called
- no module support (kernel use only)

Why Shouldn't kernel module use tracepoint?
technical problem? or any plicy exist?

Good question, I think we don't want to export so much kernel internal structures. Since tracepoint tend to export raw kernel structure to user module, I thought it might be better not to export its interface to modules.

However, I thought that again, and knew what Peter worried was about marker which exports marker list to user space. So, the issue is exporting internal structures to user space, not to modules. Thus, I think it can support modules also.

Thank you,

Signature · 7 lines

Masami Hiramatsu

Software Engineer
Hitachi Computer Products (America) Inc.
Software Solutions Division

e-mail: [email protected]

** note : this patch is submitted for early review. It applies after my current unreleased 2.6.26-rc8 LTTng patchset. Comments are welcome.

Implementation of kernel tracepoints. Inspired from the Linux Kernel Markers.

Allows complete typing verification. No format string required.

TODO : Documentation/tracepoint.txt

+/*
+ * Note : the empty asm volatile with read constraint is used here instead of a
+ * "used" attribute to fix a gcc 4.1.x bug.as
+ * Make sure the alignment of the structure in the __markers section will
+ * not add unwanted padding between the beginning of the section and the
+ * structure. Force alignment to the same alignment as the section start.

+ *
+ * The "generic" argument controls which marker enabling mechanism must be used.
+ * If generic is true, a variable read is used.
+ * If generic is false, immediate values are used.
+ */

include/linux/tracepoint.h new file +47 -0

+#define DEFINE_TRACE(name, proto, args)					\
+	static inline void _do_trace_##name(struct tracepoint *tp, proto) \
+	{								\
+		int i;							\
+		struct tracepoint_probe_closure *multi;			\
+		preempt_disable();					\
+		multi = tp->multi;					\
+		smp_read_barrier_depends();				\
+		if (multi) {						\
+			for (i = 0; multi[i].func; i++) {		\
+				((void(*)(void *private_data, proto))	\
+				(multi[i].func))(multi[i].probe_private, args);\
+			}						\
+		}							\
+		preempt_enable();					\
+	}								\
+	static inline void __trace_##name(int generic, proto)		\
+	{								\
+		static const char __tpstrtab_##name[]			\
+		__attribute__((section("__tracepoints_strings")))	\
+		= #name;						\
+		static struct tracepoint __tracepoint_##name		\
+		__attribute__((section("__tracepoints"), aligned(8))) =	\
+		{ __tpstrtab_##name, 0, NULL };				\
+		if (!generic) {						\
+			if (unlikely(imv_cond(__tracepoint_##name.state))) { \
+				imv_cond_end();				\
+				_do_trace_##name(&__tracepoint_##name, args); \
+			} else						\
+				imv_cond_end();				\
+		} else {						\
+			if (unlikely(_imv_read(__tracepoint_##name.state))) \
+				_do_trace_##name(&__tracepoint_##name, args); \
+		}							\
+	}								\
+	static inline void trace_##name(proto)				\
+	{								\
+		__trace_##name(0, args);				\
+	}								\
+	static inline void _trace_##name(proto)				\
+	{								\
+		__trace_##name(1, args);				\
+	}								\
+	static inline int register_trace_##name(			\
+		void (*probe)(void *private_data, proto),		\
+		void *private_data)					\
+	{								\

include/linux/tracepoint.h new file +4 -0

+
+extern void tracepoint_update_probe_range(struct tracepoint *begin,
+	struct tracepoint *end);
+

include/linux/tracepoint.h new file +23 -0

+
+static inline void tracepoint_update_probe_range(struct tracepoint *begin,
+	struct tracepoint *end)
+{ }
+#endif /* CONFIG_TRACEPOINTS */
+
+/*
+ * Connect a probe to a tracepoint.
+ * Internal API, should not be used directly.
+ */
+extern int tracepoint_probe_register(const char *name,
+	void *probe, void *probe_private);
+
+/*
+ * Disconnect a probe from a tracepoint.
+ * Internal API, should not be used directly.
+ */
+extern int tracepoint_probe_unregister(const char *name,
+	void *probe, void *probe_private);
+
+struct tracepoint_iter {
+	struct module *module;
+	struct tracepoint *tracepoint;

Hi Mathieu,

Thank you for making this so soon!

Quoted 2 lines · Mathieu Desnoyers

** note : this patch is submitted for early review. It applies after my current unreleased 2.6.26-rc8 LTTng patchset. Comments are welcome.

Would you mean there is no tree on which we can test this patch?

Quoted 2 lines

Implementation of kernel tracepoints. Inspired from the Linux Kernel Markers.

What would you think redesigning markers on tracepoints? because most of the logic (scaning sections, multiple probe and activation) seems very similar to markers.

For example, (not complete, I just thought :-))

struct tracepoint {

(quoted diff) +0 -0

 	const char *name;		/* Tracepoint name */
 	DEFINE_IMV(char, state);	/* Immediate value state. */
 	struct tracepoint_probe_closure *multi;	/* Closures */

void * callsite_data; /* private date from call site */
} __attribute__((aligned(8)));

#define __tracepoint_block(generic, name, data, func, args)

(quoted diff) +0 -0

 	static const char __tpstrtab_##name[]			\
 	__attribute__((section("__tracepoints_strings")))	\
 	= #name;						\
 	static struct tracepoint __tracepoint_##name		\
 	__attribute__((section("__tracepoints"), aligned(8))) =	\
 	{ __tpstrtab_##name, 0, NULL, data};			\
 	if (!generic) {						\
 		if (unlikely(imv_cond(__tracepoint_##name.state))) { \
 			imv_cond_end();				\
 			func(&__tracepoint_##name, args); \
 		} else						\
 			imv_cond_end();				\
 	} else {						\
 		if (unlikely(_imv_read(__tracepoint_##name.state))) \
 			func(&__tracepoint_##name, args); \
 	}
 
 struct marker {
        const char *name;       /* Marker name */
        const char *format;     /* Marker format string, describing the
                                 * variable argument list.
                                 */
 } __attribute__((aligned(8)));
 
 #define trace_mark(name, fmt, args...)			\
 do {							\
 	static const char __mstrtab_##name[]		\
 	__attribute__((section("__markers_strings")))	\
 	= #name "\0" fmt;				\
 	static struct marker __marker_##name		\
 	__attribute__((section("__markers"), aligned(8))) =	\
 	{ __mstrtab_##name, &__mstrtab_##name[sizeof(#name)]};	\ 	
 	__tracepoint_block(1, name, __marker_##name, marker_probe_cb, args)	\
 } while (0)

Quoted 5 lines

Allows complete typing verification. No format string required.

TODO : Documentation/tracepoint.txt

[...]

Quoted 6 lines

+/*
+ * Note : the empty asm volatile with read constraint is used here instead of a
+ * "used" attribute to fix a gcc 4.1.x bug.as
+ * Make sure the alignment of the structure in the __markers section will
+ * not add unwanted padding between the beginning of the section and the
+ * structure. Force alignment to the same alignment as the section start.

this comment should be updated...

Quoted 52 lines

+ *
+ * The "generic" argument controls which marker enabling mechanism must be used.
+ * If generic is true, a variable read is used.
+ * If generic is false, immediate values are used.
+ */

include/linux/tracepoint.h new file +47 -0

+#define DEFINE_TRACE(name, proto, args)					\
+	static inline void _do_trace_##name(struct tracepoint *tp, proto) \
+	{								\
+		int i;							\
+		struct tracepoint_probe_closure *multi;			\
+		preempt_disable();					\
+		multi = tp->multi;					\
+		smp_read_barrier_depends();				\
+		if (multi) {						\
+			for (i = 0; multi[i].func; i++) {		\
+				((void(*)(void *private_data, proto))	\
+				(multi[i].func))(multi[i].probe_private, args);\
+			}						\
+		}							\
+		preempt_enable();					\
+	}								\
+	static inline void __trace_##name(int generic, proto)		\
+	{								\
+		static const char __tpstrtab_##name[]			\
+		__attribute__((section("__tracepoints_strings")))	\
+		= #name;						\
+		static struct tracepoint __tracepoint_##name		\
+		__attribute__((section("__tracepoints"), aligned(8))) =	\
+		{ __tpstrtab_##name, 0, NULL };				\
+		if (!generic) {						\
+			if (unlikely(imv_cond(__tracepoint_##name.state))) { \
+				imv_cond_end();				\
+				_do_trace_##name(&__tracepoint_##name, args); \
+			} else						\
+				imv_cond_end();				\
+		} else {						\
+			if (unlikely(_imv_read(__tracepoint_##name.state))) \
+				_do_trace_##name(&__tracepoint_##name, args); \
+		}							\
+	}								\
+	static inline void trace_##name(proto)				\
+	{								\
+		__trace_##name(0, args);				\
+	}								\
+	static inline void _trace_##name(proto)				\
+	{								\
+		__trace_##name(1, args);				\
+	}								\
+	static inline int register_trace_##name(			\
+		void (*probe)(void *private_data, proto),		\
+		void *private_data)					\
+	{								\

include/linux/tracepoint.h line 107

Quoted 10 lines

include/linux/tracepoint.h new file +10 -0

+		return tracepoint_probe_register(#name, (void *)probe,	\
+			private_data);					\
+	}								\
+	static inline void unregister_trace_##name(			\
+		void (*probe)(void *private_data, proto),		\
+		void *private_data)					\
+	{								\
+		tracepoint_probe_unregister(#name, (void *)probe,	\
+			private_data);					\
+	}

Out of curiousity, what the private_data is for?

Quoted 4 lines

include/linux/tracepoint.h new file +4 -0

+
+extern void tracepoint_update_probe_range(struct tracepoint *begin,
+	struct tracepoint *end);
+

include/linux/tracepoint.h line 121

Quoted 10 lines

include/linux/tracepoint.h new file +10 -0

+#else /* !CONFIG_TRACEPOINTS */
+#define DEFINE_TRACE(name, proto, args)			\
+	static inline void _do_trace_##name(struct tracepoint *tp, proto) \
+	{ }								\
+	static inline void __trace_##name(int generic, proto)		\
+	{ }								\
+	static inline void trace_##name(proto)				\
+	{ }								\
+	static inline void _trace_##name(proto)				\
+	{ }

By the way, I think you'd better add below two inlines.

	static inline int register_trace_##name(			\
		void (*probe)(void *private_data, proto),		\
		void *private_data)					\
	{ return -ENOSYS; }
	static inline void unregister_trace_##name(			\
		void (*probe)(void *private_data, proto),		\
		void *private_data)					\
	{ }

Quoted 23 lines

include/linux/tracepoint.h new file +23 -0

+
+static inline void tracepoint_update_probe_range(struct tracepoint *begin,
+	struct tracepoint *end)
+{ }
+#endif /* CONFIG_TRACEPOINTS */
+
+/*
+ * Connect a probe to a tracepoint.
+ * Internal API, should not be used directly.
+ */
+extern int tracepoint_probe_register(const char *name,
+	void *probe, void *probe_private);
+
+/*
+ * Disconnect a probe from a tracepoint.
+ * Internal API, should not be used directly.
+ */
+extern int tracepoint_probe_unregister(const char *name,
+	void *probe, void *probe_private);
+
+struct tracepoint_iter {
+	struct module *module;
+	struct tracepoint *tracepoint;

include/linux/tracepoint.h line 154

Quoted 10 lines

include/linux/tracepoint.h new file +10 -0

+};
+
+extern void tracepoint_iter_start(struct tracepoint_iter *iter);
+extern void tracepoint_iter_next(struct tracepoint_iter *iter);
+extern void tracepoint_iter_stop(struct tracepoint_iter *iter);
+extern void tracepoint_iter_reset(struct tracepoint_iter *iter);
+extern int tracepoint_get_iter_range(struct tracepoint **tracepoint,
+	struct tracepoint *begin, struct tracepoint *end);
+
+#endif

[...]

Best regards,

Signature · 7 lines

Masami Hiramatsu

Software Engineer
Hitachi Computer Products (America) Inc.
Software Solutions Division

e-mail: [email protected]

Quoted 4 lines · * Masami Hiramatsu ([email protected]) wrote:

Hi Mathieu,

Thank you for making this so soon!

Hi Masami,

Thanks for the comments, I will rework the patch accordingly.

Also, one thing I thought about yesterday which I dislike is that if we have two modules declaring the same tracepoint in different headers with different prototypes, each declaration will be valid but the registration will try to connect a probe expecting wrong parameters to the other tracepoint.

It would be the case if someone does :

drivers/somedrivera/mydriver1-trace.h

DECLARE_TRACE(really_generic_name, TPPTOTO(void), TPARGS()));

drivers/somedriverb/mydriver2-trace.h

DECLARE_TRACE(really_generic_name, TPPTOTO(struct somestruct *s), TPARGS(s)));

Do you think it's worth it to append the prototype string to the tracepoint name ? I think it should fix the problem.

Quoted 25 lines · Mathieu Desnoyers

Quoted 4 lines · * Masami Hiramatsu ([email protected]) wrote:

Hi Mathieu,

Thank you for making this so soon!

Hi Masami,

Thanks for the comments, I will rework the patch accordingly.

Also, one thing I thought about yesterday which I dislike is that if we have two modules declaring the same tracepoint in different headers with different prototypes, each declaration will be valid but the registration will try to connect a probe expecting wrong parameters to the other tracepoint.

It would be the case if someone does :

drivers/somedrivera/mydriver1-trace.h

DECLARE_TRACE(really_generic_name, TPPTOTO(void), TPARGS()));

drivers/somedriverb/mydriver2-trace.h

Quoted 4 lines

DECLARE_TRACE(really_generic_name, TPPTOTO(struct somestruct *s), TPARGS(s)));

Do you think it's worth it to append the prototype string to the tracepoint name ? I think it should fix the problem.

Hmm, I think we'd better send a fix patch to them in that case. (I hope we can find that kind of conflicts soon)
I think we can make an external tool which detect those conflicts.

Anyway, signature based checking idea is good to me. I think ":" is better delimiter.

Thank you,

Signature · 7 lines

Masami Hiramatsu

Software Engineer
Hitachi Computer Products (America) Inc.
Software Solutions Division

e-mail: [email protected]

Hi Mathieu,

Thank you for making this so soon!

Quoted 2 lines · Mathieu Desnoyers

** note : this patch is submitted for early review. It applies after my current unreleased 2.6.26-rc8 LTTng patchset. Comments are welcome.

Would you mean there is no tree on which we can test this patch?

Quoted 5 lines · * Masami Hiramatsu ([email protected]) wrote:

Hi Mathieu,

Thank you for making this so soon!

Mathieu Desnoyers wrote:

Quoted 4 lines

Quoted 2 lines

** note : this patch is submitted for early review. It applies after my current unreleased 2.6.26-rc8 LTTng patchset. Comments are welcome.

Would you mean there is no tree on which we can test this patch?

It actually applies on top of the following patchset :

http://ltt.polymtl.ca/lttng/patch-2.6.26-rc8-0.10-pre56.tar.bz2

Quoted 2 lines

Implementation of kernel tracepoints. Inspired from the Linux Kernel Markers.

What would you think redesigning markers on tracepoints? because most of the logic (scaning sections, multiple probe and activation) seems very similar to markers.

From what I see in your proposal, it's mostly to extract the if() call()

For example, (not complete, I just thought :-))

struct tracepoint {

(quoted diff) +0 -0

 	const char *name;		/* Tracepoint name */
 	DEFINE_IMV(char, state);	/* Immediate value state. */
 	struct tracepoint_probe_closure *multi;	/* Closures */

void * callsite_data; /* private date from call site */
} __attribute__((aligned(8)));

#define __tracepoint_block(generic, name, data, func, args)

(quoted diff) +0 -0

 	static const char __tpstrtab_##name[]			\
 	__attribute__((section("__tracepoints_strings")))	\
 	= #name;						\
 	static struct tracepoint __tracepoint_##name		\
 	__attribute__((section("__tracepoints"), aligned(8))) =	\
 	{ __tpstrtab_##name, 0, NULL, data};			\
 	if (!generic) {						\
 		if (unlikely(imv_cond(__tracepoint_##name.state))) { \
 			imv_cond_end();				\
 			func(&__tracepoint_##name, args); \
 		} else						\
 			imv_cond_end();				\
 	} else {						\
 		if (unlikely(_imv_read(__tracepoint_##name.state))) \
 			func(&__tracepoint_##name, args); \
 	}
 
 struct marker {
        const char *name;       /* Marker name */
        const char *format;     /* Marker format string, describing the
                                 * variable argument list.
                                 */
 } __attribute__((aligned(8)));
 
 #define trace_mark(name, fmt, args...)			\
 do {							\
 	static const char __mstrtab_##name[]		\
 	__attribute__((section("__markers_strings")))	\
 	= #name "\0" fmt;				\
 	static struct marker __marker_##name		\
 	__attribute__((section("__markers"), aligned(8))) =	\
 	{ __mstrtab_##name, &__mstrtab_##name[sizeof(#name)]};	\ 	
 	__tracepoint_block(1, name, __marker_##name, marker_probe_cb, args)	\
 } while (0)

Quoted 2 lines

Quoted 8 lines

include/linux/tracepoint.h new file +8 -0

+	static inline int register_trace_##name(			\
+		void (*probe)(void *private_data, proto),		\
+		void *private_data)					\
+	{								\
+		return tracepoint_probe_register(#name, (void *)probe,	\
+			private_data);					\
+	}								\
+	static inline void unregister_trace_##name(			\

include/linux/tracepoint.h line 107

Quoted 6 lines

include/linux/tracepoint.h new file +6 -0

+		void (*probe)(void *private_data, proto),		\
+		void *private_data)					\
+	{								\
+		tracepoint_probe_unregister(#name, (void *)probe,	\
+			private_data);					\
+	}

Out of curiousity, what the private_data is for?

Quoted 4 lines

include/linux/tracepoint.h new file +4 -0

+
+extern void tracepoint_update_probe_range(struct tracepoint *begin,
+	struct tracepoint *end);
+

include/linux/tracepoint.h line 121

Quoted 10 lines

include/linux/tracepoint.h new file +10 -0

+#else /* !CONFIG_TRACEPOINTS */
+#define DEFINE_TRACE(name, proto, args)			\
+	static inline void _do_trace_##name(struct tracepoint *tp, proto) \
+	{ }								\
+	static inline void __trace_##name(int generic, proto)		\
+	{ }								\
+	static inline void trace_##name(proto)				\
+	{ }								\
+	static inline void _trace_##name(proto)				\
+	{ }

By the way, I think you'd better add below two inlines.

	static inline int register_trace_##name(			\
		void (*probe)(void *private_data, proto),		\
		void *private_data)					\
	{ return -ENOSYS; }
	static inline void unregister_trace_##name(			\
		void (*probe)(void *private_data, proto),		\
		void *private_data)					\
	{ }

Quoted 6 lines · * Masami Hiramatsu ([email protected]) wrote:

Quoted 2 lines

Implementation of kernel tracepoints. Inspired from the Linux Kernel Markers.

What would you think redesigning markers on tracepoints? because most of the logic (scaning sections, multiple probe and activation) seems very similar to markers.

We could, although markers, because they use var args, allow to put the iteration on the multi probe array out-of-line. Tracepoints cannot afford this and the iteration must be done at the initial call-site.

From what I see in your proposal, it's mostly to extract the if() call() code from the inner __trace_mark() macro and to put it in a separate macro, am I correct ? This would make the macro more readable.

Quoted 18 lines

For example, (not complete, I just thought :-))

struct tracepoint {

(quoted diff) +0 -0

 	const char *name;		/* Tracepoint name */
 	DEFINE_IMV(char, state);	/* Immediate value state. */
 	struct tracepoint_probe_closure *multi;	/* Closures */

void * callsite_data; /* private date from call site */
} __attribute__((aligned(8)));

#define __tracepoint_block(generic, name, data, func, args)

(quoted diff) +0 -0

 	static const char __tpstrtab_##name[]			\
 	__attribute__((section("__tracepoints_strings")))	\
 	= #name;						\
 	static struct tracepoint __tracepoint_##name		\
 	__attribute__((section("__tracepoints"), aligned(8))) =	\
 	{ __tpstrtab_##name, 0, NULL, data};			\
 	if (!generic) {						\
 		if (unlikely(imv_cond(__tracepoint_##name.state))) { \

Review

Quoted 8 lines

(quoted diff) +0 -0

 			imv_cond_end();				\
 			func(&__tracepoint_##name, args); \
 		} else						\
 			imv_cond_end();				\
 	} else {						\
 		if (unlikely(_imv_read(__tracepoint_##name.state))) \
 			func(&__tracepoint_##name, args); \
 	}

[...]

Quoted 7 lines

Quoted 7 lines

include/linux/tracepoint.h new file +7 -0

+	static inline int register_trace_##name(			\
+		void (*probe)(void *private_data, proto),		\
+		void *private_data)					\
+	{								\
+		return tracepoint_probe_register(#name, (void *)probe,	\
+			private_data);					\
+	}								\

Quoted 9 lines

include/linux/tracepoint.h line 107

Quoted 7 lines

include/linux/tracepoint.h new file +7 -0

+	static inline void unregister_trace_##name(			\
+		void (*probe)(void *private_data, proto),		\
+		void *private_data)					\
+	{								\
+		tracepoint_probe_unregister(#name, (void *)probe,	\
+			private_data);					\
+	}

Out of curiousity, what the private_data is for?

When a probe is registered, it gives more flexibility to be able to pass a pointer to private data associated with that probe. For instance, if a tracer needs to register the same probe to many different tracepoints, but having a different context associated with each, it will pass the same function pointer with different private_data to the registration function.

Quoted 12 lines · Mathieu Desnoyers

Quoted 6 lines · * Masami Hiramatsu ([email protected]) wrote:

Quoted 2 lines

Implementation of kernel tracepoints. Inspired from the Linux Kernel Markers.

What would you think redesigning markers on tracepoints? because most of the logic (scaning sections, multiple probe and activation) seems very similar to markers.

We could, although markers, because they use var args, allow to put the iteration on the multi probe array out-of-line. Tracepoints cannot afford this and the iteration must be done at the initial call-site.

Quoted 3 lines

From what I see in your proposal, it's mostly to extract the if() call() code from the inner __trace_mark() macro and to put it in a separate macro, am I correct ? This would make the macro more readable.

Sure, I think marker and tracepoint can share below functions;
- definition of static local variables in specific sections

- probe activation code (if() call())
- multi probe handling

Then, marker just exports marker_strings sections.

Quoted 18 lines

Quoted 18 lines

For example, (not complete, I just thought :-))

struct tracepoint {

(quoted diff) +0 -0

 	const char *name;		/* Tracepoint name */
 	DEFINE_IMV(char, state);	/* Immediate value state. */
 	struct tracepoint_probe_closure *multi;	/* Closures */

void * callsite_data; /* private date from call site */
} __attribute__((aligned(8)));

#define __tracepoint_block(generic, name, data, func, args)

(quoted diff) +0 -0

 	static const char __tpstrtab_##name[]			\
 	__attribute__((section("__tracepoints_strings")))	\
 	= #name;						\
 	static struct tracepoint __tracepoint_##name		\
 	__attribute__((section("__tracepoints"), aligned(8))) =	\
 	{ __tpstrtab_##name, 0, NULL, data};			\
 	if (!generic) {						\
 		if (unlikely(imv_cond(__tracepoint_##name.state))) { \

Quoted 8 lines

Review

Quoted 8 lines

(quoted diff) +0 -0

 			imv_cond_end();				\
 			func(&__tracepoint_##name, args); \
 		} else						\
 			imv_cond_end();				\
 	} else {						\
 		if (unlikely(_imv_read(__tracepoint_##name.state))) \
 			func(&__tracepoint_##name, args); \
 	}

So, in my idea, __trace_##name() also uses __tracepoint_block() for avoiding code duplication.

Quoted 18 lines

[...]

Quoted 7 lines

Quoted 7 lines

include/linux/tracepoint.h new file +7 -0

+	static inline int register_trace_##name(			\
+		void (*probe)(void *private_data, proto),		\
+		void *private_data)					\
+	{								\
+		return tracepoint_probe_register(#name, (void *)probe,	\
+			private_data);					\
+	}								\

Quoted 9 lines

include/linux/tracepoint.h line 107

Quoted 7 lines

include/linux/tracepoint.h new file +7 -0

+	static inline void unregister_trace_##name(			\
+		void (*probe)(void *private_data, proto),		\
+		void *private_data)					\
+	{								\
+		tracepoint_probe_unregister(#name, (void *)probe,	\
+			private_data);					\
+	}

Out of curiousity, what the private_data is for?

Quoted 6 lines

When a probe is registered, it gives more flexibility to be able to pass a pointer to private data associated with that probe. For instance, if a tracer needs to register the same probe to many different tracepoints, but having a different context associated with each, it will pass the same function pointer with different private_data to the registration function.

Hmm, only for tracepoint, it might be not so useful, because most of tracepoint's prototypes are different and so we can't use same probe to those tracepoints.
Anyway, it is useful for more general probe(ex. markers) if that is implemented on tracepoint ;-)

Thank you,

Signature · 7 lines

Masami Hiramatsu

Software Engineer
Hitachi Computer Products (America) Inc.
Software Solutions Division

e-mail: [email protected]

Quoted 15 lines · * Masami Hiramatsu ([email protected]) wrote:

Quoted 14 lines · Mathieu Desnoyers

Quoted 6 lines · * Masami Hiramatsu ([email protected]) wrote:

Quoted 2 lines

Implementation of kernel tracepoints. Inspired from the Linux Kernel Markers.

What would you think redesigning markers on tracepoints? because most of the logic (scaning sections, multiple probe and activation) seems very similar to markers.

We could, although markers, because they use var args, allow to put the iteration on the multi probe array out-of-line. Tracepoints cannot afford this and the iteration must be done at the initial call-site.

From what I see in your proposal, it's mostly to extract the if() call() code from the inner __trace_mark() macro and to put it in a separate macro, am I correct ? This would make the macro more readable.

Quoted 2 lines

Sure, I think marker and tracepoint can share below functions;
- definition of static local variables in specific sections

Given that we could want to keep activation of tracepoints and markers separate (so they don't share the same namespace), declaring the static variables in separated sections seems to make sense to me.

Quoted 2 lines

- probe activation code (if() call())
- multi probe handling

Hrm, the thing here is that because markers allow to do the iteration on the multiple probe callbacks within an internal wrapper (instead of doing it on-site as in the tracepoints), it allows to do some further optimizations (less memory allocation and less pointer dereference in the single probe case, not having to prepare the va_args in the MARK_NOARGS case) which are only done because it does not have to add code to the instrumentation site. However, tracepoints cannot have such "generic" wrapper and we have to do the iteration on callbacks in the code added to the instrumented object. Therefore, I keep it as small as possible in terms of bytes of instructions.

Hi Mathieu,

Quoted 19 lines · Mathieu Desnoyers

Quoted 17 lines · * Masami Hiramatsu ([email protected]) wrote:

Quoted 7 lines · Mathieu Desnoyers

Quoted 6 lines · * Masami Hiramatsu ([email protected]) wrote:

Quoted 2 lines

Implementation of kernel tracepoints. Inspired from the Linux Kernel Markers.

What would you think redesigning markers on tracepoints? because most of the logic (scaning sections, multiple probe and activation) seems very similar to markers.

Quoted 7 lines

We could, although markers, because they use var args, allow to put the iteration on the multi probe array out-of-line. Tracepoints cannot afford this and the iteration must be done at the initial call-site.

From what I see in your proposal, it's mostly to extract the if() call() code from the inner __trace_mark() macro and to put it in a separate macro, am I correct ? This would make the macro more readable.

Sure, I think marker and tracepoint can share below functions;
- definition of static local variables in specific sections

Quoted 3 lines

Given that we could want to keep activation of tracepoints and markers separate (so they don't share the same namespace), declaring the static variables in separated sections seems to make sense to me.

Sorry, I'm not sure what is "separate activation".
As far as I can see, both tracepoints and markers are activated when its probe handlers are registered on each tracepoint/marker. Aren't it separated?

I did not mean integrating registering interfaces, but
I think that they can share base(internal) functions.
for example, both of them has XXX_update_range/_module_XXX_update etc.

IMHO, current code is not so good for maintenance. there are many code duplications (ex. kernel/module.c, I think that both of them (and imv too?) can share the code for handling its section and iterating entries). I'm not sure those duplications are acceptable.

Quoted 4 lines

Quoted 3 lines

- probe activation code (if() call())
- multi probe handling

Quoted 10 lines

Hrm, the thing here is that because markers allow to do the iteration on the multiple probe callbacks within an internal wrapper (instead of doing it on-site as in the tracepoints), it allows to do some further optimizations (less memory allocation and less pointer dereference in the single probe case, not having to prepare the va_args in the MARK_NOARGS case) which are only done because it does not have to add code to the instrumentation site. However, tracepoints cannot have such "generic" wrapper and we have to do the iteration on callbacks in the code added to the instrumented object. Therefore, I keep it as small as possible in terms of bytes of instructions.

OK, I see. So, __tracepoint_block() macro can specify handler function. what would you think about it?

Thank you,

Signature · 7 lines

Masami Hiramatsu

Software Engineer
Hitachi Computer Products (America) Inc.
Software Solutions Division

e-mail: [email protected]

Quoted 21 lines · * Masami Hiramatsu ([email protected]) wrote:

Hi Mathieu,

Quoted 18 lines · Mathieu Desnoyers

Quoted 17 lines · * Masami Hiramatsu ([email protected]) wrote:

Quoted 7 lines · Mathieu Desnoyers

Quoted 6 lines · * Masami Hiramatsu ([email protected]) wrote:

Quoted 2 lines

Implementation of kernel tracepoints. Inspired from the Linux Kernel Markers.

What would you think redesigning markers on tracepoints? because most of the logic (scaning sections, multiple probe and activation) seems very similar to markers.

Quoted 7 lines

We could, although markers, because they use var args, allow to put the iteration on the multi probe array out-of-line. Tracepoints cannot afford this and the iteration must be done at the initial call-site.

From what I see in your proposal, it's mostly to extract the if() call() code from the inner __trace_mark() macro and to put it in a separate macro, am I correct ? This would make the macro more readable.

Sure, I think marker and tracepoint can share below functions;
- definition of static local variables in specific sections

Quoted 8 lines

Quoted 3 lines

Given that we could want to keep activation of tracepoints and markers separate (so they don't share the same namespace), declaring the static variables in separated sections seems to make sense to me.

Sorry, I'm not sure what is "separate activation".
As far as I can see, both tracepoints and markers are activated when its probe handlers are registered on each tracepoint/marker. Aren't it separated?

Yes, it is separate. This is insured by the fact that markers and tracepoints are declared in different sections. Therefore, even if they have the same "name", they won't be used by each other.

Quoted 12 lines

Quoted 12 lines

Quoted 2 lines

- probe activation code (if() call())
- multi probe handling

Hrm, the thing here is that because markers allow to do the iteration on the multiple probe callbacks within an internal wrapper (instead of doing it on-site as in the tracepoints), it allows to do some further optimizations (less memory allocation and less pointer dereference in the single probe case, not having to prepare the va_args in the MARK_NOARGS case) which are only done because it does not have to add code to the instrumentation site. However, tracepoints cannot have such "generic" wrapper and we have to do the iteration on callbacks in the code added to the instrumented object. Therefore, I keep it as small as possible in terms of bytes of instructions.

Quoted 3 lines

OK, I see. So, __tracepoint_block() macro can specify handler function. what would you think about it?

When I originally designed the markers, I tried to make sure there was absolutely no code duplication until I discovered that trying to read a huge amount of nested macros is just a pain starting from a certain level. If we only save a few duplicated lines but end up tying up markers and tracepoints, I am far from certain that it will make the code more readable.

I'll post a tracepoint version with the modifications you proposed (it's now placed earlier in the patchset), except the merge with markers.

kernel/tracepoint.c new file +12 -0

+/*
+ * Remove the tracepoint from the tracepoint hash table. Must be called with
+ * mutex_lock held.
+ */
+static int remove_tracepoint(const char *name)
+{
+	struct hlist_head *head;
+	struct hlist_node *node;
+	struct tracepoint_entry *e;
+	int found = 0;
+	size_t len = strlen(name) + 1;
+	u32 hash = jhash(name, len-1, 0);

Hi Mathieu,

I found a bug.
Mathieu Desnoyers wrote:
[...]

Quoted 12 lines

kernel/tracepoint.c new file +12 -0

+/*
+ * Remove the tracepoint from the tracepoint hash table. Must be called with
+ * mutex_lock held.
+ */
+static int remove_tracepoint(const char *name)
+{
+	struct hlist_head *head;
+	struct hlist_node *node;
+	struct tracepoint_entry *e;
+	int found = 0;
+	size_t len = strlen(name) + 1;
+	u32 hash = jhash(name, len-1, 0);

kernel/tracepoint.c line 245

Quoted 10 lines

kernel/tracepoint.c new file +10 -0

+
+	head = &tracepoint_table[hash & ((1 << TRACEPOINT_HASH_BITS)-1)];
+	hlist_for_each_entry(e, node, head, hlist) {
+		if (!strcmp(name, e->name)) {
+			found = 1;
+			break;
+		}
+	}
+	if (!found)
+		return -ENOENT;

before removing, you have to ensure refcount == 0. So,

	if (e->refcount != 0)
		return -EBUSY;

kernel/tracepoint.c line 252

Quoted 7 lines

kernel/tracepoint.c new file +7 -0

+	hlist_del(&e->hlist);
+	/* Make sure the call_rcu has been executed */
+	if (e->rcu_pending)
+		rcu_barrier();
+	kfree(e);
+	return 0;
+}

Thank you,

Signature · 7 lines

Masami Hiramatsu

Software Engineer
Hitachi Computer Products (America) Inc.
Software Solutions Division

e-mail: [email protected]

Hi Mathieu,

I found a bug.
Mathieu Desnoyers wrote:
[...]

Quoted 12 lines

kernel/tracepoint.c new file +12 -0

+/*
+ * Remove the tracepoint from the tracepoint hash table. Must be called with
+ * mutex_lock held.
+ */
+static int remove_tracepoint(const char *name)
+{
+	struct hlist_head *head;
+	struct hlist_node *node;
+	struct tracepoint_entry *e;
+	int found = 0;
+	size_t len = strlen(name) + 1;
+	u32 hash = jhash(name, len-1, 0);

kernel/tracepoint.c line 245

Quoted 10 lines

kernel/tracepoint.c new file +10 -0

+
+	head = &tracepoint_table[hash & ((1 << TRACEPOINT_HASH_BITS)-1)];
+	hlist_for_each_entry(e, node, head, hlist) {
+		if (!strcmp(name, e->name)) {
+			found = 1;
+			break;
+		}
+	}
+	if (!found)
+		return -ENOENT;

before removing, you have to ensure refcount == 0. So,

	if (e->refcount != 0)
		return -EBUSY;

kernel/tracepoint.c line 252

Quoted 7 lines

kernel/tracepoint.c new file +7 -0

+	hlist_del(&e->hlist);
+	/* Make sure the call_rcu has been executed */
+	if (e->rcu_pending)
+		rcu_barrier();
+	kfree(e);
+	return 0;
+}

Thank you,

Signature · 7 lines

Masami Hiramatsu

Software Engineer
Hitachi Computer Products (America) Inc.
Software Solutions Division

e-mail: [email protected]

Hi Masami,

I am thinking about removing the private_data, because such declaration won't work :

DEFINE_TRACE(kernel_irq_exit,

        TPPROTO(void),
        TPARGS());

because the arguments become :

(void *private_data, void)

Any better idea ?

Mathieu

Quoted 50 lines · * Masami Hiramatsu ([email protected]) wrote:

Hi Mathieu,

I found a bug.
Mathieu Desnoyers wrote:
[...]

Quoted 12 lines

kernel/tracepoint.c new file +12 -0

+/*
+ * Remove the tracepoint from the tracepoint hash table. Must be called with
+ * mutex_lock held.
+ */
+static int remove_tracepoint(const char *name)
+{
+	struct hlist_head *head;
+	struct hlist_node *node;
+	struct tracepoint_entry *e;
+	int found = 0;
+	size_t len = strlen(name) + 1;
+	u32 hash = jhash(name, len-1, 0);

kernel/tracepoint.c line 245

Quoted 10 lines

kernel/tracepoint.c new file +10 -0

+
+	head = &tracepoint_table[hash & ((1 << TRACEPOINT_HASH_BITS)-1)];
+	hlist_for_each_entry(e, node, head, hlist) {
+		if (!strcmp(name, e->name)) {
+			found = 1;
+			break;
+		}
+	}
+	if (!found)
+		return -ENOENT;

before removing, you have to ensure refcount == 0. So,

	if (e->refcount != 0)
		return -EBUSY;

kernel/tracepoint.c line 252

Quoted 7 lines

kernel/tracepoint.c new file +7 -0

+	hlist_del(&e->hlist);
+	/* Make sure the call_rcu has been executed */
+	if (e->rcu_pending)
+		rcu_barrier();
+	kfree(e);
+	return 0;
+}

Thank you,

Signature · 7 lines

Masami Hiramatsu

Software Engineer
Hitachi Computer Products (America) Inc.
Software Solutions Division

e-mail: [email protected]

Hi Peter,

I've tried to read through the comments recently posted to this thread (sorry I don't have time to answer them all specifically right now, a lot of this makes a lot of sense). I've tried to come up with a proposal, let's name it "tracepoint", which should hopefully address the full scope of the problem. Please tell me if it makes sense. It should allow compile-time verification of dynamically linked-in and activated tracepoints. I'll work on an implementation ASAP.

Mathieu

Tracepoint proposal

- Tracepoint infrastructure
- In-kernel users
- Complete typing, verified by the compiler
- Dynamically linked and activated

- Marker infrastructure
- Exported API to userland
- Basic types only

- Dynamic vs static
- In-kernel probes are dynamically linked, dynamically activated, connected to tracepoints. Type verification is done at compile-time. Those in-kernel probes can be a probe extracting the information to put in a marker or a specific in-kernel tracer such as ftrace.
- Information sinks (LTTng, SystemTAP) are dynamically connected to the markers inserted in the probes and are dynamically activated.

- Near instrumentation site vs in a separate tracer module

A probe module, only if provided with the kernel tree, could connect to internal tracing sites. This argues for keeping the tracepoing probes near the instrumentation site code. However, if a tracer is general purpose and exports typing information to userspace through some mechanism, it should only export the "basic type" information and could be therefore shipped outside of the kernel tree.

In-kernel probes should be integrated to the kernel tree. They would be close to the instrumented kernel code and would translate between the in-kernel instrumentation and the "basic type" exports. Other in-kernel probes could provide a different output (statistics available through debugfs for instance). ftrace falls into this category.

Generic or specialized information "sinks" (LTTng, systemtap) could be connected to the markers put in tracepoint probes to extract the information to userspace. They would extract both typing information and the per-tracepoint execution information to userspace.

Therefore, the code would look like :

kernel/sched.c:

#include "sched-trace.h"

schedule()
{
...
trace_sched_switch(prev, next);
...
}

kernel/sched-trace.h:

DEFINE_TRACE(sched_switch, struct task_struct *prev, struct task_struct *next);

kernel/sched-trace.c:

#include "sched-trace.h"

static probe_sched_switch(struct task_struct *prev, struct task_struct
*next)
{
trace_mark(kernel_sched_switch, "prev_pid %d next_pid %d prev_state %ld",

    prev->pid, next->pid, prev->state);
}

int __init init(void)
{
return register_sched_switch(probe_sched_switch);
}

void __exit exit(void)
{
unregister_sched_switch(probe_sched_switch);
}

Where DEFINE_TRACE internals declare a structure, a trace_* inline function, a register_trace_* and unregister_trace_* inline functions :