Re: [tracepoint] cargo-culting considered harmful...

Mathieu Desnoyers
2013-01-25T16:14:58Z

* Steven Rostedt (rostedt@goodmis.org) wrote:
> On Fri, 2013-01-25 at 09:38 -0500, Mathieu Desnoyers wrote:
>
> > Yep, I'd be OK with removing this example, since now all users are
> > expected to user TRACE_EVENT(), which is built on top of tracepoints.
>
> Can I get your Acked-by for the following patch?

Sure,

Acked-by: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>

Thanks!

Mathieu

>
> -- Steve
>
> commit 867a31fab0a064e54147371425f9fdef933e3d1d
> Author: Steven Rostedt <srostedt@redhat.com>
> Date: Fri Jan 25 09:46:36 2013 -0500
>
> tracing: Remove tracepoint sample code
>
> The tracepoint sample code was used to teach developers how to
> create their own tracepoints. But now the trace_events have been
> added as a higher level that is used directly by developers today.
>
> Only the trace_event code should use the tracepoint interface
> directly and no new tracepoints should be added.
>
> Besides, the example had a race condition with the use of the
> ->d_name.name dentry field, as pointed out by Al Viro.
>
> Best just to remove the code so it wont be used by other developers.
>
> Cc: Al Viro <viro@ZenIV.linux.org.uk>
> Cc: Mathieu Desnoyers <mathieu.desnoyers@polymtl.ca>
> Signed-off-by: Steven Rostedt <rostedt@goodmis.org>
>
> diff --git a/samples/Kconfig b/samples/Kconfig
> index 7b6792a..6181c2c 100644
> --- a/samples/Kconfig
> +++ b/samples/Kconfig
> @@ -5,12 +5,6 @@ menuconfig SAMPLES
>
> if SAMPLES
>
> -config SAMPLE_TRACEPOINTS
> - tristate "Build tracepoints examples -- loadable modules only"
> - depends on TRACEPOINTS && m
> - help
> - This build tracepoints example modules.
> -
> config SAMPLE_TRACE_EVENTS
> tristate "Build trace_events examples -- loadable modules only"
> depends on EVENT_TRACING && m
> diff --git a/samples/Makefile b/samples/Makefile
> index 5ef08bb..1a60c62 100644
> --- a/samples/Makefile
> +++ b/samples/Makefile
> @@ -1,4 +1,4 @@
> # Makefile for Linux samples code
>
> -obj-$(CONFIG_SAMPLES) += kobject/ kprobes/ tracepoints/ trace_events/ \
> +obj-$(CONFIG_SAMPLES) += kobject/ kprobes/ trace_events/ \
> hw_breakpoint/ kfifo/ kdb/ hidraw/ rpmsg/ seccomp/
> diff --git a/samples/tracepoints/Makefile b/samples/tracepoints/Makefile
> deleted file mode 100644
> index 36479ad..0000000
> --- a/samples/tracepoints/Makefile
> +++ /dev/null
> @@ -1,6 +0,0 @@
> -# builds the tracepoint example kernel modules;
> -# then to use one (as root): insmod <module_name.ko>
> -
> -obj-$(CONFIG_SAMPLE_TRACEPOINTS) += tracepoint-sample.o
> -obj-$(CONFIG_SAMPLE_TRACEPOINTS) += tracepoint-probe-sample.o
> -obj-$(CONFIG_SAMPLE_TRACEPOINTS) += tracepoint-probe-sample2.o
> diff --git a/samples/tracepoints/tp-samples-trace.h b/samples/tracepoints/tp-samples-trace.h
> deleted file mode 100644
> index 4d46be9..0000000
> --- a/samples/tracepoints/tp-samples-trace.h
> +++ /dev/null
> @@ -1,11 +0,0 @@
> -#ifndef _TP_SAMPLES_TRACE_H
> -#define _TP_SAMPLES_TRACE_H
> -
> -#include <linux/proc_fs.h> /* for struct inode and struct file */
> -#include <linux/tracepoint.h>
> -
> -DECLARE_TRACE(subsys_event,
> - TP_PROTO(struct inode *inode, struct file *file),
> - TP_ARGS(inode, file));
> -DECLARE_TRACE_NOARGS(subsys_eventb);
> -#endif
> diff --git a/samples/tracepoints/tracepoint-probe-sample.c b/samples/tracepoints/tracepoint-probe-sample.c
> deleted file mode 100644
> index 744c0b9..0000000
> --- a/samples/tracepoints/tracepoint-probe-sample.c
> +++ /dev/null
> @@ -1,57 +0,0 @@
> -/*
> - * tracepoint-probe-sample.c
> - *
> - * sample tracepoint probes.
> - */
> -
> -#include <linux/module.h>
> -#include <linux/file.h>
> -#include <linux/dcache.h>
> -#include "tp-samples-trace.h"
> -
> -/*
> - * Here the caller only guarantees locking for struct file and struct inode.
> - * Locking must therefore be done in the probe to use the dentry.
> - */
> -static void probe_subsys_event(void *ignore,
> - struct inode *inode, struct file *file)
> -{
> - path_get(&file->f_path);
> - dget(file->f_path.dentry);
> - printk(KERN_INFO "Event is encountered with filename %s\n",
> - file->f_path.dentry->d_name.name);
> - dput(file->f_path.dentry);
> - path_put(&file->f_path);
> -}
> -
> -static void probe_subsys_eventb(void *ignore)
> -{
> - printk(KERN_INFO "Event B is encountered\n");
> -}
> -
> -static int __init tp_sample_trace_init(void)
> -{
> - int ret;
> -
> - ret = register_trace_subsys_event(probe_subsys_event, NULL);
> - WARN_ON(ret);
> - ret = register_trace_subsys_eventb(probe_subsys_eventb, NULL);
> - WARN_ON(ret);
> -
> - return 0;
> -}
> -
> -module_init(tp_sample_trace_init);
> -
> -static void __exit tp_sample_trace_exit(void)
> -{
> - unregister_trace_subsys_eventb(probe_subsys_eventb, NULL);
> - unregister_trace_subsys_event(probe_subsys_event, NULL);
> - tracepoint_synchronize_unregister();
> -}
> -
> -module_exit(tp_sample_trace_exit);
> -
> -MODULE_LICENSE("GPL");
> -MODULE_AUTHOR("Mathieu Desnoyers");
> -MODULE_DESCRIPTION("Tracepoint Probes Samples");
> diff --git a/samples/tracepoints/tracepoint-probe-sample2.c b/samples/tracepoints/tracepoint-probe-sample2.c
> deleted file mode 100644
> index 9fcf990..0000000
> --- a/samples/tracepoints/tracepoint-probe-sample2.c
> +++ /dev/null
> @@ -1,44 +0,0 @@
> -/*
> - * tracepoint-probe-sample2.c
> - *
> - * 2nd sample tracepoint probes.
> - */
> -
> -#include <linux/module.h>
> -#include <linux/fs.h>
> -#include "tp-samples-trace.h"
> -
> -/*
> - * Here the caller only guarantees locking for struct file and struct inode.
> - * Locking must therefore be done in the probe to use the dentry.
> - */
> -static void probe_subsys_event(void *ignore,
> - struct inode *inode, struct file *file)
> -{
> - printk(KERN_INFO "Event is encountered with inode number %lu\n",
> - inode->i_ino);
> -}
> -
> -static int __init tp_sample_trace_init(void)
> -{
> - int ret;
> -
> - ret = register_trace_subsys_event(probe_subsys_event, NULL);
> - WARN_ON(ret);
> -
> - return 0;
> -}
> -
> -module_init(tp_sample_trace_init);
> -
> -static void __exit tp_sample_trace_exit(void)
> -{
> - unregister_trace_subsys_event(probe_subsys_event, NULL);
> - tracepoint_synchronize_unregister();
> -}
> -
> -module_exit(tp_sample_trace_exit);
> -
> -MODULE_LICENSE("GPL");
> -MODULE_AUTHOR("Mathieu Desnoyers");
> -MODULE_DESCRIPTION("Tracepoint Probes Samples");
> diff --git a/samples/tracepoints/tracepoint-sample.c b/samples/tracepoints/tracepoint-sample.c
> deleted file mode 100644
> index f4d89e0..0000000
> --- a/samples/tracepoints/tracepoint-sample.c
> +++ /dev/null
> @@ -1,57 +0,0 @@
> -/* tracepoint-sample.c
> - *
> - * Executes a tracepoint when /proc/tracepoint-sample is opened.
> - *
> - * (C) Copyright 2007 Mathieu Desnoyers <mathieu.desnoyers@polymtl.ca>
> - *
> - * This file is released under the GPLv2.
> - * See the file COPYING for more details.
> - */
> -
> -#include <linux/module.h>
> -#include <linux/sched.h>
> -#include <linux/proc_fs.h>
> -#include "tp-samples-trace.h"
> -
> -DEFINE_TRACE(subsys_event);
> -DEFINE_TRACE(subsys_eventb);
> -
> -struct proc_dir_entry *pentry_sample;
> -
> -static int my_open(struct inode *inode, struct file *file)
> -{
> - int i;
> -
> - trace_subsys_event(inode, file);
> - for (i = 0; i < 10; i++)
> - trace_subsys_eventb();
> - return -EPERM;
> -}
> -
> -static const struct file_operations mark_ops = {
> - .open = my_open,
> - .llseek = noop_llseek,
> -};
> -
> -static int __init sample_init(void)
> -{
> - printk(KERN_ALERT "sample init\n");
> - pentry_sample = proc_create("tracepoint-sample", 0444, NULL,
> - &mark_ops);
> - if (!pentry_sample)
> - return -EPERM;
> - return 0;
> -}
> -
> -static void __exit sample_exit(void)
> -{
> - printk(KERN_ALERT "sample exit\n");
> - remove_proc_entry("tracepoint-sample", NULL);
> -}
> -
> -module_init(sample_init)
> -module_exit(sample_exit)
> -
> -MODULE_LICENSE("GPL");
> -MODULE_AUTHOR("Mathieu Desnoyers");
> -MODULE_DESCRIPTION("Tracepoint sample");
>
>
>

--
Mathieu Desnoyers
EfficiOS Inc.