lkml.org 
[lkml]   [2009]   [Feb]   [24]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
Patch in this message
/
Date
From
SubjectRe: [PATCH 2/4] tracing: add event trace infrastructure
On Tue, 24 Feb 2009 21:56:10 -0500 Steven Rostedt <rostedt@goodmis.org> wrote:

> From: Steven Rostedt <srostedt@redhat.com>
>
> This patch creates the event tracing infrastructure of ftrace.
> It will create the files:
>
> /debug/tracing/available_events
> /debug/tracing/set_event
>
> The available_events will list the trace points that have been
> registered with the event tracer.
>
> set_events will allow the user to enable or disable an event hook.
>
> example:
>
> # echo sched_wakeup > /debug/tracing/set_event
>
> Will enable the sched_wakeup event (if it is registered).
>
> # echo "!sched_wakeup" >> /debug/tracing/set_event
>
> Will disable the sched_wakeup event (and only that event).

Why not

echo sched_wakeup > /debug/tracing/set_event
echo sched_wakeup > /debug/tracing/clear_event

?

> # echo > /debug/tracing/set_event
>
> Will disable all events (notice the '>')

echo 1 > /debug/tracing/clear_all_events

?

> # cat /debug/tracing/available_events > /debug/tracing/set_event
>
> Will enable all registered event hooks.
>
> Signed-off-by: Steven Rostedt <srostedt@redhat.com>
> ---
> include/asm-generic/vmlinux.lds.h | 11 ++-
> kernel/trace/Kconfig | 9 ++
> kernel/trace/Makefile | 1 +
> kernel/trace/trace_events.c | 280 +++++++++++++++++++++++++++++++++++++
> kernel/trace/trace_events.h | 52 +++++++
> 5 files changed, 352 insertions(+), 1 deletions(-)
> create mode 100644 kernel/trace/trace_events.c
> create mode 100644 kernel/trace/trace_events.h
>
> diff --git a/include/asm-generic/vmlinux.lds.h b/include/asm-generic/vmlinux.lds.h
> index c61fab1..0add6b2 100644
> --- a/include/asm-generic/vmlinux.lds.h
> +++ b/include/asm-generic/vmlinux.lds.h
> @@ -61,6 +61,14 @@
> #define BRANCH_PROFILE()
> #endif
>
> +#ifdef CONFIG_EVENT_TRACER
> +#define FTRACE_EVENTS() VMLINUX_SYMBOL(__start_ftrace_events) = .; \
> + *(_ftrace_events) \
> + VMLINUX_SYMBOL(__stop_ftrace_events) = .;
> +#else
> +#define FTRACE_EVENTS()
> +#endif
> +
> /* .data section */
> #define DATA_DATA \
> *(.data) \
> @@ -81,7 +89,8 @@
> *(__tracepoints) \
> VMLINUX_SYMBOL(__stop___tracepoints) = .; \
> LIKELY_PROFILE() \
> - BRANCH_PROFILE()
> + BRANCH_PROFILE() \
> + FTRACE_EVENTS()
>
> #define RO_DATA(align) \
> . = ALIGN((align)); \
> diff --git a/kernel/trace/Kconfig b/kernel/trace/Kconfig
> index 07877f4..999c6a2 100644
> --- a/kernel/trace/Kconfig
> +++ b/kernel/trace/Kconfig
> @@ -159,6 +159,15 @@ config CONTEXT_SWITCH_TRACER
> This tracer gets called from the context switch and records
> all switching of tasks.
>
> +config EVENT_TRACER
> + bool "Trace various events in the kernel"
> + depends on DEBUG_KERNEL
> + select TRACING
> + help
> + This tracer hooks to various trace points in the kernel
> + allowing the user to pick and choose which trace point they
> + want to trace.
> +
> config BOOT_TRACER
> bool "Trace boot initcalls"
> depends on DEBUG_KERNEL
> diff --git a/kernel/trace/Makefile b/kernel/trace/Makefile
> index 627090b..c736356 100644
> --- a/kernel/trace/Makefile
> +++ b/kernel/trace/Makefile
> @@ -38,5 +38,6 @@ obj-$(CONFIG_POWER_TRACER) += trace_power.o
> obj-$(CONFIG_KMEMTRACE) += kmemtrace.o
> obj-$(CONFIG_WORKQUEUE_TRACER) += trace_workqueue.o
> obj-$(CONFIG_BLK_DEV_IO_TRACE) += blktrace.o
> +obj-$(CONFIG_EVENT_TRACER) += trace_events.o
>
> libftrace-y := ftrace.o
> diff --git a/kernel/trace/trace_events.c b/kernel/trace/trace_events.c
> new file mode 100644
> index 0000000..05bc80e
> --- /dev/null
> +++ b/kernel/trace/trace_events.c
> @@ -0,0 +1,280 @@
> +/*
> + * event tracer
> + *
> + * Copyright (C) 2008 Red Hat Inc, Steven Rostedt <srostedt@redhat.com>
> + *
> + */
> +
> +#include <linux/debugfs.h>
> +#include <linux/uaccess.h>
> +#include <linux/module.h>
> +#include <linux/ctype.h>
> +
> +#include "trace_events.h"
> +
> +void event_trace_printk(unsigned long ip, const char *fmt, ...)
> +{
> + va_list ap;
> +
> + va_start(ap, fmt);
> + tracing_record_cmdline(current);
> + trace_vprintk(ip, task_curr_ret_stack(current), fmt, ap);
> + va_end(ap);
> +}
> +
> +static void ftrace_clear_events(void)
> +{
> + struct ftrace_event_call *call = (void *)__start_ftrace_events;
__start_ftrace_events has type `unsigned long'. It is instantiated by
the linker. There's a very old unix convention tha tthese things have
type `int'. Doesn't matter.

It's a bit strange to do the double-cast like this - one explicit, one
implicit. Doesn't matter much.

> +
> +

The patch adds various random \n\n's

> + while ((unsigned long)call < (unsigned long)__stop_ftrace_events) {
> +

and various random \n's

> + if (call->enabled) {
> + call->enabled = 0;
> + call->unregfunc();
> + }
> + call++;
> + }
> +}
> +
> +static int ftrace_set_clr_event(char *buf, int set)
> +{
> + struct ftrace_event_call *call = (void *)__start_ftrace_events;
> +
> +
> + while ((unsigned long)call < (unsigned long)__stop_ftrace_events) {
> +
> + if (strcmp(buf, call->name) != 0) {
> + call++;
> + continue;
> + }
> +
> + if (set) {
> + /* Already set? */
> + if (call->enabled)
> + return 0;
> + call->enabled = 1;
> + call->regfunc();
> + } else {
> + /* Already cleared? */
> + if (!call->enabled)
> + return 0;
> + call->enabled = 0;
> + call->unregfunc();
> + }
> + return 0;
> + }
> + return -EINVAL;
> +}

I spose if I looked at this and surrounding code enough, I could work
out what it's for.

> +/* 128 should be much more than enough */
> +#define EVENT_BUF_SIZE 127
> +
> +static ssize_t
> +ftrace_event_write(struct file *file, const char __user *ubuf,
> + size_t cnt, loff_t *ppos)
> +{
> + size_t read = 0;
> + int i, set = 1;
> + ssize_t ret;
> + char *buf;
> + char ch;
> +
> + if (!cnt || cnt < 0)
> + return 0;

cnt is unsigned, and I don't think the VFS lets through either zero or
-ve counts (I always forget).

> + ret = get_user(ch, ubuf++);
> + if (ret)
> + return ret;
> + read++;
> + cnt--;
> +
> + /* skip white space */
> + while (cnt && isspace(ch)) {
> + ret = get_user(ch, ubuf++);
> + if (ret)
> + return ret;
> + read++;
> + cnt--;
> + }
> +
> + /* Only white space found? */
> + if (isspace(ch)) {
> + file->f_pos += read;
> + ret = read;
> + return ret;
> + }
> +
> + buf = kmalloc(EVENT_BUF_SIZE+1, GFP_KERNEL);
> + if (!buf)
> + return -ENOMEM;
> +
> + if (cnt > EVENT_BUF_SIZE)
> + cnt = EVENT_BUF_SIZE;
> +
> + i = 0;
> + while (cnt && !isspace(ch)) {
> + if (!i && ch == '!')
> + set = 0;
> + else
> + buf[i++] = ch;
> +
> + ret = get_user(ch, ubuf++);
> + if (ret)
> + goto out_free;
> + read++;
> + cnt--;
> + }
> + buf[i] = 0;

Gad, what a lot of stuff.

Use strncpy_from_user()?

Use strstrip()?

Why do we care about leading and trailing whitespace - user error!

Then we can use sysfs_streq().

If we really really need to do all this, how's about positioning it as
a general copy_string_from_userspace_then_trim_the_ends() for other
callsites to use?

> + file->f_pos += read;
> +
> + ret = ftrace_set_clr_event(buf, set);
> + if (ret)
> + goto out_free;
> +
> + ret = read;
> +
> + out_free:
> + kfree(buf);
> +
> + return ret;
> +}
>
> ..
>
>
> +static int
> +ftrace_event_seq_open(struct inode *inode, struct file *file)
> +{
> + int ret;
> + const struct seq_operations *seq_ops;
> +
> + if ((file->f_mode & FMODE_WRITE) &&
> + !(file->f_flags & O_APPEND))
> + ftrace_clear_events();
> +
> + seq_ops = inode->i_private;
> + ret = seq_open(file, seq_ops);
> + if (!ret) {
> + struct seq_file *m = file->private_data;
> +
> + m->private = __start_ftrace_events;
> + }
> + return ret;
> +}

It would be nice if the code were to somewhere document the userspace
interface which it is attempting to implement. It's a bit hard to
follow if the reader doesn't already know this.




\
 
 \ /
  Last update: 2009-02-25 04:51    [W:0.180 / U:0.348 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site